Refactor dispatcher and native to use Signature structure. by ezyang · Pull Request #45990 · pytorch/pytorch

ezyang · 2020-10-07T20:10:28Z

Stack from ghstack:

Refactor dispatcher and native to use Signature structure. #45990 Refactor dispatcher and native to use Signature structure.
Remove unnecessary byte-for-byte compatibility code that is not needed. #45975 Remove unnecessary byte-for-byte compatibility code that is not needed.
Rename legacy_dispatcher to native. #45974 Rename legacy_dispatcher to native.
Reorder dispatcher/legacy_dispatcher types #45973 Reorder dispatcher/legacy_dispatcher types
Add NativeFunctionGroup #45918 Add NativeFunctionGroup
Rewrite implementation of faithful cpp signatures #45890 Rewrite implementation of faithful cpp signatures

In #45890 we introduced the concept of a CppSignature, which bundled
up all of the information necessary to declare a C++ signature for
the cpp API. This PR introduces analogous concepts for dispatcher
and native: DispatcherSignature and NativeSignature.

The three interfaces are not particularly well coupled right now,
but they do have some duck typing coincidences:

defn() which renders the C++ definition "bool f(int x)"
decl() which renders the C++ declaration "bool f(int x = 2)"
type() which renders the C++ function type "bool(int)"

Maybe at some point we'll introduce a Protocol, or a supertype.
Many other methods (like arguments()) have varying types. These
signatures also have some helper methods that forward back to real
implementations in the api modules. Something to think about is
whether or not we should attempt to reduce boilerplate here or
not; I'm not too sure about it yet.

The net effect is we get to reduce the number of variables we
have to explicitly write out in the codegen, since now these are all
bundled together into a signature. Something extra special happens
in BackendSelect, where we now dynamically select between dispatcher_sig
and native_sig as "how" the backend select is implemented.

A little bit of extra cleanup:

Some places where we previously advertised Sequence, we now advertise
a more informative Tuple.
defn() may take an optional positional parameter overriding the entire
name, or a kwarg-only prefix parameter to just add a prefix to the
name.

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

Differential Revision: D24223100

In #45890 we introduced the concept of a CppSignature, which bundled up all of the information necessary to declare a C++ signature for the cpp API. This PR introduces analogous concepts for dispatcher and native: DispatcherSignature and NativeSignature. The three interfaces are not particularly well coupled right now, but they do have some duck typing coincidences: - defn() which renders the C++ definition "bool f(int x)" - decl() which renders the C++ declaration "bool f(int x = 2)" - type() which renders the C++ function type "bool(int)" Maybe at some point we'll introduce a Protocol, or a supertype. Many other methods (like arguments()) have varying types. These signatures also have some helper methods that forward back to real implementations in the api modules. Something to think about is whether or not we should attempt to reduce boilerplate here or not; I'm not too sure about it yet. The net effect is we get to reduce the number of variables we have to explicitly write out in the codegen, since now these are all bundled together into a signature. Something extra special happens in BackendSelect, where we now dynamically select between dispatcher_sig and native_sig as "how" the backend select is implemented. A little bit of extra cleanup: - Some places where we previously advertised Sequence, we now advertise a more informative Tuple. - defn() may take an optional positional parameter overriding the entire name, or a kwarg-only prefix parameter to just add a prefix to the name. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

In #45890 we introduced the concept of a CppSignature, which bundled up all of the information necessary to declare a C++ signature for the cpp API. This PR introduces analogous concepts for dispatcher and native: DispatcherSignature and NativeSignature. The three interfaces are not particularly well coupled right now, but they do have some duck typing coincidences: - defn() which renders the C++ definition "bool f(int x)" - decl() which renders the C++ declaration "bool f(int x = 2)" - type() which renders the C++ function type "bool(int)" Maybe at some point we'll introduce a Protocol, or a supertype. Many other methods (like arguments()) have varying types. These signatures also have some helper methods that forward back to real implementations in the api modules. Something to think about is whether or not we should attempt to reduce boilerplate here or not; I'm not too sure about it yet. The net effect is we get to reduce the number of variables we have to explicitly write out in the codegen, since now these are all bundled together into a signature. Something extra special happens in BackendSelect, where we now dynamically select between dispatcher_sig and native_sig as "how" the backend select is implemented. A little bit of extra cleanup: - Some places where we previously advertised Sequence, we now advertise a more informative Tuple. - defn() may take an optional positional parameter overriding the entire name, or a kwarg-only prefix parameter to just add a prefix to the name. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: b4214b1 Pull Request resolved: #45990

ezyang · 2020-10-07T20:11:07Z

There is ZERO codegen diff on this PR

codecov · 2020-10-08T00:30:04Z

Codecov Report

Merging #45990 into gh/ezyang/852/base will increase coverage by 0.00%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           gh/ezyang/852/base   #45990   +/-   ##
===================================================
  Coverage               68.28%   68.29%           
===================================================
  Files                     410      410           
  Lines                   53468    53468           
===================================================
+ Hits                    36513    36514    +1     
+ Misses                  16955    16954    -1

Impacted Files	Coverage Δ
torch/testing/_internal/expecttest.py	`78.57% <0.00%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7abe7ba...0970fb5. Read the comment docs.

smessmer · 2020-10-09T10:09:21Z

tools/codegen/api/types.py

        return f"{self.type} {self.name}"

+@dataclass(frozen=True)
+class DispatcherSignature:


Can you add a class comment describing what the dispatcher signature is for and how it differs from NativeSignature / CppSignature ? Same for those classes.

Side note: maybe rename CppSignature to ApiSignature or FrontendSignature since they're all kind of Cpp.

This explanation would overlap with the top level comments in tools.codegen.api.FOO modules, so I'd prefer not to do it here. Maybe a Note reference would be better?

Wondering maybe we can reorganize these modules a bit so that the types aren't splattered about. One possibility is to get rid of types module and move the type definitions to their respective core modules. That will result in cycles when we reference types from the other modules. We could maybe resolve these cycles using the trick in https://mypy.readthedocs.io/en/stable/common_issues.html#import-cycles This looks kind of ugly so I tried not to do it this way, but maybe it is worth it.

Side note: maybe rename CppSignature to ApiSignature or FrontendSignature since they're all kind of Cpp.

Vetoing api because then we'll get tools.codegen.api.api (I guess we could rename the first api in this list, though I'm not sure what too; cconv as in calling convention?) Frontend might be OK; there's some potential for confusion with Python frontend but I guess it's not too bad.

In #45890 we introduced the concept of a CppSignature, which bundled up all of the information necessary to declare a C++ signature for the cpp API. This PR introduces analogous concepts for dispatcher and native: DispatcherSignature and NativeSignature. The three interfaces are not particularly well coupled right now, but they do have some duck typing coincidences: - defn() which renders the C++ definition "bool f(int x)" - decl() which renders the C++ declaration "bool f(int x = 2)" - type() which renders the C++ function type "bool(int)" Maybe at some point we'll introduce a Protocol, or a supertype. Many other methods (like arguments()) have varying types. These signatures also have some helper methods that forward back to real implementations in the api modules. Something to think about is whether or not we should attempt to reduce boilerplate here or not; I'm not too sure about it yet. The net effect is we get to reduce the number of variables we have to explicitly write out in the codegen, since now these are all bundled together into a signature. Something extra special happens in BackendSelect, where we now dynamically select between dispatcher_sig and native_sig as "how" the backend select is implemented. A little bit of extra cleanup: - Some places where we previously advertised Sequence, we now advertise a more informative Tuple. - defn() may take an optional positional parameter overriding the entire name, or a kwarg-only prefix parameter to just add a prefix to the name. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

dr-ci · 2020-10-09T14:46:31Z

💊 CI failures summary and remediations

As of commit 0970fb5 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 7 times.

In #45890 we introduced the concept of a CppSignature, which bundled up all of the information necessary to declare a C++ signature for the cpp API. This PR introduces analogous concepts for dispatcher and native: DispatcherSignature and NativeSignature. The three interfaces are not particularly well coupled right now, but they do have some duck typing coincidences: - defn() which renders the C++ definition "bool f(int x)" - decl() which renders the C++ declaration "bool f(int x = 2)" - type() which renders the C++ function type "bool(int)" Maybe at some point we'll introduce a Protocol, or a supertype. Many other methods (like arguments()) have varying types. These signatures also have some helper methods that forward back to real implementations in the api modules. Something to think about is whether or not we should attempt to reduce boilerplate here or not; I'm not too sure about it yet. The net effect is we get to reduce the number of variables we have to explicitly write out in the codegen, since now these are all bundled together into a signature. Something extra special happens in BackendSelect, where we now dynamically select between dispatcher_sig and native_sig as "how" the backend select is implemented. A little bit of extra cleanup: - Some places where we previously advertised Sequence, we now advertise a more informative Tuple. - defn() may take an optional positional parameter overriding the entire name, or a kwarg-only prefix parameter to just add a prefix to the name. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D24223100](https://our.internmc.facebook.com/intern/diff/D24223100) [ghstack-poisoned]

In #45890 we introduced the concept of a CppSignature, which bundled up all of the information necessary to declare a C++ signature for the cpp API. This PR introduces analogous concepts for dispatcher and native: DispatcherSignature and NativeSignature. The three interfaces are not particularly well coupled right now, but they do have some duck typing coincidences: - defn() which renders the C++ definition "bool f(int x)" - decl() which renders the C++ declaration "bool f(int x = 2)" - type() which renders the C++ function type "bool(int)" Maybe at some point we'll introduce a Protocol, or a supertype. Many other methods (like arguments()) have varying types. These signatures also have some helper methods that forward back to real implementations in the api modules. Something to think about is whether or not we should attempt to reduce boilerplate here or not; I'm not too sure about it yet. The net effect is we get to reduce the number of variables we have to explicitly write out in the codegen, since now these are all bundled together into a signature. Something extra special happens in BackendSelect, where we now dynamically select between dispatcher_sig and native_sig as "how" the backend select is implemented. A little bit of extra cleanup: - Some places where we previously advertised Sequence, we now advertise a more informative Tuple. - defn() may take an optional positional parameter overriding the entire name, or a kwarg-only prefix parameter to just add a prefix to the name. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 9f6f3b8 Pull Request resolved: #45990

facebook-github-bot · 2020-10-13T16:18:55Z

@ezyang merged this pull request in d705083.

ezyang requested review from bhosmer and smessmer October 7, 2020 20:10

ezyang requested a review from bdhirsh October 8, 2020 20:47

smessmer approved these changes Oct 9, 2020

View reviewed changes

ezyang mentioned this pull request Oct 9, 2020

Structured kernel definitions #45277

Closed

ezyang added 2 commits October 12, 2020 08:55

facebook-github-bot closed this in d705083 Oct 13, 2020

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

facebook-github-bot deleted the gh/ezyang/852/head branch October 17, 2020 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dispatcher and native to use Signature structure.#45990

Refactor dispatcher and native to use Signature structure.#45990
ezyang wants to merge 4 commits intogh/ezyang/852/basefrom
gh/ezyang/852/head

ezyang commented Oct 7, 2020 •

edited

Loading

Uh oh!

ezyang commented Oct 7, 2020

Uh oh!

codecov bot commented Oct 8, 2020 •

edited

Loading

Uh oh!

smessmer Oct 9, 2020

Uh oh!

ezyang Oct 9, 2020

Uh oh!

dr-ci bot commented Oct 9, 2020 •

edited

Loading

Uh oh!

facebook-github-bot commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Uh oh!

ezyang commented Oct 7, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

smessmer Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ezyang commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

dr-ci bot commented Oct 9, 2020 •

edited

Loading