Fix Megatron-FSDP checkpoint save failure - #2138
Conversation
…with_distributed_if_needed is called
|
|
||
| args.use_distributed_optimizer = ckpt_format != "torch_dcp" | ||
| args.use_dist_ckpt = ckpt_format != "torch" | ||
| args.use_megatron_fsdp = ckpt_format == "fsdp_dtensor" |
| expected_ckpt_path = ckpt_dir / "mp_rank_00" / "model_optim_rng.pt" | ||
| elif ckpt_format == "torch_dcp": | ||
| expected_ckpt_path = ckpt_dir / ".metadata" | ||
| elif ckpt_format == "fsdp_dtensor": |
There was a problem hiding this comment.
Minor issue, this if condition is running the same code as elif ckpt_format == "torch_dcp":, maybe we can combine them
There was a problem hiding this comment.
@BoxiangW If you want to apply this structurally across the codebase, ruff can be used to automatically lint and fix these duplicated if else codepaths.
There was a problem hiding this comment.
cc @ko3n1g, is there any plan on switching to ruff instead? I know that all other repos are using it already
There was a problem hiding this comment.
We have some pretty carefully chosen, mostly non-opinionated ruff rules chosen in PyTorch: https://github.com/pytorch/pytorch/blob/a74fe75c450277eb88a95c764e8b0a664a550a86/pyproject.toml#L130
This specific one we did not enable because of breakpoint debugging / traceback reasons, but on a smaller codebase like this more easy to enforce opinionated lints.
There was a problem hiding this comment.
Do you want to create a separate issue for this? It seems would be very helpful for improving Megatron-Core's formatting
There was a problem hiding this comment.
More for catching bugs lol, I've found several bugs with ruff that should have not made it past the CI into Megatron.
Do you mean issue for this specific rule or enabling ruff more broadly.
There was a problem hiding this comment.
Got it, current lint is more for formatting and import checks.
I meant enabling ruff more broadly actually, I think it would be great to have.
There was a problem hiding this comment.
hey @Skylion007, we had to put the plans to switch to ruff on ice for a bit due to the team impact when handling a code-base of this size. We hope that we can pick those plan up again end of the year. Thanks for the tooling you shared!
There was a problem hiding this comment.
I enabled most of the rules for most of PyTorch, which is a way larger codebase if you want to discuss how to enable it incrementally @ko3n1g
|
Likely closes: #2035 |
|
/ok to test 98158d9 |
Fix Megatron-FSDP checkpoint save failure by ensuring _replace_param_with_distributed_if_needed is invoked.
Prior to this change, the following error messages would appear in the logs:
What does this PR do ?
Contribution process
flowchart LR A[Pre-checks] --> B[PR Tests] subgraph Code Review/Approval C1[Expert Review] --> C2[Final Review] end B --> C1 C2 --> D[Merge]Pre-checks
Core 0.8)Code review
The following process is enforced via the CODEOWNERS file for changes into
megatron/core. For changes outside ofmegatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.For MRs into `main` branch
(Step 1): Add PR label
Expert Review(Step 2): Collect the expert reviewers reviews
Expert Reviewlabel when your PR is ready for review.Final Review might get declined if these requirements are not fulfilled.
(Step 3): Final Review
Final Reviewlabel(Optional Step 4): Cherry-pick into release branch
If this PR also needs to be merged into
core_r*release branches, after this PR has been merged, selectCherry-pickto open a new PR into the release branch.For MRs into `dev` branch
The proposed review process for `dev` branch is under active discussion.MRs are mergable after one approval by either
eharper@nvidia.comorzijiey@nvidia.com.Merging your PR
Any member of core-adlr and
core-nemowill be able to merge your PR.