Repository navigation
[X-MANIFEST] Attribute measures to their result so a multi-result report verifies (#517) - #618
Open
physics515 wants to merge 1 commit into
Open
physics515 wants to merge 1 commit into
physics515 wants to merge 1 commit into
Conversation
…ort verifies (#517) A measure was graded by metric name alone, so a report with two results looked like a duplicated measure and finish --manifest refused honest evidence. Measures now carry the result they were quoted from and uniqueness is checked within a result. Signed-off-by: Justin Icenhour <creative515.justin@gmail.com>
This branch has not been deployed
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.
Closes #517.
The defect
RunManifest::verifycollected every measure from every result into one flat list of metric namesand refused the manifest if any name repeated:
latency.p50_nsandthroughput.points_per_secare the names every result grades, so a reportwith two results could never verify: a crash matrix, a query mix, any cross-adapter run. Because
write_torefuses to returnOkfor a manifest that does not verify,finish --manifestexitednon-zero on honest evidence — which is why
#147and#513split each lane and each crash-matrixcell into its own report, and why
#511had to narrowcrash-matrix.yml.The check also broke the file's own error convention: every other
Problemnames the thing to goand look at (
measure \{name}` appears more than once` elsewhere in the same enum), while this onepushed a static string that named nothing.
The fix
A measure's identity is the result it was quoted from, which
BenchResultalready states as(profile, adapter, workload)andcompare.rsalready uses as itsWorkloadKey.Measurecarriednameandadapter, andadapteris the wrong disambiguator: a query mix runs one adapter acrossmany profiles, a cross-adapter run one profile across many adapters.
Measuregainsprofileandworkloadbesideadapter, filled inpush_result_measuresfromthe same result the numbers come from — nothing re-derives a value, per the manifest's own rule.
(profile, adapter, workload), so the same metric may appear onceper result and a repeat inside one result is still a defect.
Problem::DuplicateMeasureis now a struct variant carrying the name and the result identity, andreads:
measure `latency.p50_ns` appears more than once within result `mcar-medium / timescaledb / downsample`; each result must grade each metric name exactly once.MANIFEST_SCHEMA_VERSION23 → 24, because the fields every verifier keys on are new.v23mentions are updated:docs/reference.md's "schema v23" line and the two"schema_version":23fixtures inweft-server/src/bench_runs.rs.A v23 manifest is refused, not converted
The version check stays an exact match, and the message changed from "cannot be verified by a build
that writes v{expected}" to "was written by an older build; re-run the benchmark with a build
that writes v{expected}". A manifest records provenance that only exists during the run — source
revision, dirtiness, environment — so telling someone to regenerate one from its report would invite
fabricated provenance. Accepting a list of versions would make a v23 reader's silence about
profile/workloadinvisible.DECISIONS.mdrecords both rejections (dropping the check; converting old manifests) and one thingthis PR deliberately does not do: remove the one-report-per-cell splitting from
#513/#147. Itwas a workaround for this bug, but it is also what keeps a crash cell's artifact attributable to that
cell, so removing it belongs in its own change.
Tests
a_multi_result_report_verifies_with_cross_result_name_reuse— two results, same metric names,captures and writes and verifies with no defects. Fails on main with the old check.
a_name_repeated_within_one_result_is_still_a_defect— a secondlatency.p50_nsinside one resultis refused by
write_to, and the error names both the metric and the result.a_manifest_of_another_schema_version_is_refused_not_guessednow covers v22 and v23.Verification
cargo +nightly fmt --all -- --checklocally. The build, clippy (#![forbid(clippy::pedantic)]inthis crate), test and docs jobs run in CI on this branch;
weft-xbenchandbench/claims.jsonareuntouched, so no published figure moves.