NotificationsYou must be signed in to change notification settings
Fork26.3k
Star96k

[ONNX] Delete JitTraceConvertStrategy#152556

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

titaiwangms wants to merge3 commits intopytorch:mainfromtitaiwangms:titaiwang/sort_out_export_strategies

Closed

[ONNX] Delete JitTraceConvertStrategy#152556

titaiwangms wants to merge3 commits intopytorch:mainfromtitaiwangms:titaiwang/sort_out_export_strategies

+23 −205

Conversation

Copy link

Collaborator

titaiwangms commentedApr 30, 2025

Fixes#151703

delete two strategies

3f3f1bc

titaiwangms requested review fromjustinchuby,shubhambhokare1 andwschin ascode owners

April 30, 2025 20:51

Copy link

pytorch-botbot commentedApr 30, 2025•
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results athud.pytorch.org/pr/152556

📄 PreviewPython docs built from this PR
📄 PreviewC++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit thebot commands wiki or ouroffice hours

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

✅ You can merge normally! (1 Unrelated Failure)

As of commite534772 with merge base28efeb1 ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉Rebase onto the `viable/strict` branch to avoid these failures

trunk / macos-py3-arm64 / test (default, 2, 3, macos-m1-stable) (gh) (trunk failure)
test_optim.py::TestOptimRenewedMPS::test_can_load_from_to_named_state_dict_is_named_optim0_False_is_named_optim1_False_Adafactor_mps_float32

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

pytorch-botbot added the release notes: onnxtorch.onnx related changes that should show up in the release notes label

Apr 30, 2025

pytorchbot added the open source label

Apr 30, 2025

justinchuby approved these changes

Apr 30, 2025

View reviewed changes

Copy link

Collaborator

justinchuby left a comment

Choose a reason for hiding this comment

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

Can you go to reporting and remove the corresponding entries for the reports?

Copy link

Collaborator

justinchuby commentedApr 30, 2025

#152467 This makes me reconsider the strict=True case

Copy link

CollaboratorAuthor

titaiwangms commentedApr 30, 2025

#152467 This makes me reconsider the strict=True case

I think the real question is still "does it make sense for onnx to use torch.export.export(..., strict=True)".

Copy link

Collaborator

justinchuby commentedApr 30, 2025

I think if we know strict=False covers a superset of models than strict=True. Is there a downside?

Copy link

CollaboratorAuthor

titaiwangms commentedApr 30, 2025•
edited
Loading

I think if we know strict=False covers a superset of models than strict=True. Is there a downside?

The case we have here to support having another capture strategy looks like just a bug. I am not sure how much benefit we really gain from keeping multiple strategies. Personally, I don't like the long print out. And I believe that the reason of torch.export having this parameter is more than strict=False could have bugs.

Copy link

Collaborator

justinchuby commentedApr 30, 2025•
edited
Loading

And I believe that the reason of torch.export having this parameter is more than strict=False could have bugs.

I think it is changing the default to False now.@tugsbayasgalan @angelayi is it the plan to deprecate strict=True at some point? Is there additional value in using strict=True? Thanks!

justinchuby approved these changes

May 1, 2025

View reviewed changes

clean up _reporting anf fix func input test

8305154

Copy link

Contributor

angelayi commentedMay 1, 2025

is it the plan to deprecate strict=True at some point? Is there additional value in using strict=True?

I don't think we plan on deprecating strict=True. Because torch.compile is going through that path, depending on how it's implemented, some new features could be better supported in the strict=True path.

Copy link

Collaborator

justinchuby commentedMay 1, 2025•
edited
Loading

If that's the case@titaiwangms maybe just remove the jit convert strategy for now? Ifstrict=True will get us exportedprograms strict=False potentially can't.