Skip to content

Preseve stack trace in nodes during fx.Transform #82670

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

Closed

Conversation

SherlockNoMad
Copy link
Contributor

@SherlockNoMad SherlockNoMad commented Aug 2, 2022

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 2, 2022

🔗 Helpful links

✅ No Failures (4 Pending)

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

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚


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.

@@ -148,6 +149,8 @@ def run_node(self, n : Node) -> Any:
Returns:
Any: The result of executing ``n``
"""
self.current_node = n
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this to be dynamically scoped: so you save the current value of current_node, set it, and then on return (finally) you restore to old value

Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly helpful if you have some reentrant execution of the same interpreter. Which hopefully is pretty unlikely...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per request.

Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

okey dokey; manually written passes are going to have to be adjusted too though :(

@Chillee it's easier to hide the boilerplate if we do retracing lol. I swear people are going to keep forgetting to propagate stack traces. Maybe we need to make some modifications to the API people do node iteration over to make it easier to not forget to do this.

@SherlockNoMad
Copy link
Contributor Author

@pytorchbot rebase -s

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/SherlockNoMad/5/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/82670)

pytorchmergebot pushed a commit that referenced this pull request Aug 3, 2022
ghstack-source-id: 13298fa
Pull Request resolved: #82670
@SherlockNoMad
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge and created land time checks. See merge status here and land check progress here.

pytorchmergebot pushed a commit that referenced this pull request Aug 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

Hey @SherlockNoMad.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@SherlockNoMad SherlockNoMad deleted the gh/SherlockNoMad/5/head branch August 3, 2022 20:35
facebook-github-bot pushed a commit that referenced this pull request Aug 4, 2022
Summary:
Pull Request resolved: #82670
Approved by: https://github.com/ezyang

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

Reviewed By: kit1980

Differential Revision: D38424995

Pulled By: SherlockNoMad

fbshipit-source-id: 80e509caa938a254b043288b570fdf088d07d2c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants