-
Notifications
You must be signed in to change notification settings - Fork 25.7k
fix torch.jit.tracing for at::lift #77588
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
Conversation
🔗 Helpful links
❌ 1 New FailuresAs of commit 8d3f51e (more details on the Dr. CI page): Expand to see more
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
|
@bdhirsh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pytorchbot merge this (Initiating merge automatically since Phabricator Diff has merged) |
|
Hey @bdhirsh. |
Summary: After adding the `at::lift` op, it started getting traced during `torch.jit.trace`. We don't want that to happen for BC reasons Pull Request resolved: #77588 Reviewed By: ezyang Differential Revision: D36423018 Pulled By: bdhirsh fbshipit-source-id: 0099ce6fbce5a97c980a0c3374b1c239fc7806da
| } | ||
|
|
||
| // torch.jit.trace will continue to trace out `.to()` instead of `.lift()`, since | ||
| // changing it is BC-breaking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we should actually change this message to be FC breaking, because we can change in 2 weeks if we wanted to right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's right
After adding the
at::liftop, it started getting traced duringtorch.jit.trace. We don't want that to happen for BC reasons