fix(#696): §4.7.1 gap origin = read invocation start, not summary start#714
Merged
Conversation
The §4.7.1 30s gap check measured read.summary.start_time - write.summary.end_time, which charges the read invocation's own framework startup (Python imports, MPI spawn, CAP env-validation — ~50s in the wild) against the failover-callout budget and makes a legitimate split unmeetable. Rules.md §4.7.1 also demanded a split that the checker no longer required, leaving submitters between contradictory instructions. Rules.md §4.7.1 rewritten to (a) lead with the universal write→read ordering common to both combined and split modes, (b) reframe split mode's purpose as enabling an external failover callout (with cache flush as one common in-callout activity, not the point of the split), (c) define the gap origin as read.metadata.invocation_start_time - write.summary.end_time, and (d) treat a negative gap as NTP clock skew rather than a causality violation. Table 3 footnote follows suit. Implementation: - mlpstorage_py/_invocation.py captures INVOCATION_START at first import; main.py imports it before any heavier mlpstorage_py module. - benchmarks/base.py Benchmark.metadata emits invocation_start_time (ISO local naive, matching DLIO summary.json format) for every run. - checkpointing_checks.py cache_flush_validation reads the read-side origin from read_metadata; missing field → regenerate-required error; negative gap → warn_violation clock-skew note (not a hard violation). - Fixture and tests updated; new coverage for missing-invocation-time and negative-gap paths. Closes #696.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Original PR made cache_flush_validation hard-fail with a "regenerate"
message whenever a split-mode read-phase metadata was missing
invocation_start_time. That's the wrong policy for results directories
produced by an mlpstorage version predating this rule: those runs were
legitimately allowed to produce that shape, and forcing a regenerate on
validate is user-hostile.
New behavior for split-mode pairs whose read metadata has no
invocation_start_time:
* Fall back to read.summary.start_time as the gap origin (the legacy
measurement).
* If the legacy-origin gap is <= 30s → silent pass (matches what the
old rule would have accepted).
* If the legacy-origin gap is > 30s → warn_violation, not
log_violation. Honor the rule that was in force when the run was
produced by warning rather than failing. Message tells the submitter
the measurement includes framework startup and is not authoritative;
regenerating with a current mlpstorage produces the honest number.
* If BOTH origins are missing (metadata AND summary) → hard error
("no read-side origin") because there's nothing to measure against.
Combined-mode submissions are still silently accepted (no split pairs
= early return = no gap check), so old combined-mode results
directories continue to validate cleanly.
Tests updated:
- `test_split_mode_missing_invocation_start_time_falls_back_to_legacy_origin_and_passes`
(replaces the previous "regenerate required" assertion).
- New: `test_split_mode_legacy_origin_gap_over_30_emits_warning_not_error`
covers the downgraded-severity path.
Fixture docstring for chkpt_omit_invocation_start_time updated to
describe the fallback path it now exercises.
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
write.summary.end_timetoread.summary.start_time— but the read invocation's own framework startup (Python imports, MPI spawn, CAP env-validation, ~50s in the reporter's evidence) is recorded insidestart_time, so the mandated split hard-fails 4.7.1 on a legitimate configuration. Meanwhile the checker also accepted combined mode with no gap check, so a submitter following the prose failed while a submitter following the checker passed.invocation_start_timefield in run metadata, captured atmlpstorage main.pyentry before any heavy imports. Storage-only change; zero DLIO edits.Details
Rules.md
read.metadata.invocation_start_time − write.summary.end_time; negative gap treated as NTP clock skew, not a causality violation.Code (storage-only)
mlpstorage_py/_invocation.py(new): capturesINVOCATION_START = time.time()at first import.mlpstorage_py/main.py: imports_invocationbefore any heaviermlpstorage_pymodule so the timestamp is recorded before framework startup.mlpstorage_py/benchmarks/base.py:Benchmark.metadataemitsinvocation_start_time(ISO local naive, matching DLIO'ssummary.jsonformat) for every run — write, read, and combined.mlpstorage_py/submission_checker/checks/checkpointing_checks.py::cache_flush_validation: reads the gap-start fromread_metadata["invocation_start_time"]instead ofread_summary["start_time"]. New "missing invocation_start_time" error tells older submissions to regenerate. Negative gap →warn_violation(clock-skew note), not a hard error. The strict no-overlap ordering check remains incheckpoint_invocation_structureunchanged.Tests
mlpstorage_py/tests/conftest.py: fixture always emitsinvocation_start_timein checkpointing metadata. Existingchkpt_cache_flush_gap_secondskwarg now controls the read metadata'sinvocation_start_timerelative to write summaryend_time; may be negative to model clock skew. New opt-out kwargchkpt_omit_invocation_start_timeexercises the missing-field branch.test_checkpointing_check_phase2.py: "missing timestamps" assertion updated for the new write-side message; new tests for missing-invocation_start_time (regenerate-required) and negative-gap (clock-skew warning).Empirical impact on the reporter's evidence
Using the exact timestamps from the #696 evidence:
read.summary.start_time=03:57:12.759read.metadata.invocation_start_time≈03:56:21.296(mlpstorage entry, ~60ms afterdrop_cachesended)write.summary.end_time=03:56:18.199The unmeetable ceiling becomes trivially meetable without loosening the semantic that the rule was actually trying to enforce.
Test plan
pytest mlpstorage_py/tests tests/unit -q→ 3423 passed, 1 skipped, 0 failures.cache_flush_validationcovered for: combined mode (silent pass), split with 25s gap (pass), split with 45s gap (fail), missing writeend_time(fail w/ regenerate hint on write side), missing readinvocation_start_time(fail w/ regenerate hint), negative gap (warning, not violation).Closes #696.