Fix checkpoint flush budget calculation

[wolfgang-desalvador]

This pull request updates the checkpoint cache flush validation logic to more accurately account for final, post-benchmark data collections that occur after the main timed section. #782 The goal is to ensure that the measured gap between write and read phases does not unfairly penalize teams for legitimate post-benchmark activities. The main changes are the introduction of helper functions to locate and normalize collection timestamps, and the adjustment of the gap calculation to use the earliest qualifying post-benchmark collection timestamp when available.

Checkpoint gap calculation improvements:

• Added a new helper function _oldest_final_collection_timestamp to find the earliest post-benchmark collection_timestamp in write-phase metadata, ensuring the failover-callout gap is measured fairly.
• Modified cache_flush_validation to use the post-benchmark collection timestamp as the gap origin when available, and updated log messages to clarify which timestamp is used. [1] [2] [3] [4]
• Imported _oldest_final_collection_timestamp into checkpointing_checks.py for use in the validation logic.

Helper utilities:

• Introduced _normalize_collection_iso to standardize timestamp formats for comparison, and _iter_collection_timestamps to recursively find all collection_timestamp values in nested metadata structures. #

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[Copilot]

Pull request overview

This PR updates the §4.7.1 checkpoint cache flush (failover-callout) gap calculation so that, when available, the write-side gap origin uses the earliest post-benchmark collection_timestamp from write metadata rather than the write benchmark’s timed-section summary.end_time. This aligns the validation with real-world split-mode teardown behavior and avoids charging final multi-node collection time against the 30-second budget.

Changes:

• Added helper utilities to normalize and traverse nested metadata to find the oldest qualifying post-benchmark collection_timestamp.
• Updated cache_flush_validation to prefer that post-benchmark timestamp as the write-side gap origin (with clearer origin labeling in error output and added INFO reporting).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
mlpstorage_py/submission_checker/checks/helpers.py	Adds helpers to normalize ISO strings, iterate nested metadata for collection_timestamp, and select the earliest post-benchmark timestamp.
mlpstorage_py/submission_checker/checks/checkpointing_checks.py	Uses the new helper to adjust the write-side origin for the failover-callout gap and logs the computed gap with origin context.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfgang-desalvador]

I think we should keep it invocation_start since it is what we are aiming for. Does it make sense @russfellows @FileSystemGuy

[wolfgang-desalvador]

@FileSystemGuy I will refactor this to merge the recent changes of invocation_end_time to main. But can we keep collection timestamp as fallback. This would allow to keep expensive and long lasting runs valid?

I am fixing the PR

[wolfgang-desalvador]

@russfellows added unittest
@FileSystemGuy this allows to preserve with a reasonable assumption previous runs in case invocation_end_time is missing

[FileSystemGuy]

@wolfgang-desalvador @russfellows

I asked Claude to summarize the behavioral changes your PR makes, are the only two I have questions on:

2. NEW: earliest collection_timestamp in write_metadata that is >= write.summary.end_time.
Pre-benchmark collections (older than summary end) are explicitly skipped so the origin never moves
earlier than the summary end.

I'm confused on why we would want "earliest" anything in this context. What we want to know is the gap between the end of the write phase and the start of the read phase. The bugs from before the last few PRs were that the last timestamp of the write phase included the teardown costs in the gap (this PR) and the read phase included the setup costs in the gap (my PR from a few days ago, which should have included this PR but I didn't think far enough ahead). Those two PR's tighten the "gap" to not include the teardown or setup times, by creating a new timestamp that is the "latest" in the write phase and a new timestamp that is the "earliest" in the read phase. I don't know why "earliest" is part of the write phase. Help?

- The if read_start is None: early-exit block is deleted (checkpointing_checks.py lines 660–669 in
the pre-PR file). Previously, a read pair with no invocation_start_time and no summary.start_time
produced a specific violation: "cannot compute failover-callout gap: read-phase metadata has no
invocation_start_time and read-phase summary.json has no start_time/start". After the PR, that state
 falls through into _parse_iso_gap(write_end, None), which raises TypeError, gets caught by the
existing except (ValueError, TypeError), and produces the generic "cannot parse timestamps for gap
computation: ..." violation instead.

This is a small point, but it would be better to have the more-specific message than depending upon a raise path for a message.

[FileSystemGuy]

@wolfgang-desalvador WRT preserving the validity of existing submission runs, I agree completely that we need to do that. I thought that my PR did that by falling back to the current/prior best timestamp if the new one wasn't present. If that doesn't work or doesn't preserve prior runs, please let me know.

Alternatively asked, what case are you concerned about, what would cause the prior runs to fail the new validation checks?

[wolfgang-desalvador]

@wolfgang-desalvador @russfellows

I asked Claude to summarize the behavioral changes your PR makes, are the only two I have questions on:

2. NEW: earliest collection_timestamp in write_metadata that is >= write.summary.end_time.
Pre-benchmark collections (older than summary end) are explicitly skipped so the origin never moves
earlier than the summary end.

I'm confused on why we would want "earliest" anything in this context. What we want to know is the gap between the end of the write phase and the start of the read phase. The bugs from before the last few PRs were that the last timestamp of the write phase included the teardown costs in the gap (this PR) and the read phase included the setup costs in the gap (my PR from a few days ago, which should have included this PR but I didn't think far enough ahead). Those two PR's tighten the "gap" to not include the teardown or setup times, by creating a new timestamp that is the "latest" in the write phase and a new timestamp that is the "earliest" in the read phase. I don't know why "earliest" is part of the write phase. Help?

- The if read_start is None: early-exit block is deleted (checkpointing_checks.py lines 660–669 in
the pre-PR file). Previously, a read pair with no invocation_start_time and no summary.start_time
produced a specific violation: "cannot compute failover-callout gap: read-phase metadata has no
invocation_start_time and read-phase summary.json has no start_time/start". After the PR, that state
 falls through into _parse_iso_gap(write_end, None), which raises TypeError, gets caught by the
existing except (ValueError, TypeError), and produces the generic "cannot parse timestamps for gap
computation: ..." violation instead.

This is a small point, but it would be better to have the more-specific message than depending upon a raise path for a message.

This comment @FileSystemGuy refers to the fact that we should exclude from the collection_timestamp the initial collection round that is done at the beginning of the benchmark, at the very beginning before even DLIO is called. We should use as collection_timestamps only the one after the summary end.

I can make this clear in case

[wolfgang-desalvador]

@wolfgang-desalvador WRT preserving the validity of existing submission runs, I agree completely that we need to do that. I thought that my PR did that by falling back to the current/prior best timestamp if the new one wasn't present. If that doesn't work or doesn't preserve prior runs, please let me know.

Alternatively asked, what case are you concerned about, what would cause the prior runs to fail the new validation checks?

Actually, the change you did in main @FileSystemGuy is perfect. It is not invalidating runs already existing.

But also, is not fixing the scenario of submitters that run perfectly valid runs (even with only few seconds between read and write) since the invocation_end_time is missing in previous runs metadata. So these won't validate.

This would force submitters to re-run perfectly valid runs just for missing this metadata. The fix implemented here would allow already executed runs to be evaluated in a more "fair" way.

[FileSystemGuy]

But also, is not fixing the scenario of submitters that run perfectly valid runs (even with only few seconds between read and write) since the invocation_end_time is missing in previous runs metadata. So these won't validate. This would force submitters to re-run perfectly valid runs just for missing this metadata. The fix implemented here would allow already executed runs to be evaluated in a more "fair" way.

Yes, I agree that we want that, "already executed runs must remain valid". I'm just not sure how the code does that. What am I missing? Are you saying that since write.summary.end_time is not ideal (it includes the teardown time in the "gap"), we want to pick any timestamp from the write phase that we can find that is AFTER that point? I would agree with you, but then the code would be looking for the:

earliest LATEST collection_ timestamp in write_metadata that is >= write.summary.end_time. Pre-benchmark collections (older than summary end) are explicitly skipped so the origin never moves earlier than the summary end

Have I just over-focused on the term "earliest"?

[wolfgang-desalvador]

But also, is not fixing the scenario of submitters that run perfectly valid runs (even with only few seconds between read and write) since the invocation_end_time is missing in previous runs metadata. So these won't validate. This would force submitters to re-run perfectly valid runs just for missing this metadata. The fix implemented here would allow already executed runs to be evaluated in a more "fair" way.

Yes, I agree that we want that, "already executed runs must remain valid". I'm just not sure how the code does that. What am I missing? Are you saying that since write.summary.end_time is not ideal (it includes the teardown time in the "gap"), we want to pick any timestamp from the write phase that we can find that is AFTER that point? I would agree with you, but then the code would be looking for the:

earliest LATEST collection_ timestamp in write_metadata that is >= write.summary.end_time. Pre-benchmark collections (older than summary end) are explicitly skipped so the origin never moves earlier than the summary end

Have I just over-focused on the term "earliest"?

You are super right. Fixing this