[JIT] Add default arguments for class types #45098

SplitInfinity · 2020-09-22T00:46:20Z

Stack from ghstack:

[JIT] Add default arguments for class types #45098 [JIT] Add default arguments for class types

Summary
This commit adds support for default arguments in methods of class
types. Similar to how default arguments are supported for regular
script functions and methods on scripted modules, default values are
retrieved from the definition of a TorchScript class in Python as Python
objects, converted to IValues, and then attached to the schemas of
already compiled class methods.

Test Plan
This commit adds a set of new tests to TestClassType to test default
arguments.

Fixes
This commit fixes #42562.

Differential Revision: D23844769

**Summary** This commit adds support for default arguments in methods of class types. Similar to how default arguments are supported for regular script functions and methods on scripted modules, default values are retrieved from the definition of a TorchScript class in Python as Python objects, converted to IValues, and then attached to the schemas of already compiled class methods. **Test Plan** This commit adds a set of new tests to TestClassType to test default arguments. **Fixes** This commit fixes #42562. [ghstack-poisoned]

**Summary** This commit adds support for default arguments in methods of class types. Similar to how default arguments are supported for regular script functions and methods on scripted modules, default values are retrieved from the definition of a TorchScript class in Python as Python objects, converted to IValues, and then attached to the schemas of already compiled class methods. **Test Plan** This commit adds a set of new tests to TestClassType to test default arguments. **Fixes** This commit fixes #42562. ghstack-source-id: 9987789 Pull Request resolved: #45098

SplitInfinity · 2020-09-22T00:53:18Z

This is a different approach to #42562 than #42988 that requires less changes but uses the same approach that we use for methods and functions (take Python objects representing the default values, convert them to IValues, and attach them to the schemas after compilation).

dr-ci · 2020-09-22T00:57:19Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

Failed: GitHub Actions - clang-tidy

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

eellison

🚢

eellison · 2020-09-22T16:10:20Z

torch/jit/frontend.py

+
+    # Get method defaults. Property defaults do not need to be considered
+    # because setters cannot be invoked without a value.
+    defaults = {method[0]: get_default_args(method[1]) for method in methods}


nit maybe: defaults = {method_name: get_default_args(method_impl)) for method_name, method_impl in methods} to be a little more readable

Woah, I didn't know you could unpack tuples like that inside a comprehension but it makes sense. Cool!

**Summary** This commit adds support for default arguments in methods of class types. Similar to how default arguments are supported for regular script functions and methods on scripted modules, default values are retrieved from the definition of a TorchScript class in Python as Python objects, converted to IValues, and then attached to the schemas of already compiled class methods. **Test Plan** This commit adds a set of new tests to TestClassType to test default arguments. **Fixes** This commit fixes #42562. [ghstack-poisoned]

gmagogsfm · 2020-09-22T18:03:54Z

torch/csrc/jit/python/script_init.cpp

+          auto def_name = (*defs_it).name().name();
+          // If the method is not in the defaults map, assume there are
+          // no default arguments for it.
+          if (defaults.find(def_name) == defaults.end()) {


nit:

save results of defaults.find(def_name) and reuse on line 1364 to look up only once

Nice catch.

gmagogsfm · 2020-09-22T18:06:29Z

torch/jit/frontend.py

+        A Dict[str, Dict[str, Any]] which maps each method name to a Dict[str, Any]
+        that maps each argument name to its default value.
+    """
+    # Get methods (except static methods because those are compiled separately as


OOC, are default arg values for static methods already supported?

Yeah, static methods of classes are actually compiled as if they were standalone methods (since there is no self argument or associated instance).

gmagogsfm · 2020-09-22T18:08:06Z

torch/jit/frontend.py

+        cls,
+        predicate=lambda m: (inspect.ismethod(m) or inspect.isfunction(m))
+        and not is_static_fn(cls, m.__name__)
+        and m.__name__ in cls.__dict__


Could you shed some light on why needing to check m.__name__ in cls.__dict__? in case user doesn't want to expose this method?

To be honest I copied this from get_jit_class_def.

**Summary** This commit adds support for default arguments in methods of class types. Similar to how default arguments are supported for regular script functions and methods on scripted modules, default values are retrieved from the definition of a TorchScript class in Python as Python objects, converted to IValues, and then attached to the schemas of already compiled class methods. **Test Plan** This commit adds a set of new tests to TestClassType to test default arguments. **Fixes** This commit fixes #42562. Differential Revision: [D23844769](https://our.internmc.facebook.com/intern/diff/D23844769) [ghstack-poisoned]

**Summary** This commit adds support for default arguments in methods of class types. Similar to how default arguments are supported for regular script functions and methods on scripted modules, default values are retrieved from the definition of a TorchScript class in Python as Python objects, converted to IValues, and then attached to the schemas of already compiled class methods. **Test Plan** This commit adds a set of new tests to TestClassType to test default arguments. **Fixes** This commit fixes #42562. ghstack-source-id: 90b2eeb Pull Request resolved: #45098

codecov · 2020-09-22T23:08:00Z

Codecov Report

Merging #45098 into gh/splitinfinity/47/base will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                    Coverage Diff                    @@
##           gh/splitinfinity/47/base   #45098   +/-   ##
=========================================================
  Coverage                     67.87%   67.88%           
=========================================================
  Files                           384      384           
  Lines                         50095    50100    +5     
=========================================================
+ Hits                          34004    34009    +5     
  Misses                        16091    16091

Impacted Files	Coverage Δ
torch/jit/_script.py	`91.35% <100.00%> (+0.02%)`	⬆️
torch/jit/frontend.py	`90.98% <100.00%> (+0.07%)`	⬆️

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 c253b10...20ba7f7. Read the comment docs.

facebook-github-bot · 2020-09-23T02:18:17Z

@SplitInfinity merged this pull request in e045119.

facebook-github-bot · 2020-09-23T02:18:29Z

@SplitInfinity merged this pull request in e045119.

SplitInfinity requested a review from apaszke as a code owner September 22, 2020 00:46

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 22, 2020

SplitInfinity mentioned this pull request Sep 22, 2020

[JIT] Add support for default args in class types #42988

Closed

SplitInfinity requested review from eellison and gmagogsfm September 22, 2020 00:51

eellison approved these changes Sep 22, 2020

View reviewed changes

gmagogsfm approved these changes Sep 22, 2020

View reviewed changes

facebook-github-bot closed this in e045119 Sep 23, 2020

facebook-github-bot added the merged label Sep 23, 2020

facebook-github-bot deleted the gh/splitinfinity/47/head branch September 26, 2020 14:15

[JIT] Add default arguments for class types #45098

[JIT] Add default arguments for class types #45098

Uh oh!

Conversation

SplitInfinity commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SplitInfinity commented Sep 22, 2020

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 1 failed

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

eellison Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Sep 22, 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!

gmagogsfm Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

gmagogsfm Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

gmagogsfm Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Sep 23, 2020

Uh oh!

facebook-github-bot commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SplitInfinity commented Sep 22, 2020 •

edited

Loading

dr-ci bot commented Sep 22, 2020 •

edited

Loading

SplitInfinity Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading