[ONNX] Refactor _run_symbolic_function by BowenBao · Pull Request #67573 · pytorch/pytorch

BowenBao · 2021-10-29T22:22:47Z

Allows implementing symbolic functions for domains other than aten, for example prim, in symbolic_opset#.py.
Allows symbolic function to access extra context if needed, through SymbolicFunctionState.
- Particularly, the prim::PythonOp special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter.
prim::Loop, prim::If, etc are now moved outside of _run_symbolic_function from utils.py, and to symbolic_opset9.py.

Motivation for this change:

Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter.
The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside _run_symbolic_function. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature prim_[opname], creating separation and confusion.

pytorch-probot · 2021-10-29T22:22:50Z

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/BowenBao/pytorch/blob/81c75bf328b1cfd7d99b4ec79b28de88a4fc42b3/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default

Workflows	Labels (bold enabled)	Status
Triggered Workflows
linux-bionic-py3.6-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/noarch`, `ciflow/xla`	✅ triggered
linux-vulkan-bionic-py3.6-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/vulkan`	✅ triggered
linux-xenial-cuda11.3-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/linux`	✅ triggered
linux-xenial-py3-clang5-mobile-build	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`	✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-dynamic	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`	✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-static	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`	✅ triggered
linux-xenial-py3.6-clang7-asan	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/sanitizers`	✅ triggered
linux-xenial-py3.6-clang7-onnx	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/onnx`	✅ triggered
linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
linux-xenial-py3.6-gcc7	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
linux-xenial-py3.6-gcc7-bazel-test	`ciflow/all`, `ciflow/bazel`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single-full-jit	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`	✅ triggered
win-vs2019-cpu-py3	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/win`	✅ triggered
win-vs2019-cuda11.3-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/win`	✅ triggered
Skipped Workflows
caffe2-linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`	🚫 skipped
docker-builds	`ciflow/all`	🚫 skipped
ios-12-5-1-arm64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-arm64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-arm64-custom-ops	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-arm64-full-jit	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-arm64-metal	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-x86-64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-x86-64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
ios-12-5-1-x86-64-full-jit	`ciflow/all`, `ciflow/ios`, `ciflow/macos`	🚫 skipped
libtorch-linux-xenial-cuda10.2-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`	🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`	🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/slow`	🚫 skipped
linux-xenial-py3-clang5-mobile-code-analysis	`ciflow/all`, `ciflow/linux`, `ciflow/mobile`	🚫 skipped
parallelnative-linux-xenial-py3.6-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`	🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-xenial-cuda10.2-py3-gcc7-slow-gradcheck	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`, `ciflow/slow`, `ciflow/slow-gradcheck`	🚫 skipped
periodic-linux-xenial-cuda11.1-py3.6-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-win-vs2019-cuda11.1-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped

You can add a comment to the PR and tag @pytorchbot with the following commands:

# ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun

# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow

For more information, please take a look at the CI Flow Wiki.

facebook-github-bot · 2021-10-29T22:22:59Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/67573
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

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

2/2 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

linux-xenial-py3.6-gcc5.4 / test (backwards_compat, 1, 1, linux.2xlarge) (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

2021-11-10T18:22:17.5283463Z The PR is introduc...m to confirm whether this change is wanted or not.

2021-11-10T18:22:17.5270810Z processing existing schema:  alltoall_base(__torch__.torch.classes.dist_c10d.ProcessGroup _0, Tensor _1, Tensor _2, int[] _3, int[] _4) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5272124Z processing existing schema:  alltoall(__torch__.torch.classes.dist_c10d.ProcessGroup _0, Tensor[] _1, Tensor[] _2) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5273377Z processing existing schema:  send(__torch__.torch.classes.dist_c10d.ProcessGroup _0, Tensor[] _1, int _2, int _3) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5274623Z processing existing schema:  recv(__torch__.torch.classes.dist_c10d.ProcessGroup _0, Tensor[] _1, int _2, int _3) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5275892Z processing existing schema:  recv_anysource(__torch__.torch.classes.dist_c10d.ProcessGroup _0, Tensor[] _1, int _2) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5277118Z processing existing schema:  barrier(__torch__.torch.classes.dist_c10d.ProcessGroup _0) -> (__torch__.torch.classes.dist_c10d.Work _0)
2021-11-10T18:22:17.5278156Z processing existing schema:  __init__(__torch__.torch.classes.dist_c10d.frontend _0) -> (NoneType _0)
2021-11-10T18:22:17.5279523Z processing existing schema:  new_process_group_helper(__torch__.torch.classes.dist_c10d.frontend _0, int _1, int _2, int[] _3, str _4, __torch__.torch.classes.dist_c10d.Store _5, str? _6, int _7) -> (__torch__.torch.classes.dist_c10d.ProcessGroup _0)
2021-11-10T18:22:17.5281011Z processing existing schema:  get_process_group_by_name(__torch__.torch.classes.dist_c10d.frontend _0, str _1) -> (__torch__.torch.classes.dist_c10d.ProcessGroup _0)
2021-11-10T18:22:17.5282343Z processing existing schema:  get_name_of_process_group(__torch__.torch.classes.dist_c10d.frontend _0, __torch__.torch.classes.dist_c10d.ProcessGroup _1) -> (str _0)
2021-11-10T18:22:17.5283463Z The PR is introducing backward incompatible changes to the operator library. Please contact PyTorch team to confirm whether this change is wanted or not. 
2021-11-10T18:22:17.5284043Z 
2021-11-10T18:22:17.5284286Z Broken ops: [
2021-11-10T18:22:17.5285289Z 	aten::searchsorted.Tensor(Tensor sorted_sequence, Tensor self, *, bool out_int32=False, bool right=False, str? side=None, Tensor? sorter=None) -> (Tensor)
2021-11-10T18:22:17.5286580Z 	aten::searchsorted.Scalar(Tensor sorted_sequence, Scalar self, *, bool out_int32=False, bool right=False, str? side=None, Tensor? sorter=None) -> (Tensor)
2021-11-10T18:22:17.5287893Z 	aten::searchsorted.Tensor_out(Tensor sorted_sequence, Tensor self, *, bool out_int32=False, bool right=False, str? side=None, Tensor? sorter=None, Tensor(a!) out) -> (Tensor(a!))
2021-11-10T18:22:17.5288921Z 	aten::_foreach_norm.Scalar(Tensor[] tensors, Scalar ord=2) -> (Tensor[])
2021-11-10T18:22:17.5289684Z 	aten::split_with_sizes(Tensor(a -> *) self, int[] split_sizes, int dim=0) -> (Tensor[])
2021-11-10T18:22:17.5290435Z 	aten::split.Tensor(Tensor(a -> *) self, int split_size, int dim=0) -> (Tensor[])
2021-11-10T18:22:17.5291310Z 	aten::unbind.int(Tensor(a -> *) self, int dim=0) -> (Tensor[])
2021-11-10T18:22:17.5292036Z 	aten::unbind.Dimname(Tensor(a -> *) self, str dim) -> (Tensor[])

1 failure not recognized by patterns:

Job	Step	Action
^{pytorch_linux_xenial_py3_6_gcc5_4_test}	^{Report results}	🔁 rerun

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.

thiagocrepaldi · 2021-11-01T17:42:13Z

Allows implementing symbolic functions for domains other than aten, for example prim, in symbolic_opset#.py.

Allows symbolic function to access extra context if needed, through SymbolicFunctionState.

Particularly, the prim::PythonOp special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter.

prim::Loop, prim::If, etc are now moved outside of _run_symbolic_function from utils.py, and to symbolic_opset9.py.

As a curiosity what is the motivation for such change?

BowenBao · 2021-11-01T18:02:11Z

As a curiosity what is the motivation for such change?

Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter.

The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside _run_symbolic_function. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature prim_[opname], creating separation and confusion.

thiagocrepaldi

Any reason we don't unify this class approach for aten ops, too? This way we would have a single way implement symbolics: through class <domain> classes

torch/onnx/symbolic_helper.py

BowenBao · 2021-11-01T22:05:41Z

Any reason we don't unify this class approach for aten ops, too? This way we would have a single way implement symbolics: through class <domain> classes

I felt the convention of aten by default for symbolics is commonly accepted now, so being careful in bringing big changes. I agree unifying aten too would be great, it also resolves a lot of builtin shadowing issues, like symbolic for min, max, sum, etc.

torch/onnx/symbolic_helper.py

garymm

Thanks for this change!

While overall I think this is a big improvement, I'd like to discuss an alternative.
Maintaining a global singleton _symbolic_function_state, and having symbolic functions get access to it through a global getter function seems a lot less clean than passing it in as an argument. I find it's generally more obvious for functions to depend on their inputs rather than global state, and we should aim to make the code as obvious as possible.

What do you think of changing all of the symbolic function signatures to take an extra argument containing the state?

My main concern with this is how do we transition the custom symbolic functions that are not in this repo.

Maybe we could do something like:

try:
   symbolic_fn(symbolic_state, g, *inputs, **attrs)
except TypeError as e:
   if is_too_many_positional_args_error(e):
     logging.warn("<function name> is using the old signature. It should be updated to take a symbolic function state as its first argument. This will become a fatal error in the next release of PyTorch")
  symbolic_fn(g, *inputs, **attrs)

We can discuss in person if you prefer.

Unrelated:

As a curiosity what is the motivation for such change?

Better maintainability ...

@BowenBao can you please add this rationale to the PR description?
In general PR descriptions should contain the rationale. It's often very helpful for current and future readers.

torch/onnx/symbolic_helper.py

torch/onnx/symbolic_opset9.py

torch/onnx/symbolic_helper.py

torch/onnx/symbolic_registry.py

garymm

Thanks!

docs/source/onnx.rst

torch/onnx/__init__.py

torch/onnx/symbolic_helper.py

torch/onnx/utils.py

torch/onnx/symbolic_helper.py

torch/onnx/__init__.py

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbao@microsoft.com> [ghstack-poisoned]

…7573)" * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> ghstack-source-id: 577a7e6 Pull Request resolved: pytorch#68491

…7573)" * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

…7573)" * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> ghstack-source-id: 577a7e6 Pull Request resolved: pytorch#68491

…7573)" * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

* Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Co-authored-by: BowenBao <bowbaomicrosoft.com> Differential Revision: [D32483782](https://our.internmc.facebook.com/intern/diff/D32483782) [ghstack-poisoned]

Summary: Pull Request resolved: #68491 * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Test Plan: Imported from OSS Reviewed By: jansel Differential Revision: D32483782 Pulled By: malfet fbshipit-source-id: f9affc31b1570af30ffa6668da9375da111fd54a Co-authored-by: BowenBao <bowbao@microsoft.com>

Summary: Pull Request resolved: #68491 * Allows implementing symbolic functions for domains other than `aten`, for example `prim`, in symbolic_opset#.py. * Allows symbolic function to access extra context if needed, through `SymbolicFunctionState`. * Particularly, the `prim::PythonOp` special case can access node without the need of passing node through inputs. Updates will be made downstreams, and in a follow-up PR we will remove the previous workaround in exporter. * `prim::Loop`, `prim::If`, etc are now moved outside of `_run_symbolic_function` from utils.py, and to symbolic_opset9.py. Motivation for this change: - Better maintainability and reducing complexity. Easier to add symbolic for operators, both simple and complex ones (that need additional context), without the former needing to know the existence of the latter. - The design idea was long outdated. prim ops are no longer rare special cases, and they shouldn't all be handled inside `_run_symbolic_function`. As a result this function becomes too clumsy. There were also prim ops symbolic added in symbolic_opset#.py with signature `prim_[opname]`, creating separation and confusion. Test Plan: Imported from OSS Reviewed By: jansel Differential Revision: D32483782 Pulled By: malfet fbshipit-source-id: f9affc31b1570af30ffa6668da9375da111fd54a Co-authored-by: BowenBao <bowbao@microsoft.com> (cherry picked from commit 1e04ffd)

BowenBao requested review from neginraoof and shubhambhokare1 as code owners October 29, 2021 22:22

pytorch-probot bot added the ciflow/default label Oct 29, 2021

facebook-github-bot added the cla signed label Oct 29, 2021

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 29, 2021

pytorchbot added the open source label Oct 29, 2021

BowenBao force-pushed the onnx_symbolic_method branch from 20ec164 to e9352ff Compare October 30, 2021 04:48

BowenBao mentioned this pull request Nov 1, 2021

[ONNX] Follow-up: Remove the argument example_outpus of export() method entirely. #67629

Merged

thiagocrepaldi reviewed Nov 1, 2021

View reviewed changes

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

thiagocrepaldi self-assigned this Nov 1, 2021

garymm removed the request for review from neginraoof November 1, 2021 20:50

BowenBao force-pushed the onnx_symbolic_method branch 2 times, most recently from 55f24ae to c7a0900 Compare November 3, 2021 00:03

garymm requested changes Nov 3, 2021

View reviewed changes

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

BowenBao force-pushed the onnx_ms_1 branch from 7c78a1f to 1a91e0b Compare November 4, 2021 18:41

BowenBao added 6 commits November 4, 2021 11:46

Refactor _run_symbolic_function

6c54ef6

Add type annotation for SymbolicFunctionState

524b8ed

switch to type comment

cd260ae

instance variable

34b0950

switch back to modern type annotation

dc70bae

Fix mypy;flake8 + regression on unmatching error message

e0680a7

BowenBao force-pushed the onnx_symbolic_method branch from 4d5f5a8 to e0680a7 Compare November 4, 2021 18:46

garymm requested changes Nov 4, 2021

View reviewed changes

BowenBao added 2 commits November 4, 2021 18:03

Re-design SymbolicContext as additional input

e76291f

fix flake8 & mypy

3adb8bb

update type description for mypy

a7e477d

garymm approved these changes Nov 5, 2021

View reviewed changes

BowenBao added 3 commits November 5, 2021 11:24

nit

845c95a

nit

e0fa3f6

Update doc comments

cf01fb6

garymm reviewed Nov 10, 2021

View reviewed changes

torch/onnx/__init__.py Show resolved Hide resolved

BowenBao added 2 commits November 9, 2021 17:36

newline

81e252d

add to doc

81c75bf

BowenBao merged commit 6b19dc9 into pytorch:onnx_ms_1 Nov 11, 2021

Conversation

BowenBao commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-probot bot commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚛️ CI Flow

Uh oh!

facebook-github-bot commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

linux-xenial-py3.6-gcc5.4 / test (backwards_compat, 1, 1, linux.2xlarge) (1/1)

1 failure not recognized by patterns:

Uh oh!

thiagocrepaldi commented Nov 1, 2021

Uh oh!

BowenBao commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagocrepaldi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BowenBao commented Nov 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garymm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garymm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BowenBao commented Oct 29, 2021 •

edited

Loading

pytorch-probot bot commented Oct 29, 2021 •

edited

Loading

facebook-github-bot commented Oct 29, 2021 •

edited

Loading

BowenBao commented Nov 1, 2021 •

edited

Loading