4th: feat(reportgen): fixed webpage-parity schema for results.{csv,json} - #829
Draft
FileSystemGuy wants to merge 79 commits into
Draft
4th: feat(reportgen): fixed webpage-parity schema for results.{csv,json}#829FileSystemGuy wants to merge 79 commits into
FileSystemGuy wants to merge 79 commits into
Conversation
* Fix calculation of checkpoint gap in reports * Implement fallback to collection last timestamp
…chema The v3.0 results web page is 8 tables sharing a System-Under-Test block. reportgen must emit ONE flat results.csv/json that a staff member reduces to any single table by deleting non-applicable columns. That requires a FIXED schema (exact reference columns + Division/Benchmark Type/Model discriminators), not the current data-driven column set. Pins the emitted-file contract: header is EXACTLY the 54-column fixed schema; JSON keys match in order; internal machine columns (category/ orgname/systemname/benchmark_type prefix, *_mean_of_* dynamic means, trailing issues) are gone; values cherry-picked into fixed columns (Division=CLOSED/OPEN, Model=display label, kvcache option N -> its fixed group). Fails against the current dynamic _ordered_fieldnames.
results.{csv,json} is now a FIXED contract — exactly the reference columns
for all 8 v3.0 tables (Results Table Structure.xlsx) plus 3 discriminator
columns (Division / Benchmark Type / Model), never data-driven. A staff
member opens results.csv in Excel and reduces it to any single webpage
table by deleting the workload blocks + discriminators that don't apply.
Approach: output projection, not a rewrite of aggregation. The internal
_aggregate_* helpers (train_mean_of_* / checkpoint_mean_of_* / vdb_* /
kvcache_* machine keys, and the empty-metric StatisticsError -> INVALID
gate) are untouched, so validity behavior is unchanged. A new _final_row()
projection runs inside write_json_file/write_csv_file and cherry-picks each
fixed column from the in-memory row; the dynamic *_mean_of_* means and the
trailing issues column are simply no longer emitted (output-only change).
Schema (54 cols): 13-col left edge (Public ID, Organization, Division,
Benchmark Type, Model, Name, Description, Type, Access Protocol,
Availability, RU's, Integrated Client Storage, Usable Capacity) then
workload-qualified Training/Checkpointing/VDB/KVCache blocks (shared metric
names are unique-per-column, so they carry a 'Training - ' etc. prefix).
- Division = category upper-cased; Model = reference display label
(llama3-1t -> 1250B), blank for kvcache/vdb (single-table workloads).
- Name shows the system name -> .yaml; Description shows 'PDF' -> .pdf.
- Code/Logs are per-workload (an OPEN row may ship its own code), routed
into the row's own block from the code-image pointer.
- KVCache option 1/2/3 -> the reference groups (llama3.1-8b Storage Only /
Storage + Mem / llama3.1-70b Storage Only).
- Removed the machine-key prefix (category/orgname/systemname/
benchmark_type/model/accelerator), the issues column, and the now-dead
_ordered_fieldnames helper.
Output-file tests migrated to the fixed-schema contract; internal-structure
tests (test_aggregation helper keys) unchanged. All 4 CI suites green.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
…scores Two-invocation checkpoint submissions (Rules §2.1.23/§4.7.1: mandatory write→read split when checkpoint-per-node < 3x client RAM) blank all four score columns. Each phase dir carries only its own scalars (write dir: save_*; read dir: load_*), so _scalar_mean's all-summaries-must-be-numeric gate sees [4.64, None] and returns None. Adds three cases to TestCheckpointingFinalTableColumns: - split write→read populates all four (RED — reporter's Alluxio bug) - two combined invocations average per direction (locks averaging) - a metric missing from a phase that WAS configured to produce it stays a loud blank, never a silent partial-mean (locks the guard)
…t invocations
Two-invocation checkpoint submissions (Rules §2.1.23/§4.7.1 write→read
split, mandatory when checkpoint-per-node < 3x client RAM) blanked all four
score columns: the write dir carries only save_* and the read dir only
load_*, so the old all-summaries-must-be-numeric gate saw [4.64, None] and
returned None.
Replace _scalar_mean with _directional_mean: classify each invocation as a
write/read producer from its configured checkpoint.num_checkpoints_{write,
read} (CLOSED forces 10 or 0), then take the mean over the producing phase
only. The loud-failure guard (D-23 / PITFALLS #3: no silent partial-mean)
is preserved but scoped to the producing phase — a scalar missing from an
invocation that WAS configured to produce it stays a blank, never a partial
mean over the survivors. Falls back to a present-only mean when the phase
signal is absent (legacy / OPEN packages) so those rows don't regress.
Fixes the reporter's Alluxio CLOSED v3.0 case: llama3-8b/70b/405b now
populate Write/Read B/W and durations from their split timestamp dirs.
…heck (#831) Datasets generated before the floor->ceil datagen alignment (commit f9d414d, first shipped in mlpstorage 3.0.43) were sized with integer floor division in rules/utils.py. Rule 3.1.2 in the submission checker now re-derives the threshold with ceil, so it rejects a valid pre-alignment dataset by exactly one file (a submitter hit "actual files 4710038 < minimum required 4710039" on a Jun-28 run made with 3.0.24). Relax the 3.1.2 threshold to ceil-minus-one for the v3.0 round. We can't just floor it: datagen floored via integer `//` while this check recomputes the same formula as a float chain, so float drift can push floor() one above the recorded count and still reject. ceil-1 equals floor() for non-integer ratios and grants one file of slack exactly at the integer boundary where that drift bites, absorbing the bounded <=1-file divergence. Stays int-vs-int (no "N < N" display bug) and still rejects genuinely undersized runs (short by >1 file). Validator-only change; rules/utils.py (datagen/runtime verifier) stays on ceil. Revert to plain ceil once the v3.0 round closes -- post-3.0.43 datagen ceils, so newer datasets pass either way.
The VectorDB workload grouping key (D-06) is (category, orgname, systemname, engine, index_type) — it does not include `command`, so a workload's datasize/datagen/run leaves all group under one key and arrive at `_aggregate_vdb` as a single `runs` list in discovery order. Only the `run` leaf carries the native query-phase metrics (statistics.json / summary.json); the datasize and datagen leaves have none. `_aggregate_vdb` used `run = runs[0]`, so whenever a non-run leaf sorted first the QPS / latency / recall / read-B/W columns came out blank even though the run leaf's statistics.json held the real values (identity columns still populated because they live in every leaf's parameters). Select the `run` invocation for metrics and recall, falling back to runs[0] only when no run leaf is present (e.g. a datasize-only tree) so identity columns still populate. Resolve identity columns across all grouped leaves (run first) so a value recorded only on the datasize leaf (e.g. num_vectors / dimension) still fills in. Regression test: a [datasize, datagen, run] group now reads its metrics from the run leaf.
…thering) into reportgen-column-parity integration branch # Conflicts: # mlpstorage_py/rules/models.py
…orts mode An assembled multi-submitter submission tree has no single-org mlperf-results.yaml and must stay read-only, so reportgen against it dies at E101 today. reportgen already supports orgname=None (discover_scan_roots flat-layout fallback); pin the intended contract: reports mode proceeds with args.orgname=None when the sentinel is absent, keeps sentinel resolution when present, and benchmark modes keep the hard-fail.
…mode reportgen against an assembled multi-submitter submission tree died at E101 because _apply_lay03_orgname_gate hard-requires the mlperf-results.yaml sentinel for every --results-dir-bearing mode. Such a tree has no single-org sentinel, must stay read-only (so 'mlpstorage init' is not an option), and ReportGenerator already handles orgname=None via the discover_scan_roots flat-layout fallback. For mode=='reports' only: catch ResultsDirNotInitializedError, set args.orgname=None, log, and proceed. Sentinel-bearing dirs keep exact prior behavior (canonical-slice discovery); benchmark modes keep the locked hard-fail. All 4 CI suites green. Worklist: .planning/validate-cleanup-worklist.md item A9.
An assembled multi-submitter tree (no sentinel, orgname=None) must:
- A10: yield every {closed,open}/<org>/results/<system> slice as scan
roots so structure validation passes, instead of validating the raw
root and exiting 3.
- A11 (decision (a), 2026-07-24): write the global results.{csv,json}
at the tree root covering every org, plus a per-org rollup in each
<div>/<org>/results/ — instead of rebinding to the alphabetically
first org and dropping the aggregate for everyone into its tree.
A10: discover_scan_roots with orgname=None now probes the canonical
{closed,open}/<org>/results/<system> shape for EVERY org and returns
each system slice as a scan root. The raw root of an assembled
multi-submitter tree holds only division dirs and code-pool dirs, so
pre-fix it failed structure validation and reportgen exited 3 before
generating anything. Flat layouts still pass through unchanged.
A11 (decision (a), 2026-07-24): _resolve_effective_results_dir now
collects every (division, org) canonical match instead of returning on
the first org. With >= 2 distinct orgs the tree root stays the
effective results dir — the global results.{csv,json} covering every
org lands there — and each org additionally receives its own rollup in
<division>/<org>/results/. Single-org trees keep the established
rebind-to-org-results behavior exactly.
Also relocates the D-08 top-level assertions back to the end of
test_multi_orgname_produces_single_top_level_file_with_mixed_rows (the
RED commit accidentally split that test) and updates its stale
first-org-wins comment.
Special build for one submitter: a >30s write→read inter-phase gap in a split-mode checkpointing submission must be reported as a warning rather than a hard error, in both the submission checker (cache_flush_validation) and the reportgen-side rules checker (check_invocation_structure).
Downgrade the >30s write→read inter-phase gap breach from a hard error to a warning in both enforcement points: - submission checker cache_flush_validation: warn_violation instead of log_violation; the check no longer returns False for a gap breach - reportgen-side check_invocation_structure: warning-severity CLOSED issue instead of PARAM_VALIDATION.INVALID Missing/unparseable timestamps and overlapping phases remain hard errors. Originally authored on the checkpoint-split-gap-warning special build (PR #834) for one submitter; Curtis decided 2026-07-24 the relaxation applies to the whole v3.0 review tree, so it lands on the integration branch for all submitters (worklist A7). Cherry-picked from c45278c.
…k label Third enforcement point of the A7 §4.7.1 relaxation: the 2.1.24 gap breach warns instead of invalidating, and the message carries an 'upper bound' label when the gap was measured via the DLIO summary fallback (no invocation bookends in metadata), since that path charges startup/collection overhead against the quiet window. Unparseable timestamps remain hard errors.
…lback measured Completes worklist A7: the third gap-enforcement point downgrades to a warning like cache_flush_validation and check_invocation_structure. The message gains an upper-bound note when either gap endpoint fell back to the DLIO summary fields (bookends absent from metadata) — that path charges read-side startup and write-side cluster collection against the quiet window, so the number overstates the true gap on large topologies. Unparseable timestamps remain hard errors.
…rning A1: training datagen dirs can never hold summary.json (DLIO generate_only skips stats.save_data), so their absence must not warn; run-dir summaries keep warning. A4: a missing metadata file must warn once (find-time, with dir context), not the find+load pair.
A1: the training datagen summary.json load is warn_if_missing=False — DLIO's generate_only path never writes one, so its absence was pure noise (77 of 79 baseline 'Could not load Summary log' warnings). Presence still loads; run-phase summaries keep warning. A4: find_metadata_path returns None (after its own directory-context warning) instead of a nonexistent default path, and load_single_log treats a None path as an already-reported miss — collapsing the 46 find+load warning pairs to singles.
…uity --data-dir is optional for training datasize, so null-dir records must match any run; a single datasize record matches regardless of dir (cardinality comparison is the rule's point); pruned metadata is MALFORMED, not MISMATCH. MISMATCH fires only with >=2 distinguishable records none of which match — the existing mismatch test gains a second distinct record accordingly.
_match_datasize_for_run gains two fallbacks when no exact data_dir match exists: the all-null-dir group matches any run (--data-dir is optional for datasize), and a single overall record matches regardless of dir. The mismatch caller keys off the readable grouping instead of raw datasize_files, so pruned-metadata trees report DATASIZE-MALFORMED without a per-run MISMATCH pile-on. Eliminates the bulk of the 160 baseline 3.3.1 warnings as false positives.
The write+read invocation pair shares checkpoint_size_GB and host memory, so the per-invocation loop emitted the identical advisory twice per workload. Warn once per distinct (size, memory, hosts) tuple.
… dedupes per condition
…ation The 5.1.1 scale-table / 5.3.2 recall-target / 5.3.3 query-target 'not yet defined in config.py' notes describe validator incompleteness, not submission problems — they no longer appear as per-submission WARNINGs (12 baseline lines). BaseCheck gains info_violation so the notes keep the grep-stable [rule_id ruleName] prefix at INFO.
Subclass names already end in 'checks', so the literal in the format produced 'Some directory checks Checks failed for' (107 baseline lines). No test depended on the old sentinel — the docstring claim was stale.
…rtial failures Deterministic selection plus two warnings, on the evidence that the crux-eagle case is not unique: - `_select_kvcache_run` replaces `runs[0]`. Discovery order was genuinely arbitrary — of the three v3.0 groups holding two real runs, it published the earlier run for two and the later run for the third. Earliest `run_datetime` has no better claim than latest, but it is a rule rather than an accident. - `_kvcache_measured_runs` excludes `datasize` from selection. The kv_cache grouping key has no `command` component (D-05), so a `datasize` leaf shares the group with the run it sized; it writes no summary.json. An earlier draft of this fix blanked all twelve KVCache metrics on v3.0-0019/0021/0023/0142 before the expectation diff caught it, which is now pinned by a test. - Collapsed-group and partial_failure warnings ride on `Result.issues`. The collapsed-group warning requires two or more actual `run` invocations, so ANL polaris-nvme's pair of `datasize` leaves stays quiet. Against the v3.0 tree: 3 collapsed groups (ANL crux-eagle 3 runs, Everpure 51hosts_20 and 51hosts_30 2 runs each) and 1 partial failure (open/farmgpu llama3.1-70b-instruct, 45 missing rank files). Six of the seven collapses the first draft reported were datasize siblings, not dropped measurements. One published row moves: v3.0-0019 switches from its later run to its earlier one, since discovery order had picked the later. Refs #836
…ilter format_issues_list(show_all=False) drops every Issue whose validation is CLOSED, conflating two orthogonal fields: validation is the submission category, severity is whether a human needs to look. A finding that does not disqualify a CLOSED submission but does mean its published row is misleading is exactly CLOSED + severity="warning". Two families are invisible today — the datagen leaf-presence warnings, written to be "worth surfacing but not invalidating", and the #836 kv_cache collapsed-group and partial_failure warnings. Both reach only the log stream, alongside 183 other warnings. Refs #836
… list format_issues_list(show_all=False) now keeps CLOSED issues whose severity is "warning", badges them [WARN] in yellow rather than green [CLOSED], drops the "None: " placeholder when an issue carries no parameter, and absorbs the message's own "[WARN] " lead-in so badges don't stack. Ordinary CLOSED chatter still goes. Against the v3.0 tree this adds five lines to the printed summary — the four #836 kv_cache warnings and one datagen leaf-presence warning that had been written to be surfaced and never was. Refs #836
A kv_cache or vector_database run leaf missing its *_metadata.json is the only case where the metadata file is load-bearing: DLIO training and checkpointing runs still resolve their type through Hydra configs. The leaf raises "Could not determine benchmark type", get_runs_files logs "Failed to load run from <dir>" at WARNING and drops it, and the workload publishes with every metric column blank. The existing warning names neither the consequence, nor the missing input, nor that restoring one small file recovers it — and it is one line in a ~15,000-line reportgen pass. Pins #835 suggestions 1 and 2: 1. get_runs_files records each dropped leaf as a SkippedRunDir (path, reason, actionable detail) and its warning names both the blank-metrics consequence and *_metadata.json; 2. reportgen ends with a SKIPPED RUN DIRECTORIES section, including on the all-dropped path where print_results short-circuits to "No results to display". Also covers the present-but-incomplete metadata case, which drops identically but is not described in #835, and the multiple-metadata continue. Negative controls pin no false positives on a clean tree and back-compat for callers that pass no skipped list.
get_runs_files now takes an optional `skipped` list and records each
directory it could not load as a SkippedRunDir(path, reason, detail).
Three reasons are distinguished:
undetermined_type no usable *_metadata.json and no DLIO workflow
signal — the kv_cache / vector_database case, where
the metadata file is the only type signal and the
remedy is restoring one small file, not rerunning
the benchmark;
multiple_metadata more than one *_metadata.json in a leaf;
load_error anything else.
The first is separated from the rest by a new BenchmarkTypeUndetermined
exception rather than by matching the message text. It subclasses
ValueError, so existing handlers are unaffected.
Each warning now names the consequence ("metrics will be BLANK in
results.csv") and the remedy instead of only "Failed to load run".
ReportGenerator collects the drops across all scan roots, logs a count
mid-pass, and prints a SKIPPED RUN DIRECTORIES section at the end —
including on the all-dropped path, where print_results short-circuits
to "No results to display" and previously said nothing about what it
had thrown away.
Verified against a hardlink clone of the v3.0 submission tree: zero
directories skipped as it stands today, and with one Everpure kv_cache
and one TTA vector_database metadata file moved aside, both are named
in the section with the correct count.
Seven get_runs_files test doubles in test_reporting.py pinned the old
two-argument signature. accumulate_results wraps the call in
`except Exception`, so the TypeError surfaced as an empty scan rather
than an error — the doubles now take **kwargs.
…remedy
ResultsDirectoryValidator reports errors as DirectoryValidationError
records (path, error_type, message, suggestion) but warnings as bare
strings — twelve append sites with no path, no category, no remedy.
The gap is worst in _validate_model_dir. A model directory whose
children are neither YYYYMMDD_HHMMSS run dirs nor known command dirs
yields one line — "No valid run directories found in <dir>" — and the
walk stops there. Nothing beneath those children is examined, so any
runs inside them are invisible to both validation and the report. The
message states none of that. It is live on the v3.0 tree against
closed/ANL/results/crux-eagle/kv_cache/llama3-8b-10u, whose children
are 1nodex8ppn / 8nodex8ppn / 64nodex8ppn.
Beside it, a quieter gap: _validate_command_dir warns about children it
does not recognize, _validate_model_dir has no such branch, so junk
sitting next to real runs is dropped in silence.
Pins:
- warnings become DirectoryValidationWarning with the same four
fields as errors, and every one carries a path and a suggestion;
- a subtree that could not be descended names each child it could not
interpret and says it was not examined;
- unrecognized siblings of valid runs are reported, without either
double-reporting the all-invalid case or costing a valid run its
count;
- the detail reaches a human — get_error_report renders path and Fix,
and reportgen logs the message rather than a dataclass repr.
Same property as the #835 skipped-run work — never drop part of the
tree without saying so — applied to the structural layer.
DirectoryValidationWarning gives warnings the four fields errors have
had all along — path, warning_type, message, suggestion — across all
twelve append sites. Four types: unexpected, empty, incomplete, and
unreachable, the last meaning a subtree the walk could not descend, so
anything inside it is invisible to validation and the report alike.
__str__ returns the bare message so anything that stringifies a warning
keeps reading as prose.
_validate_model_dir gains the branch its command-level twin already
had. Children that are neither YYYYMMDD_HHMMSS run dirs nor known
command dirs are now collected and named — in the aggregate warning
when nothing in the directory is valid, individually when real runs sit
beside them. Never both, so a directory is reported once. Previously
the second case was silent.
The aggregate message now says what it found instead of what it
expected and that nothing below was examined. Against the live tree:
No run directories found in .../crux-eagle/kv_cache/llama3-8b-10u:
none of its 3 subdirectories is a YYYYMMDD_HHMMSS run directory or a
known command directory (['run', 'datagen', 'datasize']). Found
instead: 1nodex8ppn, 64nodex8ppn, 8nodex8ppn. Nothing below them was
examined — any runs they contain are invisible to both this
validation and the report.
Fix: Place each run at <model>/<command>/<YYYYMMDD_HHMMSS>/ ...
replacing "No valid run directories found in <dir>".
get_error_report renders each warning with its path and fix, matching
the error block, and reportgen logs message plus fix rather than a
dataclass repr. Five assertions in test_directory_validator.py move
from substring-on-string to .message.
Whether the validator should descend such subtrees anyway is a
behavior question that overlaps the crux-eagle layout decision in #836
and is deliberately not settled here.
Earliest-`run_datetime` selection was a rule rather than an accident, but it was still the wrong rule. A submitter iterates — run, change something, run again — so the last run is the one they mean to publish. Everpure's 51hosts_20 and 51hosts_30 each package exactly that pair, and earliest-wins publishes the first attempt of each. Latest alone is not enough either. ANL crux-eagle's two later runs lost 70% and 97% of their per-rank result files with all 9 mpirun trials exiting non-zero; their figures are an fmean over the 3-30% of ranks that survived. Proved against a copy of the tree: shifting the 64-node run's run_datetime to sort last publishes 0.0 tok/s and a 281-second P95 into v3.0-0005. So: latest error-free run, falling back to latest when none is error-free. Two existing tests re-pinned (earliest -> latest), three added: - prefers-latest-error-free, and the collapsed-group warning marks which dropped runs recorded a partial failure, so the choice reads as principled rather than positional. - the all-failed fallback, which must still publish deterministically and flag the published run. - a summary-less leaf must not win on recency. ZettaLane's run/20260627_194318 is one: option dirs, trial logs and metadata, but its summary is under the older kvcache_run_summary_*.json name, so every pass-through metric reads blank. Naive latest-wins would let such a leaf blank a row that has real measurements. The datasize-filter test now puts the datasize leaf LATER than the run (both timestamps Everpure 51hosts_20's own) so it keeps guarding under the new rule instead of passing vacuously. Refs #836
`_select_kvcache_run` now takes the LATEST error-free run in the group, falling back to the latest of any kind when none is error-free. Rationale is the submitter's own workflow: run, change something, run again — the last run is the one they mean to publish. Everpure's 51hosts_20 and 51hosts_30 each package exactly that pair. `_kvcache_run_defect` decides eligibility from the run's own summary and returns the marker a warning shows: - `partial_failure` — an option lost per-rank result files, or an mpirun trial exited non-zero (`trial_failures`). Recency must not outrank this: crux-eagle's two later runs lost 70% and 97% of their ranks with all 9 trials failing, so latest-alone publishes 0.0 tok/s and a 281-second P95. - `no summary.json` — nothing to publish from, so every pass-through metric would read blank. ZettaLane's run/20260627_194318 is such a leaf (option dirs, trial logs, metadata; summary under the older kvcache_run_summary_*.json name). Candidates are read with a new `_load_workload_summary(quiet=True)`: selection consults every run in the group, and the missing-summary warning belongs to the run that gets published, not to each one weighed. The collapsed-group warning now marks each dropped run's defect, so a later run in the dropped list reads as the rule working rather than as positional accident: published 1nodex8ppn (20260723_062638); dropped 8nodex8ppn (20260723_191548) [partial_failure], 64nodex8ppn (20260724_051511) [partial_failure] Full-tree diff, 179 rows, before/after on a copy of submissions_storage_v3.0 @ be782229a: exactly 2 rows move, both Everpure iteration pairs advancing to their later run (v3.0-0019, v3.0-0021). v3.0-0019 improves on all 12 cells; v3.0-0021 improves throughput and bandwidth but its P95 latencies rise — the rule is "latest", not "best", so it does not cherry-pick. crux-eagle (v3.0-0005) is unchanged: it was already publishing the clean 1-node run by luck, and now does so by rule. All four CI suites pass (238 + 3083 + 911 + 228). Refs #836
A workload good enough to extract, verify and aggregate must appear in
the table that is supposed to contain it. crux-eagle's does not: its
model directory holds a header-only results.csv while the top-level table
publishes v3.0-0005 from the same runs.
Placement and extraction disagree about what a run is. Extraction reads
each leaf's self-describing *_metadata.json, so a leaf named after its
topology rather than its timestamp still yields a BenchmarkRun.
`_model_group_folder` instead hops a fixed two levels up from the leaf on
the assumption it sits at `<model>/<command>/<ts>/`; one level shallower,
those hops land ON the kv_cache benchmark-type directory. The row is
written to `<system>/kv_cache/results.csv` — a path the canonical layout
has no table at — and the real model directory gets its header-only file
from the D-03 empty-model-dir pass.
`closed/ANL/results/crux-eagle/kv_cache/results.csv` is the only such
file in the v3.0 tree; every other system's kv_cache table sits one level
deeper, inside its model directory.
Two tests, both against the full ReportGenerator pipeline on a planted
two-run flattened tree:
- the model dir's results.json must hold the row the top-level table
publishes, with the same Public ID;
- nothing is written at the benchmark-type directory.
Refs #836
…l dir
`_model_group_folder`'s hop count is right at canonical depth and
overshoots below it. `_clamp_to_benchmark_type_subtree` now checks the
result against the `<system>/<benchmark-type>/` ancestor: if the hops
landed on that directory or above it, the model directory is the ancestor
that is a direct child of it, and the row goes there instead.
Canonical trees are untouched — their hop result already sits below the
anchor, so the clamp returns it unchanged. Fixture trees that plant
`<model>/run/<ts>/` straight under the results root have no
benchmark-type ancestor to anchor on; those also pass through unchanged.
When the leaf itself is the only level below the benchmark-type directory
there is no model level to write a table into. Returning the
benchmark-type directory would write the phantom this fix removes;
returning the leaf would write results.csv inside a run directory. It
returns None and logs the expected layout instead, so the row still
reaches the rollups and the reason is on the record. No such tree exists
in v3.0 today.
Verified on a copy of submissions_storage_v3.0 @ be782229a:
- crux-eagle/kv_cache/llama3-8b-10u/results.csv now holds the row it
was missing (1 row, was header-only), and nothing is written to
crux-eagle/kv_cache/ any more. That stale file is committed in the
submission repo from an earlier run and needs deleting there —
reportgen never prunes (D-04).
- 179 published rows, 0 cells changed. Placement decides which file a
row is written to, not what it says.
- One empty-model-table-with-a-published-rollup case remains, from a
different cause: OpenLake's vdb runs carry empty `parameters`, so
_derive_workload_key's `engine`/`index_type` are both "" and its
HNSW and DISKANN runs land in ONE group. The group is flagged
[INVALID] "Inconsistent models across runs", the row publishes as
HNSW, and five DISKANN runs appear in no table. Left alone here:
fixing it adds a published row and renumbers Public IDs.
All four CI suites pass (238 + 3116 + 911 + 228).
Refs #836
Two rows publish today with every measurement cell blank. The fixed final
schema has no phase column and no issues column, so neither is
distinguishable from a real result that happens to read zero:
v3.0-0006 ANL polaris-nvme — kv_cache group holding only two
`datasize` invocations. R3 keeps training/checkpointing
auxiliary phases out of the rollups via the issue-#771/#791
6-element workload keys, but kv_cache and vector_database
keys are always 5-tuples, so their auxiliary groups were
never marked.
v3.0-0138 ZettaLane mayascale — a `run` group whose summary is named
kvcache_run_summary_<ts>.json, so summary.json is absent and
every pass-through column reads blank.
Three tests. The datasize fixture deliberately keeps a summary.json,
which real kv_cache datasize leaves do not have (0 of them in the v3.0
tree) — that leaves `# Client Nodes` populated, so the test cannot be
satisfied by a blankness check and forces the phase-aware rule instead.
The no-summary test additionally pins that the withheld row is absent
from the MODEL-level table too, not just the rollups, and that the log
names the run directory and says the row carried no measurements. Third
test is the negative control: a run with a summary still publishes as
v3.0-0001.
Refs #836
Withholding ANL polaris-nvme's kv_cache row is right, but as written it happens in silence: the row is marked auxiliary and dropped from the rollups with no diagnostic, so a submitter sees a workload vanish with nothing to act on. For that system this is the ONLY signal available. Its `run` leaf (kv_cache/llama3.1-8b/run/20260721_203800) holds trial logs, option_1 and option_2 but no *_metadata.json, and the #835 skipped-run reporting never names it: a full-tree pass over submissions_storage_v3.0 @ be782229a logs zero "Could not find metadata" lines and prints no SKIPPED RUN DIRECTORIES section, because a leaf with no metadata is not enumerated as a run directory in the first place. Same for open/farmgpu's two pointer-only leaves. That gap is #835's to close; this pins that the withholding itself is at least explained. Extends the datasize-only test: the log must name the system and say the group holds no measurement invocation. Refs #836
Two mechanisms, one per cause found in the v3.0 tree. `_is_auxiliary_only_group` marks a kv_cache / vector_database group that holds no `run` invocation, extending R3 to the 5-tuple keys that have no `command` component to lengthen. ANL polaris-nvme's kv_cache group holds two `datasize` leaves and nothing else — its `run` leaf lost its metadata (#835) — and published v3.0-0006 with every metric blank. Judged on the group's CONTENTS, not on `Result.benchmark_command`. The first draft marked by that label and withheld ALL FIVE vector_database rows in the tree: a vdb group mixes datagen/datasize/run under one 5-tuple key, so its label is only the first run's command, and every vdb group in v3.0 is labelled `datagen` while holding five real `run` invocations. Caught by the full-tree diff, now pinned by the negative control. `_withhold_metricless_row` drops a measurement row whose workload block is entirely blank, from the leaf table and the rollups alike so the two stay consistent. ZettaLane mayascale (v3.0-0138) is the case: its run's summary sits under the older kvcache_run_summary_<ts>.json name, so every pass-through column reads blank. Deliberately conservative — only a wholly blank block qualifies, Code/Logs excluded since a code image is captured before the run; configuration cells like `# Client Nodes` count as content, so a thin row still publishes and the #835/#836 warnings describe it. Withholding a real submission's row would be worse. Both paths log. `_log_auxiliary_only_withholding` exists because the polaris drop was otherwise silent: that leaf holds trial logs and option dirs but no *_metadata.json, and a leaf with no metadata is never enumerated as a run directory, so a full-tree pass logs zero "Could not find metadata" lines and prints no SKIPPED RUN DIRECTORIES section for it. Same for open/farmgpu's two pointer-only leaves — a #835 coverage gap worth reporting there. Training and checkpointing keep their existing silence: their auxiliary phases sit beside a `run` group that publishes. Full tree @ be782229a: 179 rows -> 177, exactly the two all-blank rows gone, zero remaining rows with an empty workload block, and both drops named in the log. Public IDs are positional and regenerated per run, so 155 rows shift up a position or two. polaris-nvme keeps its Rules-mandated datasize leaf file (1 row); mayascale's model table is now empty, matching the rollups. All four CI suites pass (238 + 3119 + 911 + 228). Refs #836
Public IDs are positional: sorted, then numbered v3.0-0001 upward on every run, so withholding or inserting a row renumbers everything after it. That is fine until reviewers, issues and assignment documents cite IDs by number -- then a stale citation does not look broken, it points at a different, valid row. The v3.0 tree renumbered twice today for exactly this reason. Pins IDs to a `public_ids.json` registry beside the global results.csv: - Opt-in by presence. No file, no behavior change -- numbering stays positional and nothing is written. Two tests guard this so existing trees and the whole current test suite keep their semantics. - Seeded from the first run, so bootstrapping does not mean hand-authoring 177 entries. - Sticky thereafter: a row keeps its ID however the table re-sorts around it; a genuinely new row mints max+1. - IDs are never reused. A row that stops publishing leaves its ID reserved and a gap in the table; a later new row mints ABOVE the gap, never into it. Reusing a number would silently redirect every old citation. Restoring the row restores its original ID. - A hand-authored registry wins over what a positional pass would produce (this is the migration path), and a stale `next_index` is repaired from the highest ID present rather than being allowed to mint a duplicate. Identity is the generator's own workload key (D-05/D-06) plus benchmark type, NOT row position and NOT the display columns: the CSV `Model` cell is blank for kv_cache and vector_database while the key's id1/id2 carry model + performance profile / engine + index type. Two kv_cache profiles for one system are two rows that a display-column key would collapse into one. Two identical identities cannot be told apart by any registry, so that case must be reported out loud and the IDs kept unique rather than one being issued twice. 9 failing, 4 passing (the opt-in guards, already correct). Refs #836
Opt-in by presence: a tree carrying `public_ids.json` beside its global
results.csv gets pinned ids; a tree without one keeps positional numbering
and never has the file created for it. That keeps every existing tree and
the whole existing suite semantically unchanged -- including
test_reportgen_public_id.py's `test_inserting_an_earlier_org_renumbers`,
which pins the positional behavior on purpose.
Identity is the workload key, carried on the row as `__workload_key__`
rather than reconstructed later from the display columns. The CSV `Model`
cell is blank for kv_cache and vector_database while the key's id1/id2 hold
model + performance profile / engine + index type, so a display-column key
would collapse two kv_cache profiles for one system into one row and issue
them the same id. `_final_row` projects the internal key away, so nothing
reaches the output.
`_pin_public_ids` guarantees ids are never reused: `next_index` is repaired
from the highest id present rather than trusted, so a hand-edited or stale
registry cannot mint a duplicate, and a retired row's entry stays in the
file reserving its number. Restoring the row restores its original id. New
rows mint above the highest ever issued, never into a gap -- reusing a
number would leave every earlier citation pointing at a different, valid
submission, which is the failure mode that looks correct on inspection.
Three failure modes chosen to be loud rather than convenient:
- An unreadable or non-object registry is NOT treated as absent. Falling
back to positional would renumber the one kind of tree that exists to
stop that happening, so it is reported as an error and treated as empty
(mint everything), which is visible in the diff.
- Two rows with one identity cannot be told apart by any registry. Warn,
naming the identity, and mint for the duplicate so ids stay unique.
- Minting anything at all warns that `public_ids.json` must be committed,
since an unrecorded mint is re-issued differently next run.
`_public_id_registry_path` also honors the results-dir root, because
`global_summary_dir` MOVES with the tree's shape -- a multi-org tree rolls
up at the root, a single-org tree inside that org. Found while writing the
retirement test: deleting one of two orgs relocated the global dir, the
registry stopped being found, and pinning silently reverted to positional.
The test now withholds a run leaf instead of an org, and the lookup no
longer depends on a layout classification.
This is the one non-`results.{csv,json}` file reportgen writes under
<results-dir> -- a deliberate, narrow exception to D-04, and only for a
tree that already carries it.
13 tests in tests/unit/test_reportgen_public_id_registry.py.
All four CI suites pass (238 / 3132 / 911 / 228).
Refs #836
…anged ids Two changes to how pinning engages, both from the same observation: a magic file somebody has to remember to create is a file that will not exist when it matters. **Created automatically.** A tree without a `public_ids.json` gets one, filled from that run's positional assignment. Pinning becomes a ratchet rather than a step in a checklist, and the file that results is the thing staff delete once, before publication, to renumber for release. Nothing else in the tool renumbers -- no flag, since a mechanism whose only use is the once-per-cycle renumber says nothing that deleting the file does not. The cost is accepted deliberately: organizations arriving after the ratchet engages mint above the rows already published rather than displacing them, so numbering drifts out of sort order during a review cycle. Displacing a published id is worse, and the release-time renumber restores order and closes gaps. `test_reportgen_public_id.py`'s renumbering test inverts to say exactly this, and its module docstring no longer claims ids are fully regenerated per run. **The guard.** Because deletion is the renumber trigger, the tool cannot tell a deliberate renumber from a registry that was lost -- a stale clone, a file never checked out, a `git clean`. Both mint a plausible, wrong numbering and record it as authoritative. So the guard does not gate on intent, which it cannot know; it reports consequence: any run that changes an id the tree's existing results.csv already published says so, names the count, and says how to restore the registry if it was lost rather than removed. That text has to read correctly for the staffer who deleted the file on purpose and the one who did not, so it accuses nobody. Generalized past the create case on purpose -- an emptied or truncated registry renumbers identically to a deleted one, and guarding only on absence would miss it. Silent in normal pinned operation: pinning never changes a matched row's id, a minted row was in no earlier table, and a withheld row keeps its id reserved. All three are pinned as tests. The published table is matched on its display columns, which are NOT guaranteed unique -- two kv_cache performance profiles for one system share all five. Ambiguous keys are excluded from the comparison and counted rather than matched arbitrarily and reported as a change that never happened. 7 failing, 24 passing. Refs #836
…hanged ids `public_ids.json` is now created rather than opted into. A tree without one gets one, filled from that run's positional assignment, and is pinned from then on. Deleting the file is the only thing that renumbers -- staff do it once before publication to close gaps and restore sort order, and the run that follows recreates the registry, so the RELEASED numbering is pinned too and a later correction cannot silently move it. Numbering therefore drifts out of sort order during a cycle: an organization that merges after the ratchet engages mints above the rows already published instead of displacing them. That is the trade being made -- displacing a published id is worse than an out-of-order one -- and the release renumber is what fixes ordering. The guard reports consequence, not intent. Deletion being the renumber trigger means the tool cannot distinguish a deliberate renumber from a registry that was lost (stale clone, file never checked out, `git clean`); both mint a plausible numbering and record it as authoritative. So any run that changes an id the tree's existing results.csv already published warns, names the count and three examples, and gives both readings: expected if you removed the file to renumber (refresh the cited ids in the same change), and restorable if you did not. Generalized past the create case so an emptied or truncated registry is caught the same way. Matching the published table has to go through the display columns, since a written results.csv has no workload key in it. Two wrinkles, both pinned by tests: those columns are not unique (two kv_cache performance profiles for one system share all five), so ambiguous keys are excluded from the comparison and counted rather than matched arbitrarily; and `Name` / `Description` are `Hyperlink` objects until write time, so the key is built from `_final_row` output coerced with `str()` -- which is exactly the anchor the file carries. Unhashable-Hyperlink was a real crash before the coercion, caught by the existing pinning tests rather than the new ones. Creation is silent at INFO on a fresh tree: everything is minted by definition there, and a WARNING would make a submitter's first run look like a problem. Minting into an EXISTING registry still warns, because that one does need the file committed. Real-tree acceptance on a hardlink clone of the v3.0 submissions repo: - registry present -> zero files changed, guard silent, 177 ids kept. - registry deleted -> contiguous v3.0-0001..0177, gapless, registry recreated, and the warning naming 172 changed rows. All four CI suites pass (238 / 3141 / 911 / 228). Refs #836
…pt floats
Both fields are declared int(ge=1), so a submitter reporting a fractional
TiB capacity fails validation. XSKY's AIMesh systems report 111.78 TiB
usable, which the schema currently rejects outright.
Adds coverage for the widening:
* fractional values pass (111.78)
* whole numbers still pass — every example_*.yaml uses one
* the ge=1 bound survives, so 0.5 is still rejected on range rather
than incidentally on int-ness
* non-numeric strings still rejected
Renames the two test_non_integer_rejected cases to test_non_numeric_
rejected — they assert on "lots"/"some", which is a numeric-ness check,
not an integer-ness one.
RED: the two fractional cases fail with "Input should be a valid integer,
got a number with a fractional part".
usable_capacity_tib and int_client_store_tib become float. A usable capacity is rarely a whole number of TiB once RAID/EC overhead is taken out — XSKY's AIMesh systems report 111.78 — and rounding a submitter's stated figure to satisfy the schema is not ours to do. * schema_validator.py — int -> float, Optional[int] -> Optional[float] * schema.yaml — int( min=1 ) -> num( min=1 ) in the reference schema * conftest.py / test_reportgen_sut_derived.py — helper annotations ge=1 is deliberately unchanged: the widening is the type only, not the accepted range, so sub-1-TiB values are still rejected. Whole numbers keep validating. The model is lax — StrictModel sets extra="forbid", not strict=True — so an int coerces to float, and every example_*.yaml template still passes. No results-table output changes: report_generator.py reads the raw YAML via solution.get() and never goes through the pydantic model, so an integer capacity still renders as 8192, not 8192.0. Full CI-equivalent sweep green: tests 3146 passed / 1 skipped, mlpstorage_py/tests 911, kv_cache_benchmark 238, vdb_benchmark 228.
…inations at launch Rules.md 4.3.5 is missing the word 'not', which inverted subset-run validation everywhere downstream (#841). Pins the corrected runtime behavior: an explicit --checkpoint-subset CLI flag (4.3.5 sentence 1, never previously implemented) valid only for llama3-8b at 8 processes, a new check_subset_mode fail-fast gate, and check_num_processes no longer accepting 8 processes as a CLOSED form for the large models (70B@8 becomes OPEN-eligible as a TP*PP multiple; 405B@8 and 1T@8 are INVALID).
…binations at launch Corrects the runtime half of the Rules.md 4.3.5 inversion (#841 — a missing 'not' read as endorsing 8-process subset runs of the large models; ten published v3.0 rows launched through that gap). - cli: register --checkpoint-subset on the checkpointing subcommands — the explicit declaration 4.3.5's first sentence requires, whose help text had sat orphaned in common_args.py since it was written. The help text now describes the actual semantics (8B-only, one 8-GPU node, linear-scale-out claim) instead of the inverted large-model reading. - run checker: new check_subset_mode — the flag with any model but llama3-8b, or with a process count other than 8, is INVALID and aborts before DLIO. - run checker: check_num_processes drops the '8 (subset run)' CLOSED allowance the #792 fix codified for every model. CLOSED now requires the respective full count; a large-model run at 8 processes is OPEN-eligible when 8 is a TP*PP multiple (70b) and INVALID otherwise (405b, 1t). 8B is unchanged (its full count is 8). - benchmarks/dlio: auto-labeling a downscaled run now warns at classification time that a partial checkpoint is not a CLOSED form and that subset submissions are 8B-only, so the label never again reads as an opt-in. - test_issue792: the three test_subset_run_passes cases pinned the inverted allowance; updated to the corrected verdicts with a pointer to #841. The validator half (subset_run_validation inversion, 4.6.1 subset carve-out) lands separately.
… the missing 'not' Pins the corrected detection half of #841: subset_run_validation errors on a CLOSED subset run of any model but 8B (via the auto-set checkpoint.mode override OR the new explicit checkpoint_subset arg recorded in metadata), passes the well-formed 8B subset it previously rejected, and leaves OPEN downscaled runs to 4.6.4; closed_mpi_processes loses its 'subset mode: 8 processes for any model' carve-out so CLOSED counts are strict-respective; the reportgen Checkpoint Mode column honors the explicit flag (the 8B claim run is execution-identical to full, so the args snapshot is its only signal).
…s subset carve-out Corrects the detection half of the Rules.md 4.3.5 inversion (#841): - subset_run_validation now errors on a CLOSED subset run of any model but 8B — the previous version implemented the published typo faithfully, erroring 'subset run cannot use 8B model' (the only legitimate form) while passing the large-model subset runs, ten of which published in v3.0. Subset-ness is read from either signal: the auto-set checkpoint.mode override or the explicit checkpoint_subset arg (the 8B claim run is execution-identical to full, so the args snapshot is its only signal). Scope is CLOSED only — an OPEN run below the full count uses the same partial-checkpoint mechanics but is 4.6.4's business. The model regex gains a lookbehind so '405b'/'1t' never match the '8b' token inside their own names. - closed_mpi_processes drops the 'subset mode: 8 processes for any model' carve-out; CLOSED counts are strict-respective (8/64/512/1024). The legitimate 8B subset needs no exception — its respective count is 8. A mislabeled large-model run gets a parenthetical naming the corrected rule reading. - report_generator: the Checkpoint Mode column honors the explicit flag, so a declared 8B subset run publishes as Subset even though no DLIO override is written. - test_bug03 / test_issue792 post-hoc tests pinned the carve-out and the 405b-subset-as-happy-path shapes; updated to the corrected verdicts with pointers to #841.
…et sizes Invalid in Table 2 The published 4.3.5 text read '…or the model is "8B"' where the intent was '…or the model is not "8B"' (#841). Subset mode is defined only for the 8B workload — one industry-standard 8-GPU node checkpointing to solution-managed local storage, claiming linear scale-out — and the larger models have no subset form; they exist to measure architectures where checkpoint data must reach shared central storage. Table 2's 'Subset: 8-Process Size' row carried computed sizes for all four models, which the corrected rule contradicts on its face: a subset size for 70B/405B/1T describes a run the validator must reject. Those cells are now marked Invalid; the 8B value (105 GB, identical to the full run — the whole point) stands.
…'closed' Real metadata carries verification: 'CLOSED' (uppercase); the lowercase-only comparisons meant the new 4.3.5 CLOSED gate skipped every real run — and 4.6.1 has never fired on a real tree at all. Caught by the clone gate: a validate sweep with the new code returned counts byte-identical to the old baseline, zero 4.3.5 lines against the ten known-affected rows.
…al trees for the first time metadata records verification as 'CLOSED'/'OPEN' (uppercase); the 4.3.5 CLOSED gate and 4.6.1's closed-branch compared lowercase-only, so the new detection skipped every real run and 4.6.1 had never fired on a real tree at all. Both now fold case. Deliberately NOT touched: 4.6.2 / 4.6.3 / 4.6.4 carry the same lowercase-only pattern and are equally dead on real trees — enabling them changes real-tree behavior beyond #841's scope and needs its own gated verification; flagged in-code and to the review chairs.
…s-reference Follow-through on #841, per review-chair direction: - 4.3.5 now states the requirement affirmatively — a subset run must use the 8B model and exactly 8 accelerators; the validator must flag any other combination — instead of the double-negative sentence the missing 'not' hid in. The command-side requirement (accept and record the declaration) is unchanged. - The rationale for subset being 8B-only (local-NVMe architectures whose bandwidth scales linearly with node count) appears as an italic aside explicitly marked not part of the rule, and as a one-line footnote explaining Table 2's Invalid entries. - 4.6.1 gains a declarative sentence closing the reading that produced the ten v3.0 rows: no reduced-process form exists for the larger models.
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
2 participants
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.
4th (stacked on #828): fixed webpage-parity schema for
results.{csv,json}Stacked on top of
reportgen-validator-fixes(#828). Part of the reportgen review-tooling stack #826 ← #827 ← #828 ← this. Retarget base tomainonce the lower PRs land.What
reports reportgennow emitsresults.csv/results.jsonas a fixed contract — exactly the reference columns for all 8 v3.0 results-web-page tables (Results Table Structure.xlsx) plus three discriminator columns (Division,Benchmark Type,Model) so one flat file can carry every table's rows. The column set is never data-driven: a MLCommons staff member opensresults.csvin Excel and reduces it to any single web table by deleting the workload blocks + discriminator columns that don't apply.How
Output projection, not an aggregation rewrite. The internal
_aggregate_*helpers (train_mean_of_*/checkpoint_mean_of_*/vdb_*/kvcache_*machine keys, and the empty-metricStatisticsError→ INVALID gate) are untouched, so validity behavior is unchanged. A new_final_row()runs insidewrite_json_file/write_csv_fileand cherry-picks each fixed column from the in-memory row. The dynamic*_mean_of_*per-metric means and the trailingissuescolumn are simply no longer emitted — this is an output-only change with no path from pass to fail.Schema (54 columns)
Public ID,Organization,Division,Benchmark Type,Model,Name,Description,Type,Access Protocol,Availability,RU's,Integrated Client Storage,Usable Capacity (TiB).Training - …,Checkpointing - …,VDB - …,KVCache - …. Shared reference names (# Client Nodes,Code,Read B/W (GiB/s), …) are workload-qualified so every column key is unique.Division= category upper-cased (CLOSED/OPEN);Model= reference display label (llama3-1t→1250B), blank for kvcache/vdb (single-table workloads).Nameshows the system name and links tosystem-description.yaml;DescriptionshowsPDFand links to the.pdf.Code/Logsare per-workload (an OPEN row may ship its own code), routed into the row's own block from the run's code-image pointer.1/2/3map to the reference groupsllama3.1-8b Storage Only/Storage + Mem/llama3.1-70b Storage Only.category/orgname/systemname/benchmark_type/model/accelerator), theissuescolumn, and the now-dead_ordered_fieldnameshelper.Tests
RED-first. Output-file tests migrated to the fixed-schema contract; internal-structure tests (aggregation helper keys) unchanged. All 4 CI suites green:
tests/2975p·1skip,mlpstorage_py/tests898p,vdb_benchmark/tests228p,kv_cache_benchmark/tests238p.