Refine MFSDP v2 configuration validation - #6333
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
a713f67 to
107da0b
Compare
| raise ValueError("MFSDP v2 does not currently support communication overlap modes.") | ||
| if config.gradient_accumulation_fusion: | ||
| raise ValueError("MFSDP v2 does not currently support gradient accumulation fusion.") | ||
| if config.calculate_per_token_loss: |
There was a problem hiding this comment.
MFSDP v2 unconditionally overlaps reduce-scatter and parameter all-gathers, and always uses double buffering. We intentionally accept and ignore the legacy overlap_grad_reduce, overlap_param_gather, and fsdp_double_buffer flags for compatibility: their defaults are False, so rejecting those values would require every existing v2 caller to override them just to preserve current behavior. The tradeoff is that setting them to False does not disable v2 overlap or double buffering.
107da0b to
c60c1de
Compare
| raise ValueError("MFSDP v2 does not support fsdp_double_buffer.") | ||
| if ddp_config.fsdp_db_use_persist_buf_on_alloc_fail: | ||
| raise ValueError("MFSDP v2 does not support fsdp_db_use_persist_buf_on_alloc_fail.") | ||
| if ddp_config.fsdp_all_gather_in_start_param_sync: |
There was a problem hiding this comment.
This flag doesn't make sense for MFSDP v2 because it doesn't rely on this flag to prefetch the first bucket.
|
/ok to test c60c1de |
c60c1de to
6b81672
Compare
|
/ok to test 6b81672 |
| placements = Placements( | ||
| dp_axes=[0], parameter=[Flat()], gradient=[Flat()], optimizer=[Flat()] | ||
| ) | ||
| use_symm_mem = ddp_config.nccl_ub |
There was a problem hiding this comment.
Consider comment why symmetric memory depends on UB use here.
Q: Any reason not using long name "use_symmetric_memory"? the other PR has gone length to rename it that way.
There was a problem hiding this comment.
Q: Any reason not using long name "use_symmetric_memory"? the other PR has gone length to rename it that way.
Yes. I'll do that after the other PR (#6127) is merged.
There was a problem hiding this comment.
Also added a code comment as you requested.
6b81672 to
1f28ed7
Compare
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com>
1f28ed7 to
331e6d2
Compare
4 participants
Summary
nccl_ub=Trueforwardsuse_symm_mem=Truetofully_shard.Validation
isort --check-only tests/unit_tests/distributed/mfsdp_v2/test_mcore_adapter.pypython -m torch.distributed.run --nproc-per-node 2 -m pytest -q tests/unit_tests/distributed/mfsdp_v2/test_mcore_adapter.pygit diff --check