2/3 Migrate the builder and checker to MLPerf Endpoints v1.0 - #81
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
|
I'm trying out gh PR stacking - can you take a look at just this and see what you think? The hope is this should allow you us to separate changes with docs changes. |
9de5359 to
2a30804
Compare
|
Force-pushed — please re-pull before reviewing. Two commits now:
You were right to ask about I had grepped, found those call sites, and taken them for live code. So the earlier revision of this PR claimed the rewrite fixed Test count drops 863 → 825: 33 from |
|
Force-pushed again — third commit added: Remove post-submission run additions. You were right about the rules.
Everything under §8 shifted up one; the TOC still lists the removed section. CLI changes:
Sharp edge now documented: withdrawn points don't count toward the 7-point minimum, and that shortfall can no longer be repaired by adding another point. The checker reports it. This is on Also fixed stale help text I left in the previous commit — Tests 825 → 824. |
36d0b5e to
5b83dcc
Compare
Both pipelines move together, because the failure this guards against is the two halves disagreeing — a builder writing a bundle its own checker rejects, which is what issue #72 turned out to be. A test now builds a compliant 7-point curve and runs the real checker over it, so that class of bug cannot reopen silently. Implements against two OPEN endpoints_policies PRs: #119 (feat/remove_runtime_meta.json) for the file layout and #117 (feat/add-1.0-seeds) for the seed values. Everything else follows v1.0_rules_dev @ 32f6821. If either changes before merge, the corresponding piece here needs revisiting. Region boundaries are now per submission. §5.5's algorithm takes (C_max, C_min), and C_min is derived from the submission's own points rather than declared, so the checker — which computed regions before reading a single point — had to invert its control flow. _check_model is two-phase: parse every point.yaml, derive C_min, compute boundaries, then run the region-dependent rules. Three calls are made explicit in the code. C_min is per (system, model) curve, since §8.5 defines a result as one system, one model, one dataset; otherwise one model's smallest point would move another's boundaries. Only parseable points contribute, because a corrupt point.yaml has no trustworthy concurrency and including it would shift every boundary and cascade spurious failures onto the valid points — region-basis reports WARN on a partial basis and ERROR on none. C_min clamps to 32 when no point is that low, so the rest of the report stays useful instead of collapsing to one error. The 10% margin is its own region and 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 working-group ruling flips it in a line. Region names follow §5.5 and §9.1 (*_concurrency), not §8.3's stale *_throughput table. New §9.1 checks: shared-path-resolution, the four seed-set rules, target-cohort, point-disclosure-complete, warmup-logs-retained, metric-consistency-tpot-p90, system-description-consistency, region-basis, ultra-low-concurrency-coverage. Retired: run-metadata-present/-valid (the file is gone), seed-config (hard-coded 42, actively wrong once rotation is live — survives as seed-config-legacy for points declaring no seed set), and low-latency-coverage (vacuous against a derived low_latency region, which is always 1-C_min). tps_per_user is now 1000 / tpot_p90_ms. Seeds ship as data (data/seed_sets.yaml) with --seed-sets and $MLPERF_ENDPOINTS_SEED_SETS overrides, so a newly published set does not need a checker release. Two pre-existing bugs surfaced along the way. The builder normalised tps_utilization per model across systems while the checker recomputed it per curve, so no two-system submission could match its own file. And the results directory was named from config.yaml, which carries a HuggingFace path, where §8.1 asks for §8.2's supported-model-list name. Behaviour changes worth a reviewer's attention are listed in the PR description, the largest being that config.yaml is no longer required anywhere and a run whose type cannot be determined now fails the build rather than defaulting to "performance" — defaulting silently dropped accuracy results. Appendix B's full 14-row table is parametrised in test_regions.py and reproduces exactly. §5.5 requires submitters to use this implementation to compute their boundaries, so whatever it asserts is normative for everyone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
submissions/github.py has been dead code since e722875 ("Temporarily remove github usage while testing", 2026-05-29) — 143 commits and three months ago. Every call site in update/add_run/remove_run/withdraw sits inside a triple-quoted string block, and submissions create dropped PR creation entirely, so an AST walk over src/ finds zero live imports and zero live calls to it. Grep does find those call sites, which is the trap: they read like live code and cost real time to trace. I fixed a stale tree walk in prepare_pr_branch_merge earlier in this branch and wrote eight tests for it before checking whether anything reached it. Nothing did. Removes github.py (357 lines), its tests (368), the ~140 lines of commented-out call sites, and GitHubError, whose only raiser was the deleted module. Five tests whose whole subject was the PR flow go with it; the rest kept their assertions and lost only the vestigial mocks — patches of a module that no longer exists. Also drops the leftover comments in create.py that described the PR step it no longer performs. pr_url and pr_number stay on the submission record and in the formatters: the fields are still populated by whatever opens the PR now, and the CLI should keep displaying them. Recoverable from history if PR creation ever returns to the CLI, and a reviving change would want to start from §8.1's tree rather than the v0.7 one this had drifted to anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
endpoints_policies 7fd3e89 ("Remove post-submission pareto run additions.",
2026-09-02) deleted Submission Rules §8.1 Pareto Updates, which had allowed:
Submitters may add additional measurement points to their pareto curve
during a post-submission update window of 90 days from the initial
submission date.
With that section gone, a submission's points are fixed at creation. The CLI still
offered two ways to add them.
Deletes `submissions add-run`, which implemented exactly that window. Guards
`submissions update --run-ids`, which reaches the same outcome by rebuilding
against an arbitrary run list: a list that would add a run is now refused before
anything is fetched or patched, so a mistaken invocation cannot leave the
submission half-updated. A list that only drops runs still goes through.
`remove-run` stays. §8.1 Corrections still lets a submitter withdraw a faulty
measurement point during peer review, and its docstring now records the sharp edge
that follows from this change: withdrawn points do not count toward the 7-point
minimum, and that can no longer be repaired by adding another. The Submission
Checker reports the shortfall.
api.add_run_to_submission also stays, used only to undo a failed remove-run by
restoring a run the submission already had. That is the undo half of an incomplete
operation rather than a §8 addition, and both the function and the rollback helper
now say so, since the next reader will otherwise wonder why an add call survives a
commit that removes adding.
Also fixes user-facing help text left stale by the previous commit: remove-run,
withdraw and update all still advertised GitHub PR steps — "Check GitHub
prerequisites", "close its PR", "PR update" — that the deleted module used to
perform. `--help` was describing work the CLI does not do.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An unscoped `git add -A` while restructuring this branch committed three untracked scratch files from the repository root — policies_notes.txt, scratchpad-path.txt, and scanoss-key.txt — into the migration commit. The same command also swept in the untracked mutants/ tree, which the pre-commit hook caught; the .txt files it did not. The files have been removed from this branch's history. This makes the mistake unrepeatable: /*.txt ignores root-level scratch without touching the .txt files under test fixtures or docs, and scanoss-key* covers the credential by name wherever it lands. Anything matching these belongs outside the repository or in the scratchpad. A requirements*.txt exception is carved out in case one is ever added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5b83dcc to
1e12f26
Compare
Stack 2 of 3 ·
main← [#80 1-fixtures] ← 2-checker ← [3-docs]· This is the PR to review. The 439-file generated fixture diff is in #80.
Note
Force-pushed twice since the first review request — three commits now, not one.
submissions/github.pyturned out to be unreachable, so rather than have this PRrewrite it and a later one delete it, the migration commit no longer touches it and
the deletion is its own commit. A third commit removes
submissions add-run:endpoints_policies7fd3e89deleted Submission Rules §8.1 Pareto Updates, so points may no longer be added
after submission. The v1.0 migration itself is unchanged in substance.
Migrates the builder and the checker to the MLPerf Endpoints v1.0 format. 863 tests
pass;
ruff,ruff format,mypy --strictoversrc, andsphinx-build -Ware clean.Both pipelines move together, because the failure this guards against is the two halves
disagreeing — a builder writing a bundle its own checker rejects, which is what issue #72
turned out to be. A test now builds a compliant 7-point curve and runs the real checker
over it, so that class of bug cannot reopen silently.
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.feat/add-1.0-seeds) — the seed values, mirrored intodata/seed_sets.yaml.Everything else follows
v1.0_rules_dev@32f6821.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 own points ratherthan declared. The checker previously computed regions before reading a single point; that
control flow had to invert.
_check_modelis 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/-valid(file is gone),seed-config(hard-coded 42 —actively wrong once rotation is live; survives as
seed-config-legacyonly for points thatdeclare no seed set), and
low-latency-coverage(vacuous against a derivedlow_latencyregion, which is always
1–C_min).Metrics:
tps_per_useris now1000 / tpot_p90_ms(wassystem_tps / concurrency), andTTFT reports at P90. With
run_metadata.jsongone,result_summary.jsonis the only TPOTsource — 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.
data/seed_sets.yaml, overridable with--seed-sets FILEor$MLPERF_ENDPOINTS_SEED_SETS, so a newly published set does not need a checker release.Two pre-existing bugs surfaced along the way
tps_utilizationwas normalised per model across systems in the builder while thechecker recomputed it per
(system, model)curve — so no two-system submission could matchits own file.
config.yaml. §8.1 names it<model_name>, whichis §8.2's supported-model-list name;
config.yamlcarries a HuggingFace path, so the treegot
Llama-3_1-8B-Instructwhere the spec asks forllama3.1-8b.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.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 declare them.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 builder assemblesthe bundle, so no run archive can name it. Same principle already governs
tps_utilization. Validate if present, inject if absent, never overwrite; injection isappend-only, so the submitter's comments and key order survive.
Spec defects — judgement calls made here
Each 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
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 valid acrossthe 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.
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.