add async save support for fsdp checkpoints - #3339
Conversation
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test dba0517 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test a67f015 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 2be7c04 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 2e15c5c |
📝 WalkthroughWalkthroughAdded runtime detection for the NVIDIA resiliency extension and introduced async checkpoint support for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/megatron/bridge/training/checkpointing.py (1)
950-965:⚠️ Potential issue | 🔴 CriticalFix mixed async+sync execution in the FSDP DTensor save path.
When async is enabled, Line 961-965 still performs a synchronous
torch.distributed.checkpoint.save(...)unconditionally. This causes duplicate writes and defeats async behavior. Also, whenckpt_cfg.async_save=TruebutHAVE_NVRX=False,async_save_requeststaysNone, and later async assertions fail instead of falling back.💡 Suggested fix (use an effective async flag + true fallback)
@@ - async_save_request = None + async_save_request = None + effective_async_save = ckpt_cfg.async_save @@ - if ckpt_cfg.async_save and HAVE_NVRX: + if effective_async_save and HAVE_NVRX: planner = torch.distributed.checkpoint.DefaultSavePlanner() coordinator_rank = 0 fs_storage_writer = FileSystemWriterAsync( checkpoint_name, thread_count=ckpt_cfg.dist_ckpt_workers, use_msc=ckpt_cfg.enable_msc ) @@ save_state_dict_ret = save_state_dict_async_plan( state_dict, fs_storage_writer, None, coordinator_rank, planner=planner, enable_cache=ckpt_cfg.ckpt_assume_constant_structure ) async_save_request = get_save_and_finalize_callbacks(fs_storage_writer, save_state_dict_ret) - fs_storage_writer = torch.distributed.checkpoint.FileSystemWriter(checkpoint_name) - torch.distributed.checkpoint.save( - state_dict=state_dict, - storage_writer=fs_storage_writer, - ) + else: + if effective_async_save and not HAVE_NVRX: + print_rank_0( + "WARNING: async_save=True for fsdp_dtensor but nvidia_resiliency_ext async_ckpt is unavailable; " + "falling back to synchronous save." + ) + effective_async_save = False + fs_storage_writer = torch.distributed.checkpoint.FileSystemWriter(checkpoint_name) + torch.distributed.checkpoint.save( + state_dict=state_dict, + storage_writer=fs_storage_writer, + ) @@ - if ckpt_type != CheckpointType.LOCAL: - if not ckpt_cfg.async_save: + if ckpt_type != CheckpointType.LOCAL: + if not effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if cfg.logger.log_progress and ckpt_cfg.async_save: + if cfg.logger.log_progress and effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: @@ - if ckpt_cfg.async_save: + if effective_async_save: schedule_async_save(state, async_save_request) @@ - save_dir, leave_ckpt_num=ckpt_cfg.most_recent_k, do_async=ckpt_cfg.async_save + save_dir, leave_ckpt_num=ckpt_cfg.most_recent_k, do_async=effective_async_save )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/megatron/bridge/training/checkpointing.py` around lines 950 - 965, The code unconditionally calls torch.distributed.checkpoint.save causing duplicate sync writes and doesn't fallback when async is requested but HAVE_NVRX is false; fix by computing an effective_async flag = ckpt_cfg.async_save and HAVE_NVRX, only create FileSystemWriterAsync / call save_state_dict_async_plan / get_save_and_finalize_callbacks when effective_async is true, otherwise create the synchronous torch.distributed.checkpoint.FileSystemWriter and call torch.distributed.checkpoint.save; ensure async_save_request is set/left None predictably so later assertions can detect fallback. Reference symbols: ckpt_cfg.async_save, HAVE_NVRX, FileSystemWriterAsync, save_state_dict_async_plan, get_save_and_finalize_callbacks, torch.distributed.checkpoint.FileSystemWriter, torch.distributed.checkpoint.save, async_save_request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/megatron/bridge/training/checkpointing.py`:
- Around line 898-901: The config validation currently only allows async_save
for 'torch_dist', which blocks the new fsdp_dtensor async path; update the
validation that checks async_save (the assertion/conditional referencing
'torch_dist') to also permit 'fsdp_dtensor' (i.e., include 'fsdp_dtensor'
alongside 'torch_dist' in the allowed ckpt_format check for async_save) and
adjust any related error/assert messages to mention both formats (refer to
async_save, ckpt_format, and the 'fsdp_dtensor' and 'torch_dist' format
strings).
---
Outside diff comments:
In `@src/megatron/bridge/training/checkpointing.py`:
- Around line 950-965: The code unconditionally calls
torch.distributed.checkpoint.save causing duplicate sync writes and doesn't
fallback when async is requested but HAVE_NVRX is false; fix by computing an
effective_async flag = ckpt_cfg.async_save and HAVE_NVRX, only create
FileSystemWriterAsync / call save_state_dict_async_plan /
get_save_and_finalize_callbacks when effective_async is true, otherwise create
the synchronous torch.distributed.checkpoint.FileSystemWriter and call
torch.distributed.checkpoint.save; ensure async_save_request is set/left None
predictably so later assertions can detect fallback. Reference symbols:
ckpt_cfg.async_save, HAVE_NVRX, FileSystemWriterAsync,
save_state_dict_async_plan, get_save_and_finalize_callbacks,
torch.distributed.checkpoint.FileSystemWriter,
torch.distributed.checkpoint.save, async_save_request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dc37e54-6e6a-44ec-a288-38ffca6e5ddb
📒 Files selected for processing (1)
src/megatron/bridge/training/checkpointing.py
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 04be225 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 94ce030 |
|
@dimapihtar needs tests |
|
New checkpoint configs should be added to the checkpoint config: So far looking good, just saved a checkpoint async. Doing stop-and-go tests... In addition to this, |
|
/ok to test 9f90f6c |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test ae4eacf |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 558f799 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test 0fccf12 |
|
/ok to test 59e3fe4 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com>
|
/ok to test c60768d |
|
/ok to test dcfbfea |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com> Signed-off-by: Dmytro Pykhtar <37850217+dimapihtar@users.noreply.github.com> Signed-off-by: Vasudevan Rengasamy <vrengasamy@nvidia.com>
What does this PR do ?
Adds async save support for fsdp checkpoints.
Changelog
GitHub Actions CI
See the CI sectionin the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.
Before your PR is "Ready for review"
Pre checks:
If you haven't finished some of the above items you can still open "Draft" PR.
Additional Information
Summary by CodeRabbit