Skip to content
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

[ONNX] Update the default opset to version 14 #83284

Closed
wants to merge 6 commits into from

Conversation

justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Aug 11, 2022

Stack from ghstack (oldest at bottom):

Update the default opset by running the update_default_opset_version.py script. The update is done in a regularly to ensure we are in sync with the onnx updates. All changes are produced by the script.

Update the default opset by running the `update_default_opset_version.py` script

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 11, 2022

🔗 Helpful links

❌ 1 New Failures

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

Expand to see more
  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

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

See GitHub Actions build pull / linux-bionic-py3_7-clang8-xla / test (xla, 1, 1, linux.2xlarge) (1/1)

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

2022-08-16T16:59:26.6678052Z ##[error]Process completed with exit code 1.
2022-08-16T16:59:26.6638716Z Non-cacheable calls                   0
2022-08-16T16:59:26.6639064Z Non-compilation calls                 0
2022-08-16T16:59:26.6639265Z Unsupported compiler calls            0
2022-08-16T16:59:26.6639482Z Average cache write               0.000 s
2022-08-16T16:59:26.6639884Z Average cache read miss           0.000 s
2022-08-16T16:59:26.6640176Z Average cache read hit            0.000 s
2022-08-16T16:59:26.6640397Z Failed distributed compilations       0
2022-08-16T16:59:26.6640951Z Cache location                  S3, bucket: Bucket(name=ossci-compiler-cache-circleci-v2, base_url=http://ossci-compiler-cache-circleci-v2.s3.amazonaws.com/)
2022-08-16T16:59:26.6641348Z + echo ::endgroup::
2022-08-16T16:59:26.6641819Z ##[endgroup]
2022-08-16T16:59:26.6678052Z ##[error]Process completed with exit code 1.
2022-08-16T16:59:26.6734172Z Prepare all required actions
2022-08-16T16:59:26.6734470Z Getting action download info
2022-08-16T16:59:26.8545818Z ##[group]Run ./.github/actions/get-workflow-job-id
2022-08-16T16:59:26.8546038Z with:
2022-08-16T16:59:26.8546346Z   github-token: ***
2022-08-16T16:59:26.8546520Z env:
2022-08-16T16:59:26.8546692Z   GIT_DEFAULT_BRANCH: master
2022-08-16T16:59:26.8546863Z ##[endgroup]
2022-08-16T16:59:26.8572956Z ##[group]Run nick-fields/retry@71062288b76e2b6214ebde0e673ce0de1755740a
2022-08-16T16:59:26.8573173Z with:

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.

Update the default opset by running the `update_default_opset_version.py` script

[ghstack-poisoned]
@justinchuby justinchuby linked an issue Aug 11, 2022 that may be closed by this pull request
33 tasks
Update the default opset by running the `update_default_opset_version.py` script

[ghstack-poisoned]
Update the default opset by running the `update_default_opset_version.py` script

[ghstack-poisoned]
Copy link
Collaborator

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

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

Hi @justinchuby, please provide a clear and concise description of why we are changing the default opset. Documenting the reason helps us in the future, when we are debugging problems that are specific to opset version that affects a particular runtime

What is the motivation for the proposal? Changing the default opset can break Caffe2 and any other runtime that consumes. IIRC, it might be because we are trying to stay 3 version behind the latest, is that it?

@justinchuby justinchuby added release notes: onnx torch.onnx related changes that should show up in the release notes topic: improvements topic category labels Aug 12, 2022
@justinchuby
Copy link
Collaborator Author

justinchuby commented Aug 12, 2022

Hi @justinchuby, please provide a clear and concise description of why we are changing the default opset. Documenting the reason helps us in the future, when we are debugging problems that are specific to opset version that affects a particular runtime

What is the motivation for the proposal? Changing the default opset can break Caffe2 and any other runtime that consumes. IIRC, it might be because we are trying to stay 3 version behind the latest, is that it?

Done. PTAL.

The change is done by running the script. As I understand it - yes it is because we want to use the oldest version in the last 18 months (mentioned in the script).

If versions are usually explicitly specified during conversion, I am hoping nothing will break with this change? Do we need an import to verify?

@thiagocrepaldi
Copy link
Collaborator

Hi @justinchuby, please provide a clear and concise description of why we are changing the default opset. Documenting the reason helps us in the future, when we are debugging problems that are specific to opset version that affects a particular runtime
What is the motivation for the proposal? Changing the default opset can break Caffe2 and any other runtime that consumes. IIRC, it might be because we are trying to stay 3 version behind the latest, is that it?

Done. PTAL.

The change is done by running the script. As I understand it - yes it is because we want to use the oldest version in the last 18 months (mentioned in the script).

If versions are usually explicitly specified during conversion, I am hoping nothing will break with this change? Do we need an import to verify?

Assuming people use explicit version on the torch.onnx.export call, nothing should break, but the API also allows not setting it, so a break is possible. Although the ONNX converter team know about the policy for updating the opset, the general public probably dont, so expliciting the reason on the PR is always a good idea. Especially for those that do not explicitly specify opset version and spend days debugging a model change they have no idea where it came from :P

Copy link
Collaborator

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

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

The change LGTM.

One aspect that we never discussed when changing opset versions before, but I thought about it now is: Should we add module: BC-breaking or topic: BC breaking Github labels on the PR that changes opset version?

For scripts that use implicit opset version, the opset change can cause BC issues and the PyTorch release notes should explicit that

@BowenBao @shubhambhokare1 any thoughts?

@justinchuby justinchuby added topic: bc breaking topic category onnx-needs-import This PR is related to ONNX, but touches files outside of merge rule patterns, and hence needs import labels Aug 12, 2022
@BowenBao
Copy link
Collaborator

The change LGTM.

One aspect that we never discussed when changing opset versions before, but I thought about it now is: Should we add module: BC-breaking or topic: BC breaking Github labels on the PR that changes opset version?

For scripts that use implicit opset version, the opset change can cause BC issues and the PyTorch release notes should explicit that

@BowenBao @shubhambhokare1 any thoughts?

I agree with the BC breaking concern and I think it's the cost we have to pay for moving forward. Good idea to add the BC-breaking tags. Linking the original PR from Gary for reference #73898.

@justinchuby I'm not sure about this line Update the default opset ... to prepare for opset 17 support. Why are these related? If there is no dependency I'd actually recommend separating this PR, as it might get reverted for failing Meta internal tests.

@justinchuby
Copy link
Collaborator Author

I agree with the BC breaking concern and I think it's the cost we have to pay for moving forward. Good idea to add the BC-breaking tags. Linking the original PR from Gary for reference #73898.

@justinchuby I'm not sure about this line Update the default opset ... to prepare for opset 17 support. Why are these related? If there is no dependency I'd actually recommend separating this PR, as it might get reverted for failing Meta internal tests.

Thanks for pointing this out. For some reason I just assumed it is the process when there is a new opset coming out. I have detached #83287

Update the default opset by running the `update_default_opset_version.py` script. The update is done in a regularly to ensure we are in sync with the onnx updates. All changes are produced by the script.

[ghstack-poisoned]
justinchuby added a commit that referenced this pull request Aug 12, 2022
Update the default opset by running the `update_default_opset_version.py` script

ghstack-source-id: fe35edbbc06ab78276200db75104705de07f1958
Pull Request resolved: #83284
@titaiwangms titaiwangms self-assigned this Aug 15, 2022
Copy link
Collaborator

@titaiwangms titaiwangms left a comment

Choose a reason for hiding this comment

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

🍺

Update the default opset by running the `update_default_opset_version.py` script. The update is done in a regularly to ensure we are in sync with the onnx updates. All changes are produced by the script.

[ghstack-poisoned]
justinchuby added a commit that referenced this pull request Aug 16, 2022
Update the default opset by running the `update_default_opset_version.py` script

ghstack-source-id: fe35edbbc06ab78276200db75104705de07f1958
Pull Request resolved: #83284
@thiagocrepaldi thiagocrepaldi changed the title [ONNX] Update the default opset [ONNX] Update the default opset to version 14 Aug 16, 2022
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

LGTM, not sure why it has onnx-needs-import label

@justinchuby
Copy link
Collaborator Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

@pytorchmergebot
Copy link
Collaborator

Merge failed
Reason: Refusing to merge as mandatory check(s) pull failed for rule superuser
Raised by https://github.com/pytorch/pytorch/actions/runs/2884173564

@justinchuby
Copy link
Collaborator Author

@pytorchbot merge -f "unrelated xla build error"

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the force (-f) flag. This means your change will be merged immediately, bypassing any CI checks (ETA: 1-5 minutes). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

facebook-github-bot pushed a commit that referenced this pull request Aug 19, 2022
Summary:
Update the default opset by running the `update_default_opset_version.py` script. The update is done in a regularly to ensure we are in sync with the onnx updates. All changes are produced by the script.

Pull Request resolved: #83284
Approved by: https://github.com/AllenTiTaiWang, https://github.com/malfet, https://github.com/BowenBao

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/e4f74f0891da9e49c5c82df05794f7723b05cbac

Reviewed By: atalman

Differential Revision: D38852456

fbshipit-source-id: 0555d1eb1d2d66687ee60c9f3409fca677f7b41b
@facebook-github-bot facebook-github-bot deleted the gh/justinchuby/7/head branch August 22, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: onnx Related to torch.onnx onnx-needs-import This PR is related to ONNX, but touches files outside of merge rule patterns, and hence needs import open source release notes: onnx torch.onnx related changes that should show up in the release notes topic: bc breaking topic category topic: improvements topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ONNX] Support opset 17 operators
8 participants