Update tensor_parallel_example.py#1324
Conversation
|
Hi @nrothGIT! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
✅ Deploy Preview for pytorch-examples-preview canceled.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug by moving the optimizer definition so that it is instantiated after the model has been parallelized.
- Reorders the optimizer creation to occur after module parallelization
- Ensures the optimizer tracks the correct set of parameters for the parallelized model
Comments suppressed due to low confidence (1)
distributed/tensor_parallelism/tensor_parallel_example.py:106
- Moving the optimizer instantiation below the module parallelization is correct; please verify that the parallelized model's parameters are correctly registered for optimization in the latest PyTorch version.
lr = 0.25
lessw2020
left a comment
lessw2020
left a comment
There was a problem hiding this comment.
good catch!
Yes, the optimizer should be created post parallelization.
Ran the pre and post code to verify
a) no update as claimed with the current example and
b) proper post-parallelization optimizer application results in updated weights.
We actually expressly call out this ordering (parallelize first, then optimizer) in titan ironically:
https://github.com/pytorch/torchtitan/blob/a4ed09c329e3f2d4c3e9f5fcbc2ce50e076f9782/torchtitan/train.py#L248
lgtm and thanks for the fix!
|
thanks @nrothGIT for catching this and for the PR! |
I believe this is a small bug. If you run the current code in latest pytorch, the model will not update. I think the optimizer should be defined after the weights are parallelized as in the sequence parallelism example?