-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[dynamo] Remove workaround for functools.wraps in functorch
#142014
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🧪 See artifacts and rendered test results at hud.pytorch.org/pr/142014
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit abf8368 with merge base c376b29 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torch/_functorch/eager_transforms.py
Outdated
| if not torch._dynamo.is_compiling(): | ||
| wrapper_fn = wraps(func)(wrapper_fn) | ||
|
|
||
| wrapper_fn = wraps(func)(wrapper_fn) |
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.
Can you put the wraps as a decorator on the wrapper_fn? i.e.
@wraps(func)
def wrapper_fn(...):
....
ditto below.
That's how the original code looked like before we did this hack
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.
Ah right, much cleaner, updated.
|
@pytorchbot merge |
Merge startedYour 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 |
Merge failedReason: HTTP Error 504: Gateway Timeout Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
Merge startedYour 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 |
…h#142014) This is no longer needed after pytorch#142000. Fixes pytorch#123365. Pull Request resolved: pytorch#142014 Approved by: https://github.com/zou3519 ghstack dependencies: pytorch#142000
|
@pytorchmergebot revert -c ghfirst -m "Sorry #142000 is failing internally, need to revert this" |
|
@pytorchbot successfully started a revert job. Check the current status here. |
|
@StrongerXi your PR has been successfully reverted. |
…#142014)" This reverts commit ed77901. Reverted #142014 on behalf of https://github.com/atalman due to Sorry #142000 is failing internally, need to revert this ([comment](#142014 (comment)))
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "unrelated failures" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "unrelated failures" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "unrelated failures" let's try one more time |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: This PR has internal changes and must be landed via Phabricator! Please try reimporting/rexporting the PR! Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "unrelated failures" third time's a charm |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
…h#142014) This is no longer needed after pytorch#142000. Fixes pytorch#123365. Pull Request resolved: pytorch#142014 Approved by: https://github.com/zou3519 ghstack dependencies: pytorch#142000
…pytorch#142014)" This reverts commit ed77901. Reverted pytorch#142014 on behalf of https://github.com/atalman due to Sorry pytorch#142000 is failing internally, need to revert this ([comment](pytorch#142014 (comment)))
This is no longer needed after #142000. Fixes #123365. D66838774 Pull Request resolved: #142014 Approved by: https://github.com/zou3519 ghstack dependencies: #142000
Stack from ghstack (oldest at bottom):
functools.wrapsin functorch #142014functools.wraps#142000This is no longer needed after #142000.
Fixes #123365.
D66838774