Skip to content

Check for checkpoints#84

Merged
PatrickRMiles merged 2 commits into
LBANN:mainfrom
michaelmckinsey1:better-checkpointing
Jul 23, 2026
Merged

Check for checkpoints#84
PatrickRMiles merged 2 commits into
LBANN:mainfrom
michaelmckinsey1:better-checkpointing

Conversation

@michaelmckinsey1

Copy link
Copy Markdown
Collaborator

I have been using checkpoints via

--restart \
  --run-dir /usr/workspace/mckinsey/ScaFFold-RFP/scaling/tuolumne/torchrun_hpc-scaffold_2026-06-23_06h34m11s_380741e8/benchmark_runs/benchmark_20260623-063806

and I noticed if an incorrect path is specified, e.g. if scaffold can't find a checkpoint, it will start from scratch. This led to me accidentally restarting the same jobs. I think we should check for the checkpoint file first and exit if the user made a mistake, instead of starting from the beginning and wasting compute cycles.

@michaelmckinsey1 michaelmckinsey1 self-assigned this Jul 9, 2026
@michaelmckinsey1
michaelmckinsey1 marked this pull request as ready for review July 9, 2026 19:58
@michaelmckinsey1 michaelmckinsey1 changed the title [WIP] Check checkpoints Check checkpoints Jul 9, 2026
@michaelmckinsey1 michaelmckinsey1 changed the title Check checkpoints Check for checkpoints Jul 9, 2026
Comment thread ScaFFold/utils/checkpointing.py Outdated
self.optimizer.zero_grad(set_to_none=True)

def load_from_checkpoint(self) -> int:
def load_from_checkpoint(self, require_checkpoint: bool = False) -> int:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only call load_from_checkpoint on restart, will there ever be a case where we would want require_checkpoint to be False? If not, maybe we can remove this arg altogether and just require finding a valid existing checkpoint in the following logic.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think you are right. I have removed the extra arg and we just assume the checkpoint is required now

@PatrickRMiles
PatrickRMiles merged commit 3618daf into LBANN:main Jul 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants