Skip to content
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

[pipelining] Follow improvements in export.unflatten #126217

Closed
wants to merge 1 commit into from

Conversation

kwen2501
Copy link
Contributor

@kwen2501 kwen2501 commented May 14, 2024

Previously, we make a copy of torch.export.unflatten in pippy/_unflatten.py.

But it turns out to be too hard to track bug fixes and improvements in upstream version. For example, torch.export.unflatten recently added support for tied parameters, which is something pipelining needs.

Now that we moved into pytorch, we make a reference to torch.export.unflatten instead of maintaining a copy.

Stack from ghstack (oldest at bottom):

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang @d4l3k

@pytorch-bot pytorch-bot bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label May 14, 2024
Copy link

pytorch-bot bot commented May 14, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit c75e69b with merge base 3759676 (image):
💚 Looks good so far! There are no failures yet. 💚

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

kwen2501 added a commit that referenced this pull request May 14, 2024
ghstack-source-id: ae40df3b4575acbf2f35039db6891ed1478d0619
Pull Request resolved: #126217
@kwen2501 kwen2501 requested review from wconstab and H-Huang May 14, 2024 22:24
@kwen2501 kwen2501 added the topic: not user facing topic category label May 14, 2024
Copy link
Member

@H-Huang H-Huang left a comment

Choose a reason for hiding this comment

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

looks good!

assert node_module_stack == self.module_stack
self.copy_node(node)
node_idx += 1
from torch.export.unflatten import _ModuleFrame


def _outline_submodules(orig_graph: torch.fx.Graph):
Copy link
Member

Choose a reason for hiding this comment

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

Any value of this just getting moved to torch.export.unflatten as well? Or is this pretty PP specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is the one func that's special.

@kwen2501
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 17, 2024
@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

ZelboK pushed a commit to ZelboK/pytorch that referenced this pull request May 19, 2024
Previously, we make a copy of `torch.export.unflatten` in pippy/_unflatten.py.

But it turns out to be too hard to track bug fixes and improvements in upstream version. For example, `torch.export.unflatten` recently added support for tied parameters, which is something pipelining needs.

Now that we moved into pytorch, we make a reference to `torch.export.unflatten` instead of maintaining a copy.

Pull Request resolved: pytorch#126217
Approved by: https://github.com/H-Huang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: distributed Add this issue/PR to distributed oncall triage queue topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants