10000 [ONNX] Remove special handling of torchvision.ops imports in onnx export by Bludator · Pull Request #141569 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

[ONNX] Remove special handling of torchvision.ops imports in onnx export #141569

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[ONNX] Remove special handling of torchvision.ops imports in onnx export #141569

wants to merge 6 commits into from

Conversation

Bludator
Copy link
Contributor

Fixes #141568

@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Nov 26, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

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

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

@pytorch-bot pytorch-bot bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Nov 26, 2024
@colesbury colesbury added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 26, 2024
justinchuby
justinchuby previously requested changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator
@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

This will remove support for the torch vision ops, right? Is that intended?

@justinchuby
Copy link
Collaborator

Ok sorry. I think what should be done instead is to remove "ops." from the get attr line

justinchuby
justinchuby reviewed Nov 26, 2024
View reviewed changes
@titaiwangms
Copy link
Collaborator

@justinchuby I tested this locally, and it actually fixes the bug. We do have torchvision::nms. It's the bug in registration that we wrongly registered it under the function nms itself. In here, it will register torchvision::nms symbolic function under nms function itself because that's what we get from return getattr(torchvision.ops, op_name). In fact, torchvision ops are accessible the same way as aten ops it seems.

@titaiwangms
Copy link
Collaborator

@Bludator Do you mind adding your repro as a test here: https://github.com/titaiwangms/pytorch/blob/main/test/onnx/exporter/test_small_models_e2e.py 8000

@titaiwangms titaiwangms added the topic: bug fixes topic category label Nov 27, 2024
@justinchuby
Copy link
Collaborator

@justinchuby I tested this locally, and it actually fixes the bug. We do have torchvision::nms. It's the bug in registration that we wrongly registered it under the function nms itself. In here, it will register torchvision::nms symbolic function under nms function itself because that's what we get from return getattr(torchvision.ops, op_name). In fact, torchvision ops are accessible the same way as aten ops it seems.

thank you!

@titaiwangms titaiwangms changed the title Remove special handling of torchvision.ops imports in onnx export [ONNX] Remove special handling of torchvision.ops imports in onnx export Nov 27, 2024
@justinchuby justinchuby added this to the 2.6.0 milestone Nov 27, 2024
justinchuby
justinchuby reviewed Nov 27, 2024
View reviewed changes
Copy link
Collaborator
@justinchuby 10000 justinchuby left a comment

Choose a reason for hiding this comment

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

LGTM if torch.ops.torchvision.* is defined

@Bludator
Copy link
Contributor Author

@Bludator Do you mind adding your repro as a test here: https://github.com/titaiwangms/pytorch/blob/main/test/onnx/exporter/test_small_models_e2e.py

Is the torchvision installed in the test environment?

@titaiwangms
Copy link
Collaborator

@Bludator Do you mind adding your repro as a test here: https://github.com/titaiwangms/pytorch/blob/main/test/onnx/exporter/test_small_models_e2e.py

Is the torchvision installed in the test environment?

I believe so. We have torchvision tests for legacy torchscript exporter.

titaiwangms
titaiwangms reviewed Nov 28, 2024
View reviewed changes
titaiwangms
titaiwangms approved these changes Nov 28, 2024
View reviewed changes
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.

Thanks!

@Bludator @titaiwangms
Update test/onnx/exporter/test_small_models_e2e.py
148880f 
Co-authored-by: Ti-Tai Wang <titaiwang@microsoft.com>
@titaiwangms
Copy link
Collaborator

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 28, 2024
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@Bludator
Copy link
Contributor Author

Now it should work, I had broken environment so I didn't properly test it.
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 3 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@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

pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
@Bludator @justinchuby @titaiwangms
[ONNX] Remove special handling of torchvision.ops imports in onnx exp…
0d70a34 
…ort (pytorch#141569)

Fixes pytorch#141568

Pull Request resolved: pytorch#141569
Approved by: https://github.com/titaiwangms

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: Ti-Tai Wang <titaiwang@microsoft.com>
@atalman atalman mentioned this pull request Jan 13, 2025
Closed
73 tasks
@titaiwangms titaiwangms mentioned this pull request Feb 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinchuby justinchuby justinchuby left review comments

@titaiwangms titaiwangms titaiwangms approved these changes

@shubhambhokare1 shubhambhokare1 Awaiting requested review from shubhambhokare1 shubhambhokare1 is a code owner

@wschin wschin Awaiting requested review from wschin wschin is a code owner

Assignees

@titaiwangms titaiwangms

Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: onnx torch.onnx related changes that should show up in the release notes topic: bug fixes topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

ONNX export (dynamo) not working with torchvision ops
6 participants
@Bludator @justinchuby @titaiwangms @pytorchmergebot @colesbury @pytorchbot
0