3rd: fix(reportgen/validate): four warnings-only review-tooling fixes (C1/C2/P5/P6)#828
Draft
FileSystemGuy wants to merge 9 commits into
Draft
3rd: fix(reportgen/validate): four warnings-only review-tooling fixes (C1/C2/P5/P6)#828FileSystemGuy wants to merge 9 commits into
FileSystemGuy wants to merge 9 commits into
Conversation
'key in parser' raised AttributeError because __contains__ referenced a non-existent self.messages instead of self.d. Latent (callers use []/.get).
…(C1) 'key in parser' now works instead of raising AttributeError. Isolated correctness fix; callers previously used []/.get() so nothing relied on the broken behavior.
_aggregate_workload_metrics is wired (accumulate_results), not 'not yet
wired'; the metrics={} in _model_group_folder is a throwaway proxy, not
the call site. Point the vdb-aggregate ref at VectorDBBenchmark._run_aggregate
(method name, not a brittle line number). Comment-only.
…(C2) Late-stage doctrine: 4.3.4 must not flip a submission INVALID. New tests expect warn_violation + return True when aggregate accelerator memory < checkpoint size. Adds chkpt_summary_num_accelerators conftest knob.
Aggregate accelerator memory shortfall now surfaces via warn_violation and returns True instead of flipping the submission INVALID. The 80 GiB H100 per-accelerator baseline is advisory (checkpointing metadata carries no accelerator-type field), so this must not hard-fail during the submission window. Enriched the message with the accelerator count and a pointer to verify against the actual accelerator's memory.
…t (P6) Two gaps in rules/models.py metric handling: - DLIOResultParser.parse stores summary['metric'] UNFILTERED, so a scalar or string entry can reach report_generator's fmean(metric_list) and raise TypeError; the parse path must filter to list-valued keys like the metadata-complete path does. - Both paths blank metrics silently (metrics=None) when no list-valued keys survive; a diagnostic must be logged so an empty metrics column is traceable to a reason. RED: parse keeps the scalars; the collapse-to-None path logs nothing.
Add module-level _filter_list_metrics(): filters a DLIO summary 'metric' block to list-valued keys (so report_generator's fmean never receives a scalar) and logs a diagnostic both when non-list keys are dropped and when the block collapses to nothing — a blank metrics column is now always traceable to a logged reason. - DLIOResultParser.parse now filters summary['metric'] through the helper instead of storing it unfiltered (was a latent fmean TypeError on the metadata-incomplete fallback path). - ResultFilesExtractor._from_metadata routes through the same helper and gains a logger param, threaded from extract(), so the collapse-to-None case is no longer silent. Warnings-only: no validity change, no new hard-fail.
_aggregate_option_results folds an empty trial's sum([])==0.0 into the across-trial fmean and coerces a fully-empty aggregate to 0.0, so a total data loss (no rank produced any result) is indistinguishable from a real measured zero when reportgen surfaces it verbatim. RED asserts: total loss -> None for all five aggregated fields; an empty trial is skipped rather than diluting the mean (4.0, not 2.0). Also flips test_none_p95_when_no_successful_reads to match its own name (None).
_aggregate_option_results now skips a trial that produced no rank data (guarding the per-trial append) instead of folding its sum([])==0.0 into the across-trial mean, and blanks the aggregated bandwidth/throughput fields to None on total loss instead of coercing to 0.0. A present-rank 0.0 (CPU-tier working set) is still a legitimate measured value and is counted. reportgen passes these through verbatim, so a blank cell now means no data rather than a measured zero. Warnings-only: no validity change; total loss was already logged per-rank.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
FileSystemGuy
changed the title
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
1 participant
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
Third branch in the review-tooling stack. Four low-blast-radius correctness fixes to
reportgen/validate, applied so they can be used during the v3.0 submission-review window while submitters stay on v3.0-rc1. Every change is warnings-at-most — none introduces a new hard-fail and none can flip a submissionpass → fail.Stacked on top of:
reportgen-v3-final-results-tables)validate-flaw-warnings) ← base of this PRReview this PR's diff against
validate-flaw-warnings; it isolates just the commits below.Changes
JSONParser.__contains__self.messages(nonexistent) →AttributeError. Now usesself.d. Was latent because callers reach[]/.get, neverin.warn_violation(with the accelerator count + a pointer to verify against the real accelerator memory) and returnsTrue.DLIOResultParser.parsestoredsummary['metric']unfiltered, so a scalar/string entry could reachreport_generator'sfmean(metric_list)and raiseTypeError. New_filter_list_metrics()filters to list-valued keys on both parse paths and logs a diagnostic when keys are dropped or the block collapses to nothing — a blank metrics column is now traceable to a logged reason instead of vanishing silently._aggregate_option_resultsfolded an empty trial'ssum([]) == 0.0into the across-trial mean and coerced a fully-empty aggregate to0.0, making a total data loss indistinguishable from a real measured zero. It now skips no-data trials and emitsNone(blank) on total loss. A present-rank0.0(CPU-tier working set) is a legitimate measured value and is still counted.Testing
TDD RED→GREEN for each item. Full CI-mirror sweep green after every commit:
tests/— 2970 passed, 1 skippedmlpstorage_py/tests— 898 passedvdb_benchmark/tests— 228 passedkv_cache_benchmark/tests— 238 passedNotes
mainonce 1st: feat(reportgen): populate v3.0 final results tables (all 8 tables) #826 and 2nd: feat(validate): warnings-only submission-flaw checks — 3.3.7 node capability, 4.7.4 simultaneous R/W #827 land.results.{csv,json}exactly the webpage columns) and a realLogshref target — both tracked as follow-ups.