Decide checkpoint phase-awareness from the checkpoint name only - #1076
Open
arpitjain099 wants to merge 1 commit into
Open
Decide checkpoint phase-awareness from the checkpoint name only#1076arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
_populate_from_str ran the train_step/eval_step/predict_step substring test over the whole path, including the caller's dirpath. A phase-naive checkpoint under a directory whose name contains one of those substrings was routed onto the phase-aware regex, which either drops the step into the metric-name group or fails to match at all. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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.
Summary:
CheckpointPath._populate_from_strpicks between the phase-aware and phase-naive regex by testing fortrain_step/eval_step/predict_stepanywhere in the path string, which includes the user-supplied dirpath. A phase-naive checkpoint that happens to live under a directory containing one of those substrings gets routed onto the phase-aware regex, and there are two ways that goes wrong:foo/eval_step_sweeps/epoch_2_step_1_acc=0.98parses, but(?:_(\w+)=(...))?swallows_step_1_acc=0.98as the metric, so the result hasstep={}and a metric namedstep_1_acc.foo/train_step_dir/epoch_2_step_1(no metric) matches neither branch of the phase-aware regex and raisesValueError: Attempted to parse malformed checkpoint path.Both are reachable from ordinary use.
CheckpointPath.pathproduces exactly these strings, so the serialize then parse round-trip breaks;__setstate__calls_populate_from_str, so unpickling a CheckpointPath raises, which matters because this class is broadcast across ranks; andget_latest_checkpoint_path/get_checkpoint_dirpaths/CheckpointManagerdiscovery break for anyone whose output directory is named something likeeval_step_sweeps.The dirpath has no bearing on whether the checkpoint itself is phase-aware, so the fix looks at the final path component only.
Test plan:
Added two entries to the existing
valid_pathstable inCheckpointPathTest::test_from_str, one per failure mode above.test_from_strfails before the change and the file passes after:pytest tests/utils/test_checkpoint.pygives 28 passed on torch 2.13 CPU, Python 3.12.