Dynamo asserts FSDP wrapped modules use_orig_param #89523

wconstab · 2022-11-22T21:53:14Z

Stack from ghstack (oldest at bottom):

This is a strict requirement given the way dynamo+FSDP is implemented,
but isn't convenient to assert.
By plumbing use_orig_param field on all wrapped modules, we can
do this assertion inside dynamo

cc @mlazos @soumith @voznesenskym @yanboliang @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @chunyuan-w @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo [ghstack-poisoned]

pytorch-bot · 2022-11-22T21:53:16Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

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

❌ 1 Failures

As of commit 098429e:

The following jobs have failed:

cuda11.6-py3.10-gcc7-sm86 / test (default, 2, 4, linux.g5.4xlarge.nvidia.gpu)

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

awgu · 2022-11-22T22:02:14Z

torch/distributed/fsdp/fully_sharded_data_parallel.py

+                # for this, since Dynamo skips all the FSDP code frames and thus can't inspect the
+                # FSDP module directly
+                submodule._fsdp_use_orig_params = use_orig_params
+


This FSDP change looks fine to me!

ok. the rest of the change is pretty trivial. I think the biggest risk is that we are cluttering up the submodules this way. the dynamo change is not a risk, and the unit test works.

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

wconstab · 2022-11-28T20:34:26Z

@pytorchbot merge

pytorchmergebot · 2022-11-28T20:36:15Z

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

pytorchmergebot · 2022-11-28T20:36:16Z

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team

Raised by workflow job

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

wconstab · 2022-11-28T20:41:51Z

@pytorchbot merge

pytorchmergebot · 2022-11-28T20:43:26Z

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

pytorchmergebot · 2022-11-28T20:53:33Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

Lint

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

wconstab · 2022-11-28T21:17:08Z

@pytorchbot merge

pytorchmergebot · 2022-11-28T21:19:00Z

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

pytorchmergebot · 2022-11-29T01:06:17Z

Merge failed

Reason: 1 additional jobs have failed, first few of them are: trunk

Details for Dev Infra team

Raised by workflow job

wconstab · 2022-11-29T01:23:37Z

@pytorchbot merge -f "Flaky CI"

pytorchmergebot · 2022-11-29T01:25:25Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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 · 2022-11-29T01:25:32Z

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x 21c8ca605c44114a34566279f6ebc058dd79cd31 returned non-zero exit code 1

Auto-merging torch/_dynamo/variables/builder.py
CONFLICT (content): Merge conflict in torch/_dynamo/variables/builder.py
Auto-merging torch/distributed/fsdp/fully_sharded_data_parallel.py
CONFLICT (content): Merge conflict in torch/distributed/fsdp/fully_sharded_data_parallel.py
error: could not apply 21c8ca605c... Dynamo asserts FSDP wrapped modules use_orig_param
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Details for Dev Infra team

Raised by workflow job

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

wconstab · 2022-11-29T05:25:38Z

@pytorchbot merge -f "Flaky CI"

pytorchmergebot · 2022-11-29T05:27:16Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` [ghstack-poisoned]

After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` ghstack-source-id: ace270d Pull Request resolved: #90100

…rig_params=True" After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` ghstack-source-id: 938e65b Pull Request resolved: #90100

After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` cc mlazos soumith voznesenskym yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

After #89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` Pull Request resolved: #90100 Approved by: https://github.com/wconstab

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo Pull Request resolved: pytorch#89523 Approved by: https://github.com/awgu

) After pytorch#89523, we now need to assert use_orig_params=True, even in the non-recursive case where (I think) we wouldn't otherwise need to run with use_orig_params=True. Tested with `python benchmarks/dynamo/torchbench.py --training --accuracy --only hf_T5 --fsdp` Pull Request resolved: pytorch#90100 Approved by: https://github.com/wconstab

Dynamo asserts FSDP wrapped modules use_orig_param

d4ade8f

- This is a strict requirement given the way dynamo+FSDP is implemented, but isn't convenient to assert. - By plumbing use_orig_param field on all wrapped modules, we can do this assertion inside dynamo [ghstack-poisoned]

wconstab requested review from H-Huang, awgu, kwen2501, mrshenli, pritamdamania87, rohan-varma and zhaojuanmao as code owners November 22, 2022 21:53

wconstab mentioned this pull request Nov 22, 2022

Fix inductor fallback_random for dropout/rand_like #89515

Closed

wconstab mentioned this pull request Nov 22, 2022

Add limited FSDP correctness to torchdynamo benchmark #89469

Closed

pytorch-bot bot added the release notes: distributed (sharded) release notes category label Nov 22, 2022

This was referenced Nov 22, 2022

Special-case fsdp wrapped modules to be Unspecialized #89330

Closed

Fix AutogradMonkeyPatch typo, enable replacements #89520

Closed

Add gpu memory profiler script for FSDP bench #89457

Closed

github-actions bot added ciflow/inductor module: dynamo labels Nov 22, 2022

wconstab requested a review from davidberard98 November 22, 2022 21:59

awgu reviewed Nov 22, 2022

View reviewed changes

awgu approved these changes Nov 22, 2022

View reviewed changes

wconstab added 2 commits November 22, 2022 22:57

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 28, 2022

wconstab mentioned this pull request Nov 28, 2022

Add debugability comments to DDPOptimizer #89802

Closed

pytorchmergebot added the Merged label Nov 29, 2022

pytorchmergebot closed this in 705ad36 Nov 29, 2022

davidberard98 mentioned this pull request Dec 3, 2022

[Dynamo+FSDP] Update benchmarks with use_orig_params=True #90100

Closed

facebook-github-bot deleted the gh/wconstab/49/head branch June 8, 2023 19:17

Dynamo asserts FSDP wrapped modules use_orig_param #89523

Dynamo asserts FSDP wrapped modules use_orig_param #89523

Uh oh!

Conversation

wconstab commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 Failures

Uh oh!

awgu Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

wconstab Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

wconstab commented Nov 28, 2022

Uh oh!

pytorchmergebot commented Nov 28, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 28, 2022

Merge failed

Uh oh!

wconstab commented Nov 28, 2022

Uh oh!

pytorchmergebot commented Nov 28, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 28, 2022

Merge failed

Uh oh!

wconstab commented Nov 28, 2022

Uh oh!

pytorchmergebot commented Nov 28, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 29, 2022

Merge failed

Uh oh!

wconstab commented Nov 29, 2022

Uh oh!

pytorchmergebot commented Nov 29, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 29, 2022

Merge failed

Uh oh!

wconstab commented Nov 29, 2022

Uh oh!

pytorchmergebot commented Nov 29, 2022

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wconstab commented Nov 22, 2022 •

edited

Loading

pytorch-bot bot commented Nov 22, 2022 •

edited

Loading