[jit] refactor tryMatchSchema by zdevito · Pull Request #26499 · pytorch/pytorch

zdevito · 2019-09-19T21:49:16Z

Stack from ghstack:

[jit] refactor tryMatchSchema #26499 [jit] refactor tryMatchSchema
[JIT] add type refinements for isinstance checks #26271 [JIT] add type refinements for isinstance checks
[jit] refactor extra sugared values #26270 [jit] refactor extra sugared values
[jit] add dynamic isinstance #26269 [jit] add dynamic isinstance
Allow types as node attributes #26268 Allow types as node attributes
[JIT] add CondValue to unify refinements and code emission #26145 [JIT] add CondValue to unify refinements and code emission

We've changed how these functions are used over time, so I cleaned up
the header file API to match. In particular:

tryMatchSchemas was added since the overload logic got copy/pasted
into three separate locations.
With this change, tryMatchSchema is no longer public, as it is not needed
outside of tryMatchSchemas
emitBuiltinFunction no longer needs a requires argument (it was always true)
Argument order for all the schema matching stuff now puts the 'self'
builtin override last. This is only rarely used and was inconsistent with
matchSchema
Removed OverloadedFunctionValue and OverloadedMethodValue, this functionality was folded into FunctionValue and MethodValue to keep the number of sugared values low.

Differential Revision: D17488297

eellison

Looks great!! Pasta be gone 🚫 🍝

I have a couple comments / questions

eellison · 2019-09-24T18:02:12Z

torch/csrc/jit/script/schema_matching.cpp

+    const c10::optional<NamedValue>& self,
+    bool render_errors) {
+  TORCH_INTERNAL_ASSERT(schemas.size() > 0);
+  if (schemas.size() == 1) {


Why on schema size of 1 are we not trying allow_conversions ?

matchSchema does do conversions. This is avoiding the duplicate work of first trying without conversions, which is only done when there is multiple schema in order to prioritize non-converting matches first.

Maybe add comment

eellison · 2019-09-24T18:04:27Z

torch/csrc/jit/script/schema_matching.cpp

+  // error strings. If we discover it did error, then we replay it, recording
+  // the errors.
+  if (!render_errors) {
+    return matchSchemas(


i don't understand this - if we have reached this point in the code, none of the schemas match, can't we just throw the error ?

We start with render_errors=false, optimistically assuming that we will succeed and not spending time doing error message formatting for an overload that won't happen. If we reach here we know there will be an error, but we have not rendered the error messages, so we replay the match to render the errors.

I assume this is just to cut down on compile time. It would be nice to hide the render_errors argument from the api since any invocation outside of this file shouldn't need to know about this.

Related: we ran into a compile - slow down in compiler.cpp with eagerly concatenating strings and it was solved by buffering up the concatenation with lambdas:

pytorch/torch/csrc/jit/script/compiler.cpp

Line 236 in 264c438

auto runner = this;

eellison · 2019-09-24T18:04:33Z

torch/csrc/jit/script/schema_matching.cpp

+                         << "The following variants are available:\n"
+                         << prefixLine(failure_messages.str(), "  ")
+                         << "\nThe original call is";
+  throw ErrorReport(loc) << failure_messages.str();


unreachable code

eellison · 2019-09-24T18:07:53Z

torch/csrc/jit/script/schema_matching.cpp

+    return emitBuiltinNode(matched.second, loc, graph, name);
+  } else {
+    Function* fn = builtin_functions[matched.first - variants.size()];
+    return insertGraph(graph, *fn->graph(), matched.second.inputs).at(0);


keep comment around ?

we inline builtin calls because they are normally very small // wrappers and are not useful for keeping around to debug

Context: i ran into this when trying to add a builtin_functions with a loop that failed and the comment was helpful

eellison

Looks good. I think we should probably hide the render_errors arg with an impl pattern or something

eellison · 2019-09-24T22:03:22Z

torch/csrc/jit/script/schema_matching.cpp

+    const c10::optional<NamedValue>& self,
+    bool render_errors) {
+  TORCH_INTERNAL_ASSERT(schemas.size() > 0);
+  if (schemas.size() == 1) {


Maybe add comment

eellison · 2019-09-24T22:20:26Z

torch/csrc/jit/script/schema_matching.cpp

+  // error strings. If we discover it did error, then we replay it, recording
+  // the errors.
+  if (!render_errors) {
+    return matchSchemas(


I assume this is just to cut down on compile time. It would be nice to hide the render_errors argument from the api since any invocation outside of this file shouldn't need to know about this.

Related: we ran into a compile - slow down in compiler.cpp with eagerly concatenating strings and it was solved by buffering up the concatenation with lambdas:

pytorch/torch/csrc/jit/script/compiler.cpp

Line 236 in 264c438

auto runner = this;

eellison · 2019-09-24T22:21:02Z

torch/csrc/jit/script/schema_matching.cpp

+    return emitBuiltinNode(matched.second, loc, graph, name);
+  } else {
+    Function* fn = builtin_functions[matched.first - variants.size()];
+    return insertGraph(graph, *fn->graph(), matched.second.inputs).at(0);


Context: i ran into this when trying to add a builtin_functions with a loop that failed and the comment was helpful

[jit] refactor tryMatchSchema We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema gh-metadata: pytorch pytorch 26499 gh/zdevito/112/head

facebook-github-bot · 2019-10-10T07:09:22Z

@zdevito merged this pull request in 51656ee.

ezyang · 2019-10-11T00:34:43Z

Oct 10 07:42:45 test_interface (__main__.TestClassType) ... terminate called after throwing an instance of 'std::runtime_error'
Oct 10 07:42:45   what():  pybind11_object_dealloc(): Tried to deallocate unregistered instance!
Oct 10 07:42:45 Traceback (most recent call last):
Oct 10 07:42:45   File "test/run_test.py", line 458, in <module>
Oct 10 07:42:45     main()
Oct 10 07:42:45   File "test/run_test.py", line 450, in main
Oct 10 07:42:45     raise RuntimeError(message)
Oct 10 07:42:45 RuntimeError: test_jit failed! Received signal: SIGIOT

seems to be real

ezyang · 2019-10-11T00:34:48Z

https://app.circleci.com/jobs/github/pytorch/pytorch/3145541

We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema

We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema gh-metadata: pytorch pytorch 27725 gh/zdevito/123/head

We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema

refactor tryMatchSchema (#26499) We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema gh-metadata: pytorch pytorch 27773 gh/zdevito/125/head

Summary: Pull Request resolved: #27773 We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema Test Plan: Imported from OSS Differential Revision: D17885425 Pulled By: zdevito fbshipit-source-id: 064bc9fa4bd57b2e5366fff9f3c6ab9b9945e08b

Summary: Pull Request resolved: pytorch#26499 We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema Test Plan: Imported from OSS Differential Revision: D17488297 Pulled By: zdevito fbshipit-source-id: a32d838ce35544972fa8767557acc22149081b55

Summary: Pull Request resolved: pytorch#27773 We've changed how these functions are used over time, so I cleaned up the header file API to match. In particular: * tryMatchSchemas was added since the overload logic got copy/pasted into three separate locations. * With this change, tryMatchSchema is no longer public, as it is not needed outside of tryMatchSchemas * emitBuiltinFunction no longer needs a requires argument (it was always true) * Argument order for all the schema matching stuff now puts the 'self' builtin override last. This is only rarely used and was inconsistent with matchSchema Test Plan: Imported from OSS Differential Revision: D17885425 Pulled By: zdevito fbshipit-source-id: 064bc9fa4bd57b2e5366fff9f3c6ab9b9945e08b

zdevito requested a review from apaszke as a code owner September 19, 2019 21:49

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 19, 2019

This was referenced Sep 19, 2019

[JIT] add CondValue to unify refinements and code emission #26145

Closed

Allow types as node attributes #26268

Closed

[jit] refactor extra sugared values #26270

Closed

[JIT] add type refinements for isinstance checks #26271

Closed

zdevito requested a review from eellison September 19, 2019 21:51

zdevito mentioned this pull request Sep 20, 2019

[jit] Allow 'Any' to appear as a type argument. #26572

Closed

eellison reviewed Sep 24, 2019

View reviewed changes

zdevito mentioned this pull request Sep 24, 2019

[jit] Fix circular deps in loading #26758

Closed

eellison approved these changes Sep 24, 2019

View reviewed changes

This was referenced Sep 24, 2019

[jit] Remove attempToRecoverType #26767

Closed

[jit] remove dead LEGACY_PythonPrint #26786

Closed

[jit] Make PythonPrint a class #26787

Closed

[jit] clean up exported source format #26788

Merged

zdevito added 5 commits September 25, 2019 13:23

zdevito mentioned this pull request Sep 27, 2019

Remove unnecessary Node* closures in operator registration #27007

Closed

zdevito added 3 commits September 30, 2019 16:45

zdevito mentioned this pull request Oct 8, 2019

[jit] always use the closure to resolve variable names #27571

Closed

zdevito added 4 commits October 8, 2019 15:18

facebook-github-bot closed this in 51656ee Oct 10, 2019

facebook-github-bot added the merged label Oct 10, 2019

zdevito mentioned this pull request Oct 11, 2019

refactor tryMatchSchema (#26499) #27725

Closed

This was referenced Oct 11, 2019

[JIT] add type refinements for isinstance checks #27772

Closed

refactor tryMatchSchema (#26499) #27773

Closed

facebook-github-bot deleted the gh/zdevito/112/head branch October 28, 2019 22:23

mruberry added the Merged label Oct 28, 2020

Conversation

zdevito commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 10, 2019

Uh oh!

ezyang commented Oct 11, 2019

Uh oh!

ezyang commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zdevito commented Sep 19, 2019 •

edited

Loading