[JIT] Add mutability checks in FunctionSchema and create SchemaInfo subclass #80734

ttanpcs · 2022-06-30T19:54:05Z

Stack from ghstack (oldest at bottom):

Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable.
Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature.
Tested is_mutable method overloads in new test_schema_info.cpp file.

Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits

Differential Revision: D37651384

…ubclass [ghstack-poisoned]

facebook-github-bot · 2022-06-30T19:54:12Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/80734
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

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

Expand to see more

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

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

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

…ubclass ghstack-source-id: c21f7cd Pull Request resolved: #80734

…chemaInfo subclass" [ghstack-poisoned]

…ubclass ghstack-source-id: af25011 Pull Request resolved: #80734

…chemaInfo subclass" [ghstack-poisoned]

ttanpcs · 2022-07-06T16:34:44Z

@goldenxuett has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…chemaInfo subclass" - Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable. - Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature. - Tested is_mutable method overloads in new test_schema_info.cpp file. **Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits** Differential Revision: [D37651384](https://our.internmc.facebook.com/intern/diff/D37651384) [ghstack-poisoned]

ttanpcs · 2022-07-06T17:52:31Z

@goldenxuett has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

davidberard98

LGTM!

davidberard98 · 2022-07-06T18:18:04Z

test/cpp/jit/test_schema_info.cpp

+
+namespace torch {
+namespace utils {
+namespace {


very minor nit / my personal preference: I think this doesn't need to be in an anonymous namespace. AFAIK we typically put utility functions and functions that are only used within a given cpp file into an anonymous namespace, but tests are sort of user-facing (and also most of the other jit test files don't put the tests in anonymous namespaces)

…chemaInfo subclass" - Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable. - Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature. - Tested is_mutable method overloads in new test_schema_info.cpp file. **Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits** Differential Revision: [D37651384](https://our.internmc.facebook.com/intern/diff/D37651384) [ghstack-poisoned]

…ubclass ghstack-source-id: c444cec Pull Request resolved: #80734

ttanpcs · 2022-07-06T18:32:47Z

@goldenxuett has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

aten/src/ATen/core/function_schema.h

Gamrix · 2022-07-06T23:00:42Z

torch/csrc/utils/schema_info.h

+ * Subclass of FunctionSchema that publicizes argument value specific operator
+ * behavior (mutation, aliasing, special cases, etc...)
+ */
+
+struct TORCH_API SchemaInfo : c10::FunctionSchema {
+ public:
+  explicit SchemaInfo(c10::FunctionSchema schema) : FunctionSchema(schema) {}
+  explicit SchemaInfo(const char* signature)
+      : FunctionSchema(torch::jit::getOperatorForLiteral(signature)->schema()) {
+  }
+};


@davidberard98 Should we be concerned about performance impacts from virtualizing FunctionSchema? FunctionSchema is used in Core.

A way to avoid this is by using composition instead of inheritance.

Are any methods on FunctionSchema actually getting virtualized in this PR? It looks like SchemaInfo inherits from FunctionSchema` (but without virtualizing any methods). I think that should be fine.

@bdhirsh but in future PRs there would presumably be some virtualized methods (i.e. is_mutable, which on FunctionSchema will return static results but on SchemaInfo will return data-dependent results)

@bdhirsh Look at https://github.com/pytorch/pytorch/pull/80972/files for functions in SchemaInfo that we would be virtualizing. (They are currently not marked as virtual, but I expect that we would need them to be virtual in order for things to work as intended).

Ah ok, in agreement with John then on composition over inheritance. FunctionSchema is probably hot-path enough that we want to avoid virtualizing some of its common methods.

…chemaInfo subclass" - Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable. - Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature. - Tested is_mutable method overloads in new test_schema_info.cpp file. **Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits** Differential Revision: [D37651384](https://our.internmc.facebook.com/intern/diff/D37651384) [ghstack-poisoned]

…ubclass ghstack-source-id: 17b9e24 Pull Request resolved: #80734

ttanpcs · 2022-07-08T00:15:38Z

@goldenxuett has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…chemaInfo subclass" - Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable. - Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature. - Tested is_mutable method overloads in new test_schema_info.cpp file. **Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits** Differential Revision: [D37651384](https://our.internmc.facebook.com/intern/diff/D37651384) [ghstack-poisoned]

…ubclass ghstack-source-id: 68bfa9f Pull Request resolved: #80734

ttanpcs · 2022-07-08T18:32:53Z

@goldenxuett has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

davidberard98

looks good!

facebook-github-bot · 2022-07-11T19:11:42Z

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

pytorchmergebot · 2022-07-11T19:13:01Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-07-11T19:14:16Z

Hey @goldenxuett.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

…ubclass (#80734) Summary: Pull Request resolved: #80734 - Added overloads to is_mutable method in FunctionSchema to tell whether an argument at index is mutable or an argument with name is mutable. - Created SchemaInfo subclass of FunctionSchema with constructors from FunctionSchema and from const char* signature. - Tested is_mutable method overloads in new test_schema_info.cpp file. **Note that this pr is used to set up SchemaInfo. Implementation for SchemaInfo will be addressed in later commits** Test Plan: Imported from OSS Reviewed By: davidberard98 Differential Revision: D37651384 Pulled By: goldenxuett fbshipit-source-id: 5e3f618758590ad518cfbd14f0ab50f010049e09

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

5bb8bae

…ubclass [ghstack-poisoned]

facebook-github-bot added the cla signed label Jun 30, 2022

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 30, 2022

ttanpcs pushed a commit that referenced this pull request Jun 30, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

5b35c60

…ubclass ghstack-source-id: c21f7cd Pull Request resolved: #80734

Update on "[JIT] Add mutability checks in FunctionSchema and create S…

da2ed9c

…chemaInfo subclass" [ghstack-poisoned]

ttanpcs pushed a commit that referenced this pull request Jun 30, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

01b0a36

…ubclass ghstack-source-id: af25011 Pull Request resolved: #80734

Update on "[JIT] Add mutability checks in FunctionSchema and create S…

67b646a

…chemaInfo subclass" [ghstack-poisoned]

ttanpcs mentioned this pull request Jul 5, 2022

[JIT] Add may_alias in FunctionSchema with associated tests #80918

Closed

ttanpcs requested review from Gamrix, Krovatkin, bdhirsh, davidberard98 and eellison July 6, 2022 16:47

ttanpcs mentioned this pull request Jul 6, 2022

[JIT] Add argumentValue functions and is_mutable checks to SchemaInfo #80972

Closed

davidberard98 approved these changes Jul 6, 2022

View reviewed changes

goldenxuett added 2 commits July 6, 2022 11:27

ttanpcs pushed a commit that referenced this pull request Jul 6, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

1c53eb3

…ubclass ghstack-source-id: c444cec Pull Request resolved: #80734

ttanpcs mentioned this pull request Jul 6, 2022

[JIT] Add aliasing checks in SchemaInfo with associated tests #80984

Closed

davidberard98 suggested changes Jul 6, 2022

View reviewed changes

aten/src/ATen/core/function_schema.h Show resolved Hide resolved

davidberard98 approved these changes Jul 6, 2022

View reviewed changes

aten/src/ATen/core/function_schema.h Show resolved Hide resolved

This was referenced Jul 6, 2022

[JIT] Add determinism checks for ops in SchemaInfo subclass #81000

Closed

[JIT] Add side effects checks for ops in SchemaInfo subclass #81002

Closed

[JIT] Add special cases batch_norm, instance_norm and dropout for SchemaInfo #81007

Closed

Gamrix reviewed Jul 6, 2022

View reviewed changes

ttanpcs pushed a commit that referenced this pull request Jul 8, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

5b6f046

…ubclass ghstack-source-id: 17b9e24 Pull Request resolved: #80734

ttanpcs requested review from davidberard98 and removed request for davidberard98 July 8, 2022 16:42

ttanpcs pushed a commit that referenced this pull request Jul 8, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo s…

473725f

…ubclass ghstack-source-id: 68bfa9f Pull Request resolved: #80734

davidberard98 approved these changes Jul 8, 2022

View reviewed changes

pytorchmergebot added the Merged label Jul 11, 2022

pytorchmergebot closed this in b4e3429 Jul 11, 2022

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo subclass #80734

[JIT] Add mutability checks in FunctionSchema and create SchemaInfo subclass #80734

Uh oh!

Conversation

ttanpcs commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

ttanpcs commented Jul 6, 2022

Uh oh!

ttanpcs commented Jul 6, 2022

Uh oh!

davidberard98 left a comment

Choose a reason for hiding this comment

Uh oh!

davidberard98 Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

ttanpcs commented Jul 6, 2022

Uh oh!

Uh oh!

Uh oh!

Gamrix Jul 6, 2022 • 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!

bdhirsh Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

davidberard98 Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

Gamrix Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

bdhirsh Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

ttanpcs commented Jul 8, 2022

Uh oh!

ttanpcs commented Jul 8, 2022

Uh oh!

davidberard98 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 11, 2022

Uh oh!

pytorchmergebot commented Jul 11, 2022

Uh oh!

github-actions bot commented Jul 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ttanpcs commented Jun 30, 2022 •

edited

Loading

facebook-github-bot commented Jun 30, 2022 •

edited

Loading

Gamrix Jul 6, 2022 •

edited

Loading