Fix gangs coordinator rank check - #1521
Conversation
|
Hi @joaopedroassad! 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! |
What does this PR do? Please describe:
Gangs.__post_init__enforces that whenroot.rank == 0, the coordinator process must be rank 0 in every parallel gang. The check read:Since Python binds
andtighter thanor, this evaluates asdp.rank != 0 or (tp.rank != 0 and pp.rank != 0). As a result, aroot.rank == 0process that is non-zero in exactly one of the tensor- or pipeline-parallel gangs (for exampletp.rank == 1, pp.rank == 0, ortp.rank == 0, pp.rank == 1) passes validation, contradicting the error message ("must be rank 0 in all parallel gangs"). This PR joins all three checks withor, adds parametrized regression tests, and adds a CHANGELOG entry.No associated issue.
Does your PR introduce any breaking changes? If yes, please list them:
No. It only makes the validation reject coordinator configurations it was always meant to reject; valid configurations (rank 0 in every parallel gang) behave exactly as before.
Check list:
tests/unit/test_gang.pycovering the previously-accepted(tp=1, pp=0)and(tp=0, pp=1)coordinator configurations.isort/black/flake8clean. The fullpytestsuite could not run locally because the nativefairseq2nmodule has no macOS build; the new tests exercise the realGangsclass and run in CI.