fix(diloco test): set enable_checkpointing explicitly when loading state - #5132
Open
gulsumgudukbay wants to merge 1 commit into
Open
fix(diloco test): set enable_checkpointing explicitly when loading state#5132gulsumgudukbay wants to merge 1 commit into
gulsumgudukbay wants to merge 1 commit into
Conversation
`load_full_state_path` requires `enable_checkpointing=true`, and the third config in this test relied on the base file to supply it. `base.yml` defaults it to true but `decoupled_base_test.yml` sets it to false, so the test fails config validation whenever `get_test_config_path()` resolves to the decoupled base. The first config in the same test already passes the flag explicitly; do the same here rather than depend on the base default.
There was a problem hiding this comment.
Code Review
This pull request updates the integration test test_diloco_checkpoint_saving_and_normal_resume in tests/integration/diloco_test.py by adding the enable_checkpointing=true configuration parameter. There are no review comments, and I have no feedback to provide.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
gulsumgudukbay
marked this pull request as ready for review
September 4, 2026 04:43
gulsumgudukbay
requested review from
A9isha,
NuojCheng,
RissyRan,
SurbhiJainUSC,
abhinavclemson,
aireenmei,
bvandermoon,
darisoy,
dipannita08,
gagika,
gobbleturk,
hengtaoguo,
huytransformer,
igorts-git,
jiangjy1982,
khatwanimohit,
richjames0,
shralex,
shuningjin,
vipannalla and
xibinliu
as code owners
September 4, 2026 04:43
shralex
approved these changes
Sep 4, 2026
khatwanimohit
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
test_diloco_checkpoint_saving_and_normal_resumebuilds a third config to verify normal (non-DiLoCo) full-state restoration. It setsload_full_state_pathbut relies on the base config file forenable_checkpointing, and config validationrejects that combination: "You must set enable_checkpointing=True to load a checkpoint."
base.ymldefaults the flag to true, so the test passes today. Butget_test_config_path()resolves todecoupled_base_test.ymlwhenDECOUPLE_GCLOUD=TRUE, and that file setsenable_checkpointing: false, so thetest fails validation there. The first config in the same test already passes the flag explicitly; this makes the third one do the same instead of depending on which base file happens to be selected.
Tests
Reproduce the failure on
mainand the fix on this branch with:Verified on 4x AMD Instinct MI355X (ROCm 10, jax 0.11.1) with
DECOUPLE_GCLOUD=TRUE: the test fails onmainwith the validation error above and passes on this branch.
Checklist
gemini-reviewlabel.