Skip to content

[invoke_subgraph] Run joint passes on the hop graphs#139325

Closed
anijain2305 wants to merge 43 commits intogh/anijain2305/571/basefrom
gh/anijain2305/571/head
Closed

[invoke_subgraph] Run joint passes on the hop graphs#139325
anijain2305 wants to merge 43 commits intogh/anijain2305/571/basefrom
gh/anijain2305/571/head

Conversation

@anijain2305
Copy link
Contributor

@anijain2305 anijain2305 commented Oct 30, 2024

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 30, 2024

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 7dc0e1a with merge base 30375cb (image):

NEW FAILURE - The following job has failed:

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

def propagate_meta_info(new_hop_gm, new_call_function_node, old_call_function_node):
# Copy all the fields from the old call_function node. And then override
# the `val` meta field with the outputs of new_hop_gm.
new_call_function_node.meta = copy.copy(old_call_function_node.meta)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we really need to copy.copy() all of the metadata here. Presumably we aren't mutating the FakeTensors that we stashed onto the graph?

Copy link
Collaborator

Choose a reason for hiding this comment

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

as a datapoint, when we propagate metadata natively in FX when retracing, we just use the exact same values in the meta dict directly: https://github.com/pytorch/pytorch/blob/main/torch/fx/node.py#L718

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, if we can avoid the copy that would be better

anijain2305 added a commit that referenced this pull request Feb 28, 2025
ghstack-source-id: 5624ea0
Pull Request resolved: #139325
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
anijain2305 added a commit that referenced this pull request Feb 28, 2025
ghstack-source-id: 8dd63e6
Pull Request resolved: #139325
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames

[ghstack-poisoned]
anijain2305 added a commit that referenced this pull request Feb 28, 2025
ghstack-source-id: c6d3ac6
Pull Request resolved: #139325
Copy link
Collaborator

@bdhirsh bdhirsh left a comment

Choose a reason for hiding this comment

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

sgtm

@anijain2305
Copy link
Contributor Author

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 1 checks: Lint / lintrunner-noclang / linux-job

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 pushed a commit to min-jean-cho/pytorch that referenced this pull request Mar 5, 2025
@github-actions github-actions bot deleted the gh/anijain2305/571/head branch April 4, 2025 02:13
Divigroup-RAP pushed a commit to Divigroup-RAP/PYTORCH that referenced this pull request Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants