Fix calculation of checkpoint gap in reports

[wolfgang-desalvador]

This pull request updates the handling of benchmark invocation timing metadata to improve the accuracy of cache-flush gap measurement and ensure compliance with the Rules.md specification. The main changes introduce new fields for invocation bookends, propagate them throughout the data model, and update logic in the checkpointing submission checker to prefer these authoritative timestamps.

Enhancements to timing metadata handling:

• Added invocation_start_time and invocation_end_time fields to the BenchmarkRunData dataclass, including documentation explaining their significance for cache-flush gap measurement and their use as authoritative origins per Rules.md §4.7.1.
• Updated the parse and _from_metadata methods in models.py to extract and populate the new invocation bookend fields from metadata, falling back to empty strings if not present. [1] [2] [3]
• Added property accessors for invocation_start_time and invocation_end_time in the model class for convenient and consistent access.

Submission checker logic improvements:

• Modified the checkpointing submission checker to prefer the new invocation bookend fields (invocation_end_time and invocation_start_time) when calculating the cache-flush gap, with a fallback to the older summary fields for backward compatibility. This ensures startup/collection overhead is not incorrectly included in gap measurements.
• Updated error reporting in the checker to display the correct fields used for timing, reflecting the new logic.

[github-actions]

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

[wolfgang-desalvador]

This is again, down to the report, the same issue with the gap calculation @FileSystemGuy . Reported it here #825

[FileSystemGuy]

Thanks for this — the mechanism is right, and the model-plumbing of the invocation bookends is exactly what the reportgen path was missing. I'd like to accept this approach, but there's one gap I think we need to close before it fully fixes #825 for the v3.0 round.

The constraint: older submissions are eligible, and they have no bookends

For v3.0 we are accepting previously-generated results with no regeneration required. That matters here because the invocation bookends are very recent additions to the v3.0 codebase, not a baseline every result dir carries:

• invocation_start_time (read side) landed in fix(#696): §4.7.1 gap origin = read invocation start, not summary start #714 (b2dc7d9)
• invocation_end_time (write side) landed in fix(storage#782): exclude write-side teardown from §4.7.1 failover-callout gap #787 / storage#782 (7b44f61)

So a large fraction of eligible submissions were generated before #787 and have invocation_end_time == "" (and some, pre-#714, also have invocation_start_time == "").

The flaw this creates in the current PR

For a pre-#787 result dir, the write-side fallback chain collapses straight to the summary end:

write_end = (
    _parse_summary_timestamp(ordered[0].invocation_end_time)  # "" -> None
    or _parse_summary_timestamp(ordered[0].end_datetime)      # -> summary "end"
)

That is exactly the measurement #825 is about — it charges the write-side post-benchmark cluster collection against the 30s cache-flush budget. And because that collection window scales with node count, the large-topology submissions are precisely the ones that will still trip the false 4.7.1 violation. So as written, the PR fixes #825 for freshly-generated (bookend-carrying) results but silently leaves it open for the older, larger eligible submissions.

The standalone submission checker already handles those older dirs — it has a middle fallback tier between the bookend and the summary end:

• submission_checker/checks/checkpointing_checks.py (the 4.7.1 checkpointCacheFlushValidation check) selects the write-side origin as: invocation_end_time → latest post-benchmark collection_timestamp (_latest_final_collection_timestamp(...)) → summary end.

The reportgen path in this PR only has tiers 1 and 3, so it diverges from the standalone checker for exactly the pre-#787 vintage. (That middle tier exists specifically because pre-bookend dirs are expected to validate — which is the same situation we're in for this round.)

Suggested enhancement

Add the middle tier to the reportgen path so it matches the standalone checker for the write side:

1. Carry a precomputed write-side proxy on the model. Add a field to BenchmarkRunData (e.g. final_collection_time: str = "") and populate it in both parse() and _from_metadata() — both already receive the full metadata.json dict — by reusing the existing helper:

   from mlpstorage_py.submission_checker.checks.helpers import _latest_final_collection_timestamp
   # summary_end is the same value used for end_datetime
   final_collection_time = _latest_final_collection_timestamp(metadata, summary_end) or ""

   rules/models.py already imports from submission_checker (dlio_summary_helpers, line ~603), so this follows the existing dependency direction — no new coupling.

2. Insert it as the middle write-side tier in the checker:

   write_end = (
       _parse_summary_timestamp(ordered[0].invocation_end_time)      # #787+ dirs
       or _parse_summary_timestamp(ordered[0].final_collection_time) # pre-#787: node-release proxy
       or _parse_summary_timestamp(ordered[0].end_datetime)          # oldest: summary end
   )

   The formats are compatible: _latest_final_collection_timestamp returns a naive ISO string, and _parse_summary_timestamp tries datetime.fromisoformat(value) first, so it parses cleanly.

3. Read side can stay as you have it (invocation_start_time → run_datetime). There's no equivalent proxy for read-side framework startup, and its overhead doesn't scale with node count, so it's far less likely to push a real run over 30s. One nuance to be aware of (not necessarily to solve in this PR): the standalone checker downgrades a breach to a warning when it had to fall back to the read summary-start origin, whereas the reportgen path emits a hard INVALID. If you'd rather not diverge there, one option is to not emit an INVALID from reportgen when the authoritative read origin is absent — but that's a separate, lower-priority decision.

4. Regression test. A BenchmarkVerifier-level test over a pre-fix(storage#782): exclude write-side teardown from §4.7.1 failover-callout gap #787 fixture (bookend absent, per-node collection_timestamps present) whose summary-gap > 30s but proxy-gap ≤ 30s, asserting the result is now CLOSED rather than INVALID. The conftest.py checkpointing fixtures (chkpt_write_teardown_seconds, chkpt_omit_invocation_start_time) already produce this shape.

With the middle tier added, this PR would fix #825 for the whole eligible set — including the large-topology older submissions that are the ones most likely to hit it — while staying a bounded change (one field + one helper reuse + one fallback line on top of what you have). Happy to pair on it or push a follow-up commit if that's easier. Thanks again!

[FileSystemGuy]

@wolfgang-desalvador Just in case you weren't notified by github that I added a response to your PR...

[wolfgang-desalvador]

Thanks for this — the mechanism is right, and the model-plumbing of the invocation bookends is exactly what the reportgen path was missing. I'd like to accept this approach, but there's one gap I think we need to close before it fully fixes #825 for the v3.0 round.

The constraint: older submissions are eligible, and they have no bookends

For v3.0 we are accepting previously-generated results with no regeneration required. That matters here because the invocation bookends are very recent additions to the v3.0 codebase, not a baseline every result dir carries:

• invocation_start_time (read side) landed in fix(#696): §4.7.1 gap origin = read invocation start, not summary start #714 (b2dc7d9)
• invocation_end_time (write side) landed in fix(storage#782): exclude write-side teardown from §4.7.1 failover-callout gap #787 / storage#782 (7b44f61)

So a large fraction of eligible submissions were generated before #787 and have invocation_end_time == "" (and some, pre-#714, also have invocation_start_time == "").

The flaw this creates in the current PR

For a pre-#787 result dir, the write-side fallback chain collapses straight to the summary end:

write_end = (
    _parse_summary_timestamp(ordered[0].invocation_end_time)  # "" -> None
    or _parse_summary_timestamp(ordered[0].end_datetime)      # -> summary "end"
)

That is exactly the measurement #825 is about — it charges the write-side post-benchmark cluster collection against the 30s cache-flush budget. And because that collection window scales with node count, the large-topology submissions are precisely the ones that will still trip the false 4.7.1 violation. So as written, the PR fixes #825 for freshly-generated (bookend-carrying) results but silently leaves it open for the older, larger eligible submissions.

The standalone submission checker already handles those older dirs — it has a middle fallback tier between the bookend and the summary end:

• submission_checker/checks/checkpointing_checks.py (the 4.7.1 checkpointCacheFlushValidation check) selects the write-side origin as: invocation_end_time → latest post-benchmark collection_timestamp (_latest_final_collection_timestamp(...)) → summary end.

The reportgen path in this PR only has tiers 1 and 3, so it diverges from the standalone checker for exactly the pre-#787 vintage. (That middle tier exists specifically because pre-bookend dirs are expected to validate — which is the same situation we're in for this round.)

Suggested enhancement

Add the middle tier to the reportgen path so it matches the standalone checker for the write side:

1. Carry a precomputed write-side proxy on the model. Add a field to BenchmarkRunData (e.g. final_collection_time: str = "") and populate it in both parse() and _from_metadata() — both already receive the full metadata.json dict — by reusing the existing helper:

   from mlpstorage_py.submission_checker.checks.helpers import _latest_final_collection_timestamp
   # summary_end is the same value used for end_datetime
   final_collection_time = _latest_final_collection_timestamp(metadata, summary_end) or ""

   rules/models.py already imports from submission_checker (dlio_summary_helpers, line ~603), so this follows the existing dependency direction — no new coupling.

2. Insert it as the middle write-side tier in the checker:

   write_end = (
       _parse_summary_timestamp(ordered[0].invocation_end_time)      # #787+ dirs
       or _parse_summary_timestamp(ordered[0].final_collection_time) # pre-#787: node-release proxy
       or _parse_summary_timestamp(ordered[0].end_datetime)          # oldest: summary end
   )

   The formats are compatible: _latest_final_collection_timestamp returns a naive ISO string, and _parse_summary_timestamp tries datetime.fromisoformat(value) first, so it parses cleanly.

3. Read side can stay as you have it (invocation_start_time → run_datetime). There's no equivalent proxy for read-side framework startup, and its overhead doesn't scale with node count, so it's far less likely to push a real run over 30s. One nuance to be aware of (not necessarily to solve in this PR): the standalone checker downgrades a breach to a warning when it had to fall back to the read summary-start origin, whereas the reportgen path emits a hard INVALID. If you'd rather not diverge there, one option is to not emit an INVALID from reportgen when the authoritative read origin is absent — but that's a separate, lower-priority decision.

4. Regression test. A BenchmarkVerifier-level test over a pre-fix(storage#782): exclude write-side teardown from §4.7.1 failover-callout gap #787 fixture (bookend absent, per-node collection_timestamps present) whose summary-gap > 30s but proxy-gap ≤ 30s, asserting the result is now CLOSED rather than INVALID. The conftest.py checkpointing fixtures (chkpt_write_teardown_seconds, chkpt_omit_invocation_start_time) already produce this shape.

With the middle tier added, this PR would fix #825 for the whole eligible set — including the large-topology older submissions that are the ones most likely to hit it — while staying a bounded change (one field + one helper reuse + one fallback line on top of what you have). Happy to pair on it or push a follow-up commit if that's easier. Thanks again!

Working on it @FileSystemGuy