-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make tests pass with enable_first_class_module() enabled. #21565
Conversation
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
…bled." [WIP] make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
make tests pass with enable_first_class_module() enabled. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this to be splitted into several PRs, since some of the changes are completely independent - e.g. quantization and requires_grad
changes. And you already proposed a good separation in the description. Apart from that, the changes look good with just some minor comments (inline).
@@ -10083,7 +10079,7 @@ def traced_fn(x): | |||
FileCheck().check("aten::neg").check("aten::add").run(str(traced_fn.graph)) | |||
|
|||
def test_call_script_mod_from_tracing_fn(self): | |||
with self.disableEmitHook(): | |||
with self.assertRaisesRegex(RuntimeError, "must be registered as submodules"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileCheck
call on line 10101 should probably be removed with this change.
Value* definition; | ||
IValue value; | ||
}; | ||
static void gatherParamsFirstClass(script::Module& module, Value* module_value, std::vector<ParamValue>& params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-format please 😛
torch/csrc/jit/python_tracer.cpp
Outdated
@@ -79,7 +79,7 @@ std::shared_ptr<torch::jit::Graph> createGraphByTracing( | |||
"captured in traces, so it would be a no-op."); | |||
} | |||
tracer::exit({toIValue(out)}); | |||
if (!script::getFirstClassMode()) { | |||
if (true || !script::getFirstClassMode()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this some leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, Ill remove it
torch/csrc/jit/script/compiler.cpp
Outdated
@@ -3073,7 +3073,7 @@ void runCleanupPasses(std::shared_ptr<Graph>& to_clean, bool convert_ssa) { | |||
// be inlined into forked closures | |||
liftClosures(to_clean); | |||
inlineForkedClosures(to_clean); | |||
if (!script::getFirstClassMode()) { | |||
if (!script::getFirstClassMode() || true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this.
@@ -99,6 +99,8 @@ struct TORCH_API Method { | |||
return *function_; | |||
} | |||
|
|||
std::pair<std::shared_ptr<Graph>, std::vector<at::Tensor>> _lowered_graph(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some documentation.
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution: * Fixed a bug where Objects were not considered requires_grad when one of their members might be. The default requires_grad now just checks contained types for requires grad, which should be more robust. * Edited some tests to be less fragile by removing checks for implementation details and putting some tests on more standard interfaces. * Provided `_lower_graph()` call that can be used by ONNX to still get the lowered exportable graph. gh-metadata: pytorch pytorch 21565 gh/zdevito/60/head
Summary: Pull Request resolved: pytorch/pytorch#21565 ghimport-source-id: d1fe735fb7821eadc59116fb921d8fe39a49f818 Reviewed By: driazati Differential Revision: D15729503 Pulled By: zdevito fbshipit-source-id: fabb678f040d21fae7545e3b2be1d098e24c544e
Grabbag of fixes required to make it possible to pass the test suite using first-class module execution:
_lower_graph()
call that can be used by ONNX to still get the lowered exportable graph.Stack from ghstack:
Differential Revision: D15729503