[JIT] Allow module interface subtyping to skip argument name matching #47950

SplitInfinity · 2020-11-14T00:11:07Z

Stack from ghstack:

[JIT] Allow module interface subtyping to skip argument name matching #47950 [JIT] Allow module interface subtyping to skip argument name matching

Summary
This commit allows users to define a class attribute in a module
interface definition that specifies which arguments of which methods
should be ignored when determining if modules subtype the interface in
question. Methods of interfaces that allow name mismatches cannot be
called with named arguments.

Test Plan
This commit adds a unit test that tests that implementations with
mismatched arguments subtype properly if the interface allows it.

Differential Revision: D24966648

cc @pietern @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @SciPioneer @H-Huang @cbalioglu @gcramer23

…ching **Summary** This commit allows `@torch.jit.interface` to accept a boolean argument that specify whether implementations must have the same argument names for matching methods. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. [ghstack-poisoned]

dr-ci · 2020-11-14T00:12:27Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/47950
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

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

Commit ec8bc05 was recently pushed. Waiting for builds...

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

SplitInfinity · 2020-11-14T00:16:14Z

Pros
👍 simple, restricted to interfaces only
👍 no need to introduce positional args which only exist in 3.7+

Cons
👎 BC-breaking (but the fix is a simple find and replace!)

Rather than adding positional args (because they are not supported in python3.6), I would prefer some way to express the unimportance of argument names on the interface itself (but preferably not in a way that breaks BC like this). Maybe a class attribute?

…nt name matching" **Summary** This commit allows `@torch.jit.interface` to accept a boolean argument that specify whether implementations must have the same argument names for matching methods. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

…ching **Summary** This commit allows `@torch.jit.interface` to accept a boolean argument that specify whether implementations must have the same argument names for matching methods. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. ghstack-source-id: 15858ff Pull Request resolved: #47950

eellison · 2020-11-16T21:05:59Z

I haven't looked into the details of this, but my preference would be for the implementation to set "ignore argument name" to be an argument-level property, instead of a property of the whole schema. The former sets us up to easily support the 3.7 positional-only arg syntax, and is arguably a better API for users of 3.7. It's also "a thing" in python, whereas whole function schema no args isn't.

Other comment is that it's not backwards-incompatible if we add a default -

def interface(*, ignore_argument_names=False) (keyword only is debatable but IMO it's a little more explicit and readable)

SplitInfinity · 2020-11-16T22:25:40Z

What do you think of using a class attribute to indicate which arguments should be ignored?

eellison · 2020-11-16T22:35:55Z

Yea, I could see something like:

@torch.jit.interface
def module():
    ___ignored_argument_names = {"forward": ['x', 'y'], "other_method": ...}

eellison · 2020-11-17T00:36:57Z

The API though, we can do whatever. More just that I htink it would be nice from an implementation perspective to make it easy to switch to the 3.7 syntax when that comes.

…nt name matching" **Summary** This commit allows `@torch.jit.interface` to accept a boolean argument that specify whether implementations must have the same argument names for matching methods. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

…me matching" **Summary** This commit allows users to define a class attribute in a module interface definition that specify which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

…me matching" **Summary** This commit allows users to define a class attribute in a module interface definition that specifies which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

**Summary** This commit allows users to define a class attribute in a module interface definition that specify which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. ghstack-source-id: a747954 Pull Request resolved: #47950

SplitInfinity

I left some comments to highlight items I'd like to discuss.

aten/src/ATen/core/function_schema.h

SplitInfinity · 2020-11-19T02:39:48Z

aten/src/ATen/core/function_schema_inl.h

    std::ostream* why_not) {
  if (child.size() != parent.size()) {
    return false;
  }
  for (size_t i = 0; i < child.size(); ++i) {
    const Argument& c = child[i];
    const Argument& p = parent[i];
-    if (c.name() != p.name()) {
+    if ((ignored_arg_names.count(c.name()) == 0) && (c.name() != p.name())) {


Perhaps there should be another argument specifying whether ignored_arg_names refers to child or parent?

Also, I agree that this implementation facilitates the transition to positional args, but also allows users to write an interface method with the schema (a: Any, b: Any, c: Any) -> Any and ignore the names of a and c during subtyping, something that is not possible with the positional argument syntax unless the arguments are reordered.

If I take a step back, we are working on this solution mainly to unblock tracing issue that could generate arbitrary argument names. From a user's perspective, it is difficult to know which argument names would be changed, even harder to know what they are changed to. Allowing specifying either child/parent argument name would help in this case.

What's the best way to express this? Boolean argument? Enum argument? It feels very awkward.

SplitInfinity · 2020-11-19T03:02:58Z

torch/csrc/jit/serialization/import_source.cpp

+      // Strip the Assign for the ignored argument names from the ClassDef for
+      // the interface because define_interface doesn't handle those.
+      ClassDef stripped_class_def = stripNonDefFromClass(class_def);


This is not elegant but is a necessary consequence of the fact that ClassDef is used to store the AST for interface types and that the parsing machinery for class types is reused for interfaces.

torch/jit/_script.py

aten/src/ATen/core/function_schema.h

gmagogsfm · 2020-11-20T22:11:35Z

aten/src/ATen/core/function_schema_inl.h

    std::ostream* why_not) {
  if (child.size() != parent.size()) {
    return false;
  }
  for (size_t i = 0; i < child.size(); ++i) {
    const Argument& c = child[i];
    const Argument& p = parent[i];
-    if (c.name() != p.name()) {
+    if ((ignored_arg_names.count(c.name()) == 0) && (c.name() != p.name())) {


If I take a step back, we are working on this solution mainly to unblock tracing issue that could generate arbitrary argument names. From a user's perspective, it is difficult to know which argument names would be changed, even harder to know what they are changed to. Allowing specifying either child/parent argument name would help in this case.

torch/csrc/jit/frontend/sugared_value.h

torch/csrc/jit/python/script_init.cpp

torch/csrc/jit/serialization/import_source.cpp

…me matching" **Summary** This commit allows users to define a class attribute in a module interface definition that specifies which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

**Summary** This commit allows users to define a class attribute in a module interface definition that specify which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. ghstack-source-id: c9e2ac7 Pull Request resolved: #47950

…me matching" **Summary** This commit allows users to define a class attribute in a module interface definition that specifies which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. Differential Revision: [D24966648](https://our.internmc.facebook.com/intern/diff/D24966648) [ghstack-poisoned]

**Summary** This commit allows users to define a class attribute in a module interface definition that specify which arguments of which methods should be ignored when determining if modules subtype the interface in question. Methods of interfaces that allow name mismatches cannot be called with named arguments. **Test Plan** This commit adds a unit test that tests that implementations with mismatched arguments subtype properly if the interface allows it. ghstack-source-id: d56a5df Pull Request resolved: #47950

codecov · 2020-12-09T03:46:17Z

Codecov Report

Merging #47950 (ec8bc05) into gh/splitinfinity/67/base (2b70bcd) will increase coverage by 0.00%.
The diff coverage is 96.77%.

@@                    Coverage Diff                    @@
##           gh/splitinfinity/67/base   #47950   +/-   ##
=========================================================
  Coverage                     80.74%   80.74%           
=========================================================
  Files                          1870     1870           
  Lines                        201724   201779   +55     
=========================================================
+ Hits                         162877   162925   +48     
- Misses                        38847    38854    +7

facebook-github-bot · 2021-09-17T09:19:49Z

Hi @SplitInfinity!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

pytorchbot · 2022-04-12T03:21:05Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
Stale pull requests will automatically be closed 30 days after being marked Stale

SplitInfinity requested review from apaszke, mingzhe09088, mrshenli, pietern, pritamdamania87, rohan-varma and zhaojuanmao as code owners November 14, 2020 00:11

facebook-github-bot added the cla signed label Nov 14, 2020

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue oncall: distributed Add this issue/PR to distributed oncall triage queue labels Nov 14, 2020

SplitInfinity requested review from suo, gmagogsfm and eellison November 14, 2020 00:32

SplitInfinity changed the title ~~[WIP][JIT] Allow module interface subtyping to skip argument name matching~~ [JIT] Allow module interface subtyping to skip argument name matching Nov 18, 2020

SplitInfinity commented Nov 19, 2020

View reviewed changes

gmagogsfm reviewed Nov 20, 2020

View reviewed changes

eellison removed their request for review December 2, 2020 23:19

SplitInfinity requested a review from gmagogsfm December 3, 2020 01:43

gmagogsfm removed request for apaszke, pietern and gmagogsfm February 13, 2021 06:28

suo removed their request for review February 19, 2021 19:17

pytorchbot added the open source label Sep 17, 2021

pytorchbot added the Stale label Apr 12, 2022

github-actions bot closed this May 12, 2022

facebook-github-bot deleted the gh/splitinfinity/67/head branch June 11, 2022 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] Allow module interface subtyping to skip argument name matching #47950

[JIT] Allow module interface subtyping to skip argument name matching #47950

SplitInfinity commented Nov 14, 2020 •

edited by pytorch-probot bot

Loading

dr-ci bot commented Nov 14, 2020 •

edited by facebook-github-bot

Loading

SplitInfinity commented Nov 14, 2020 •

edited

Loading

eellison commented Nov 16, 2020

SplitInfinity commented Nov 16, 2020

eellison commented Nov 16, 2020

eellison commented Nov 17, 2020

SplitInfinity left a comment

SplitInfinity Nov 19, 2020 •

edited

Loading

gmagogsfm Nov 20, 2020

SplitInfinity Dec 3, 2020

SplitInfinity Nov 19, 2020

gmagogsfm Nov 20, 2020

codecov bot commented Dec 9, 2020

facebook-github-bot commented Sep 17, 2021

pytorchbot commented Apr 12, 2022

[JIT] Allow module interface subtyping to skip argument name matching #47950

[JIT] Allow module interface subtyping to skip argument name matching #47950

Conversation

SplitInfinity commented Nov 14, 2020 • edited by pytorch-probot bot Loading

dr-ci bot commented Nov 14, 2020 • edited by facebook-github-bot Loading

🔗 Helpful links

💊 CI failures summary and remediations

SplitInfinity commented Nov 14, 2020 • edited Loading

eellison commented Nov 16, 2020

SplitInfinity commented Nov 16, 2020

eellison commented Nov 16, 2020

eellison commented Nov 17, 2020

SplitInfinity left a comment

Choose a reason for hiding this comment

SplitInfinity Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

gmagogsfm Nov 20, 2020

Choose a reason for hiding this comment

SplitInfinity Dec 3, 2020

Choose a reason for hiding this comment

SplitInfinity Nov 19, 2020

Choose a reason for hiding this comment

gmagogsfm Nov 20, 2020

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2020

Codecov Report

facebook-github-bot commented Sep 17, 2021

Process

pytorchbot commented Apr 12, 2022

SplitInfinity commented Nov 14, 2020 •

edited by pytorch-probot bot

Loading

dr-ci bot commented Nov 14, 2020 •

edited by facebook-github-bot

Loading

SplitInfinity commented Nov 14, 2020 •

edited

Loading

SplitInfinity Nov 19, 2020 •

edited

Loading