KEMBAR78
[ONNX] Use the torchlib opset number and fix opset import logic by justinchuby · Pull Request #141413 · pytorch/pytorch · GitHub
Skip to content

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Nov 23, 2024

Stack from ghstack (oldest at bottom):

  • Update the ONNX IR add_opset_imports pass to remove the heuristics of taking the max of the seen opsets. Instead, it uses the torchlib default opset version for the model's opset_import. The version converter is able to take the true opset versions in the nodes and convert the model to the correct version.
  • Update all hard coding of opset 18 to instead query the default torchlib opset from onnxscript, introduced in [Stable APIs] Create torchlib_opset for torch 2.6 microsoft/onnxscript#1963

Fixes #141260

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 23, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/141413

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 9e84596 with merge base 259a00b (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

justinchuby added a commit that referenced this pull request Nov 23, 2024
@pytorch-bot pytorch-bot bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Nov 23, 2024
@justinchuby justinchuby added module: onnx Related to torch.onnx topic: improvements topic category and removed fx labels Nov 23, 2024
…logic"


- Update the ONNX IR `add_opset_imports` pass to remove the heuristics of taking the `max` of the seen opsets. Instead, it uses the torchlib default opset version for the model's opset_import. The version converter is able to take the true opset versions in the nodes and convert the model to the correct version.
- Update all hard coding of opset 18 to instead query the default torchlib opset from onnxscript, introduced in microsoft/onnxscript#1963

Fixes #141260

[ghstack-poisoned]
justinchuby added a commit that referenced this pull request Nov 24, 2024
# so we don't need to specifically process them.
with onnxscript.evaluator.default_as(tracer):
output = onnxscript.opset18.Concat(*sequence_mixed_elements, axis=0) # type: ignore[type-var]
output = onnxscript_apis.torchlib_opset().Concat(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops in _building.py also needs the changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opset version is used in https://github.com/pytorch/pytorch/pull/141413/files#diff-c2f19ccd7bc57c1edd3281f4d758f616c933599307fded5d3ad3d063a5313222R115 which is then used in

opset = _get_onnxscript_opset(registry.opset_version)
and
return onnxscript.values.Opset("", opset_version)
. So we should be good

[ghstack-poisoned]
justinchuby added a commit that referenced this pull request Nov 24, 2024
@justinchuby justinchuby added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 24, 2024
@justinchuby justinchuby requested a review from xadupre November 24, 2024 18:50
@justinchuby justinchuby added this to the 2.6.0 milestone Nov 24, 2024
@titaiwangms
Copy link
Collaborator

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
…rch#141413)

- Update the ONNX IR `add_opset_imports` pass to remove the heuristics of taking the `max` of the seen opsets. Instead, it uses the torchlib default opset version for the model's opset_import. The version converter is able to take the true opset versions in the nodes and convert the model to the correct version.
- Update all hard coding of opset 18 to instead query the default torchlib opset from onnxscript, introduced in microsoft/onnxscript#1963

Fixes pytorch#141260
Pull Request resolved: pytorch#141413
Approved by: https://github.com/titaiwangms
pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
…rch#141413)

- Update the ONNX IR `add_opset_imports` pass to remove the heuristics of taking the `max` of the seen opsets. Instead, it uses the torchlib default opset version for the model's opset_import. The version converter is able to take the true opset versions in the nodes and convert the model to the correct version.
- Update all hard coding of opset 18 to instead query the default torchlib opset from onnxscript, introduced in microsoft/onnxscript#1963

Fixes pytorch#141260
Pull Request resolved: pytorch#141413
Approved by: https://github.com/titaiwangms
@github-actions github-actions bot deleted the gh/justinchuby/103/head branch January 18, 2025 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: onnx Related to torch.onnx open source release notes: onnx torch.onnx related changes that should show up in the release notes topic: improvements topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants