New folder structure - #79
Closed
arav-agarwal2 wants to merge 4 commits into
Closed
arav-agarwal2 wants to merge 4 commits into
arav-agarwal2 wants to merge 4 commits into
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Three fixes flushed out by asking why config.yaml and point.yaml were treated asymmetrically between the run folder and the submission bundle. _extract_model preferred config.yaml over point.yaml while _extract_concurrency right beside it preferred point.yaml. §8.1 names the results directory <model_name>, which §8.2 defines as the supported-model-list name, so the disclosure is authoritative and the optional file is only a fallback. config.yaml carries a HuggingFace path, so the tree got Llama-3_1-8B-Instruct where the spec asks for llama3.1-8b. Reverts a fixture paper-over that set model_id to the HF name to hide the same disagreement. _extract_run_type defaulted to "performance" when nothing declared a run type. That was silently destructive: an accuracy run shipped without a config.yaml would be filed as its concurrency's performance run, collide with the real one, and drop its accuracy results from the bundle. It now reads config.yaml's datasets[].type, falls back to point.yaml's §8.3 dataset_type, and fails the build naming the run when neither answers. config.yaml keeps precedence here — the opposite of _extract_model, and deliberately: dataset_type describes the dataset, so "Accuracy + Performance" cannot say what this run measured. prepare_pr_branch_merge still walked v0.7's pareto/<system>/<model>/ tree and copied a top-level systems/ directory. Against current builder output the walk silently matched nothing and the systems/ copy raised FileNotFoundError, so submissions update, add-run and remove-run all failed once the org directory existed on the PR branch — the normal amendment case. Rewritten against §8.1, preserving reviewer-edited system_desc.json across rebuilds. It had no coverage for the merge body, which is why it survived the migration; now has eight tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#78 moved the summary `runs create` reads to performance/result_summary.json, the path mlcommons/endpoints actually writes, and made accuracy_results.json the artifact truncated before upload. This branch had made point.yaml required and config.yaml optional in the same file. Textual conflicts (parser.py, runs/create.py, test_parser.py) resolved by combining both: the endpoints phase layout from #78, with point.yaml required and config.yaml optional from v1.0. _REQUIRED_FILES = ( "system_desc.json", "point.yaml", "performance/result_summary.json", ) #78's endpoints_run_folder fixture predates point.yaml being required, so it gains one; like system_desc.json it is submitter-authored, not an endpoints artifact. The merge also had a semantic conflict git could not see, because the two sides touched different files: build_submission_folder reads the *uploaded archive* and still looked for result_summary.json flat, so `submissions create` failed on every archive `runs create` now produces. Reproduced, then fixed. The builder reads the endpoints path first and the flat one second — it consumes archives the API already holds, including ones uploaded before the layout settled, whereas `runs create` validates new input and is deliberately strict. Same for accuracy: accuracy/accuracy_results.json, then accuracy/results.json, then accuracy_scores embedded in results.json. Five regression tests cover both layouts, and assert the assembled bundle stays flat per §8.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
As we switched test fixtures, the large-scale test submissions changed. Unfortunately we can't really change how the diff looks long-term. |
Collaborator
Author
|
Superseded by a 3-PR stack — same tree, split for review:
The stack tip is byte-identical to this branch (same tree hash |
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.
Migrates the builder and the checker to the MLPerf Endpoints v1.0 submission format.
Both pipelines move together, because the failure mode this is guarding against is the
two halves disagreeing — a builder writing a bundle its own checker rejects, which is
what issue #72 turned out to be.
Important
This implements against two open
endpoints_policiesPRs. If either changes beforemerge, the corresponding piece here needs revisiting.
feat/remove_runtime_meta.json) — the file layout below.feat/add-1.0-seeds) — the seed values, mirrored intosrc/submission_checker/data/seed_sets.yaml.Everything else follows
v1.0_rules_dev@32f6821.The format change
run_metadata.jsonand the per-systemsystem_desc_id.jsonare both gone; the §8.2system description now lives in every Pareto point.
Region boundaries are now per submission
The largest behavioural change, and the one worth reviewing most closely: §5.5's
algorithm takes
(C_max, C_min), andC_minis derived from the submission's ownpoints rather than declared. The checker previously computed regions before reading a
single point; that control flow had to invert.
_check_modelis now two-phase — parse everypoint.yaml, deriveC_min = min(concurrency),compute boundaries, then run the region-dependent rules. Three calls made explicit in code:
C_minis per(system, model)curve, not per submission — §8.5 defines a result asone system, one benchmark model, one dataset. Otherwise one model's smallest point would
move another model's boundaries.
point.yamlhas no trustworthyconcurrency; including it would shift every boundary and cascade spurious failures onto
the valid points.
region-basisreports WARN on a partial basis, ERROR on none.C_minclamps to 32 when no point is that low, so the rest of the report stays usefulinstead of collapsing; the missing band is reported by
ultra-low-concurrency-coverage.The 10 % margin is its own region now (
high_concurrencyends atC_max), and a marginpoint does not satisfy High Concurrency coverage — §5.4 says the margin "does not
affect the required point distribution". That reading is one constant,
MARGIN_SATISFIES_HIGH_CONCURRENCY, so a WG ruling flips it in one line.Region names follow §5.5/§9.1 (
*_concurrency), not §8.3's stale*_throughputtable.New §9.1 checks
shared-path-resolutionshared_src/shared_docsresolve under the submission root, traversal-freeseed-set-consistencyseed-set-membershipseed-runtime-matchseed-set-adoptiontarget-cohortYYYY-MM-C0/-C1point-disclosure-completewarmup-logs-retainedmetric-consistency-tpot-p90system-description-consistencyregion-basisC_minand how many points it came fromultra-low-concurrency-coverageRetired:
run-metadata-present/run-metadata-valid(file is gone),seed-config(hard-coded 42 — actively wrong once rotation is live; survives as
seed-config-legacyonly for points that declare no seed set), and
low-latency-coverage(vacuous against aderived
low_latencyregion, which is always1–C_min).Metrics:
tps_per_useris now1000 / tpot_p90_ms(wassystem_tps / concurrency),and TTFT reports at P90. With
run_metadata.jsongone,result_summary.jsonis the onlyTPOT source — so §9.1's "the valid per-response TPOT distribution must be non-empty" is
only verifiable as a property of the reported percentile. The rule message says so
rather than implying the samples were audited.
Seeds ship as data
v0.7 fixed every seed at 42. v1.0 rotates them, so the published sets live in
src/submission_checker/data/seed_sets.yamland are overridable with--seed-sets FILEor
$MLPERF_ENDPOINTS_SEED_SETS— a newly published set does not need a checker release.Bug fixes found along the way
submissions update/add-run/remove-runwere broken.prepare_pr_branch_mergestill walked v0.7's
pareto/<system>/<model>/{points,results,accuracy}/and copied atop-level
systems/. Against current builder output the walk silently matched nothing andthe
systems/copy raisedFileNotFoundError— so every amendment failed once the orgdirectory existed on the branch, which is the normal case. Rewritten against §8.1's tree,
preserving reviewer-edited
system_desc.jsonacross rebuilds. It had no test coveragefor the merge body, which is why it survived the migration; now has eight.
tps_utilizationwas normalised per model across systems in the builder while thechecker recomputed it per
(system, model)curve — so no two-system submission couldmatch its own file.
config.yaml. §8.1 names it<model_name>,which is §8.2's supported-model-list name;
config.yamlcarries a HuggingFace path, sothe tree got
Llama-3_1-8B-Instructwhere the spec asks forllama3.1-8b. Now read frompoint.yaml, matching_extract_concurrencyright beside it.Behaviour changes reviewers should weigh
config.yamlis no longer required — in the run folder or the bundle.point.yamlisrequired in both. It is still shipped whenever a run supplies it.
"performance". Defaulting was silently destructive: an accuracy run shipped without a
config.yamlwould be filed as its concurrency's performance run, collide with the realone, and drop its accuracy results from the bundle.
Llama-3_1-8B-Instruct→llama3_1-8b(see above).point-disclosure-completelanded as ERROR, not WARN. Full §9.1 conformance requiresit; easy to downgrade if the first round should land softly.
shared_src/shared_docswhen a point does not declarethem. This is the one exception to Ensure config and point yamls are handled properly #72's "the builder does not derive §8.3 disclosure",
and deliberately so: these keys carry no measurement claim, and their referent —
src/<impl>/, the union of every run'ssrc/— does not exist until the builderassembles the bundle, so no run archive can name it. Same principle already governs
tps_utilization. Validate if present, inject if absent, never overwrite; injectionis append-only so the submitter's comments and key order survive.
Spec defects — judgement calls made here
Each of these is a place the spec contradicts itself; the resolution is recorded at the
code site. Worth a WG confirmation.
regionenum says*_throughput; §5.5 and §9.1 say*_concurrencyshared_src,shared_docs,seed_set,target_cohortrequired by §8.1 prose + §9.1 but deleted from §8.3's table by PR #90publication_status/system_availability_status/availability_status)system_desc_id.jsonreferences survive PR #119 (§8.2 heading, TOC, §8.2.1, §8.5, §9.1)system_desc.json16–26, Med…–116; the algorithm and Appendix B give17–26and…–117seedset.yamlcarries no cohort keys, so §4.6's four-cohort adoption window is unevaluableseed-set-adoptionreports SKIP + INFO; enforces automatically once cohorts appearVerification
ruff,ruff format,mypy --strictoversrc, andsphinx-build -Ware all clean.
test_regions.pyand reproducesexactly. This is the highest-value test here: §5.5 requires submitters to use this
implementation to compute their boundaries, so whatever it asserts is normative for
everyone.
compute_regions(1024, 32)is asserted to reproduce the v0.7 boundaries(
33–42 / 43–131 / 132–1024), which is what keeps the layout-coupled fixtures validacross the signature change.
builder passes the real checker with zero errors, with a second test naming all 15 new
rules individually so a regression says which one broke.
point.yamlandC_mincomes fromthe rest with
region-basisWARN; corrupt them all and it is an ERROR whilepoint-countstill runs and the region-dependent rules are skipped rather than reported against no basis.
Fixture corpus
All 12
test_submissions/trees regenerated bytests/tools/regenerate_fixtures.py,committed and idempotent — a one-shot migration script would be unrunnable the moment
someone hand-edited a fixture, and the next format change would start from scratch. Its
idempotence is asserted, not trusted.
It also cleared stale artifacts already in the corpus (
results_summary.json×10,point_<N>.yaml×9 from the pre-r<N>naming) and gave every tree thesrc/<impl>/README.md§2.2.1 has required all along — those fixtures were failingsrc-dirfor reasons no test asserted, and every test had to look past it.One fixture needed repair:
valid_standardizedis the must-pass tree and stoppedpassing under v1.0 —
C_max=1000with a derivedC_min=16puts Low Concurrency at17–26, and its points were16/38/88/…, so nothing landed in the window. v0.7's fixed33–42caught 38, which is why it was built that way.r38→r20is the minimal fix.Not done
upstream.
MIN_QUERY_COUNTtable; §6.4 gives prose, not numbers.result_summary.json/accuracy_results.jsonhave no field-level schema anywhere in thepolicies repo — ours remains the de-facto contract.