Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp by ezyang · Pull Request #46991 · pytorch/pytorch

ezyang · 2020-10-28T16:01:13Z

Stack from ghstack:

ATen DerivedType is dead, long live ATen RegisterDispatchKey #47011 ATen DerivedType is dead, long live ATen RegisterDispatchKey
Convert from higher order functions to classes in tools.codegen.gen #47008 Convert from higher order functions to classes in tools.codegen.gen
Delete TypeDefault.h and TypeDerived.h codegen entirely. #47002 Delete TypeDefault.h and TypeDerived.h codegen entirely.
Delete TypeDefault call code generation logic in VariableType #47000 Delete TypeDefault call code generation logic in VariableType
Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp #46991 Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp
Desugar missing dispatch field into singleton Math entry #46970 Desugar missing dispatch field into singleton Math entry
Some miscellaneous cleanup in codegen #46940 Some miscellaneous cleanup in codegen
Delete Vulkan from code generator. #46938 Delete Vulkan from code generator.

This change is motivated by a problem bdhirsh observed which is
that in internal builds that include both SchemaRegister.cpp
and TypeDefault.cpp, some operators have their schemas defined
multiple times. Instead of dumping schema registrations in
multiple files, it seems better to just toggle how many schemas
we write into TypeDefault.cpp.

ljk53 observes that technically SchemaRegister.cpp is only needed by
full-JIT frontend, and not by light interpreter (to resolve schema
lookups). However, in practice, the registration file seems to be
unconditionally loaded. This change will make it harder to do the
optimization where we drop schemas in the light interpreter, but you
probably want to architect this differently (similar to per-op
registrations, DON'T do any registrations in ATen, and then write out
the schema registrations in a separate library.)

I took this opportunity to also simplify the TypeDefault generation
logic by reworking things so that we only ever call with None argument
when registering. Soon, we should be able to just split these
files up entirely.

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D24593704

This change is motivated by a problem bdhirsh observed which is that in internal builds that include both SchemaRegister.cpp and TypeDefault.cpp, some operators have their schemas defined multiple times. Instead of dumping schema registrations in multiple files, it seems better to just toggle how many schemas we write into TypeDefault.cpp. ljk53 observes that technically SchemaRegister.cpp is only needed by full-JIT frontend, and not by light interpreter (to resolve schema lookups). However, in practice, the registration file seems to be unconditionally loaded. This change will make it harder to do the optimization where we drop schemas in the light interpreter, but you probably want to architect this differently (similar to per-op registrations, DON'T do any registrations in ATen, and then write out the schema registrations in a separate library.) I took this opportunity to also simplify the TypeDefault generation logic by reworking things so that we only ever call with None argument when registering. Soon, we should be able to just split these files up entirely. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

…t.cpp" This change is motivated by a problem bdhirsh observed which is that in internal builds that include both SchemaRegister.cpp and TypeDefault.cpp, some operators have their schemas defined multiple times. Instead of dumping schema registrations in multiple files, it seems better to just toggle how many schemas we write into TypeDefault.cpp. ljk53 observes that technically SchemaRegister.cpp is only needed by full-JIT frontend, and not by light interpreter (to resolve schema lookups). However, in practice, the registration file seems to be unconditionally loaded. This change will make it harder to do the optimization where we drop schemas in the light interpreter, but you probably want to architect this differently (similar to per-op registrations, DON'T do any registrations in ATen, and then write out the schema registrations in a separate library.) I took this opportunity to also simplify the TypeDefault generation logic by reworking things so that we only ever call with None argument when registering. Soon, we should be able to just split these files up entirely. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

This change is motivated by a problem bdhirsh observed which is that in internal builds that include both SchemaRegister.cpp and TypeDefault.cpp, some operators have their schemas defined multiple times. Instead of dumping schema registrations in multiple files, it seems better to just toggle how many schemas we write into TypeDefault.cpp. ljk53 observes that technically SchemaRegister.cpp is only needed by full-JIT frontend, and not by light interpreter (to resolve schema lookups). However, in practice, the registration file seems to be unconditionally loaded. This change will make it harder to do the optimization where we drop schemas in the light interpreter, but you probably want to architect this differently (similar to per-op registrations, DON'T do any registrations in ATen, and then write out the schema registrations in a separate library.) I took this opportunity to also simplify the TypeDefault generation logic by reworking things so that we only ever call with None argument when registering. Soon, we should be able to just split these files up entirely. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: f1e7c46 Pull Request resolved: #46991

dr-ci · 2020-10-28T16:45:40Z

💊 CI failures summary and remediations

As of commit 819e9ab (more details on the Dr. CI page):

1/2 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)
1/2 broken upstream at merge base c886c7f on Oct 28 from 6:48am to 10:32am PDT (11 commits; 46b252b - 61ee024)

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_linux_backward_compatibility_check_test on Oct 28 from 6:48am to 10:32am PDT (11 commits; 46b252b - 61ee024)
- 🔁 rerun

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.8-py3.6

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 3 times.

bdhirsh · 2020-10-28T16:49:48Z

        args_str = ', '.join(map(str, args))
-        # TODO: don't need dispatch is None shortly
-        dispatch_to_all_backends = dispatch is None or dispatch in KEYWORD_ALL_BACKENDS
+        dispatch_to_all_backends = dispatch is not None and dispatch in KEYWORD_ALL_BACKENDS


nit: we don't really need the is not None check since KEYWORD_ALL_BACKENDS doesn't contain None

Actually we do, because KEYWORD_ALL_BACKENDS is typed as a dict from string to thing, so a None value woudn't be well typed 👍

bdhirsh · 2020-10-28T16:55:03Z

-            return {
-                'schema_registrations': schema_registrations,
-            }
-        cpu_fm.write('SchemaRegister.cpp', computeSchemaRegister)


The old SchemaRegister.cpp file used the TORCH_LIBRARY_FRAGMENT_THIS_API.... macro instead of TORCH_LIBRARY, so merging all of the logic into TypeDefault.cpp means that we no longer use that macro.

Just checking, but I actually think that's fine in this case- right? That macro lets us add multiple blocks that use the same namespace. My guess as to why we originally used that macro in SchemaRegister.cpp is because of the fact that we explicitly had op registration blocks for aten in two files- and now we only have it in one.

(tldr; my round-about way of saying that this change is correct)

Yep that's right.

facebook-github-bot · 2020-10-30T02:15:22Z

@ezyang merged this pull request in 41f8641.

…rch#46991) Summary: Pull Request resolved: pytorch#46991 This change is motivated by a problem bdhirsh observed which is that in internal builds that include both SchemaRegister.cpp and TypeDefault.cpp, some operators have their schemas defined multiple times. Instead of dumping schema registrations in multiple files, it seems better to just toggle how many schemas we write into TypeDefault.cpp. ljk53 observes that technically SchemaRegister.cpp is only needed by full-JIT frontend, and not by light interpreter (to resolve schema lookups). However, in practice, the registration file seems to be unconditionally loaded. This change will make it harder to do the optimization where we drop schemas in the light interpreter, but you probably want to architect this differently (similar to per-op registrations, DON'T do any registrations in ATen, and then write out the schema registrations in a separate library.) I took this opportunity to also simplify the TypeDefault generation logic by reworking things so that we only ever call with None argument when registering. Soon, we should be able to just split these files up entirely. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Reviewed By: ljk53 Differential Revision: D24593704 Pulled By: ezyang fbshipit-source-id: f01ea22a3999493da77b6e254d188da0ce9adf2f

This was referenced Oct 28, 2020

Delete Vulkan from code generator. #46938

Closed

Some miscellaneous cleanup in codegen #46940

Closed

Desugar missing dispatch field into singleton Math entry #46970

Closed

ezyang requested review from bdhirsh and ljk53 October 28, 2020 16:10

ljk53 approved these changes Oct 28, 2020

View reviewed changes

bdhirsh reviewed Oct 28, 2020

View reviewed changes

bdhirsh approved these changes Oct 29, 2020

View reviewed changes

facebook-github-bot closed this in 41f8641 Oct 29, 2020

facebook-github-bot added the Merged label Oct 30, 2020

facebook-github-bot deleted the gh/ezyang/858/head branch November 2, 2020 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp#46991

Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp#46991
ezyang wants to merge 2 commits intogh/ezyang/858/basefrom
gh/ezyang/858/head

ezyang commented Oct 28, 2020 •

edited

Loading

Uh oh!

dr-ci Bot commented Oct 28, 2020 •

edited

Loading

Uh oh!

bdhirsh Oct 28, 2020

Uh oh!

ezyang Oct 28, 2020

Uh oh!

bdhirsh Oct 28, 2020 •

edited

Loading

Uh oh!

ezyang Oct 28, 2020

Uh oh!

facebook-github-bot commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ezyang commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci Bot commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🚧 1 fixed upstream failure:

ci.pytorch.org: 1 failed

Uh oh!

bdhirsh Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

bdhirsh Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ezyang commented Oct 28, 2020 •

edited

Loading

dr-ci Bot commented Oct 28, 2020 •

edited

Loading

bdhirsh Oct 28, 2020 •

edited

Loading