[bugfix]: address verify-pass findings on the 1546 fix branch
- recipe_fingerprint also excludes attention.resolved_backend (a silent runtime fallback must fail the gate in its cohort, not open a fresh ungated one); requested_backend stays in the hash; regression test extended to both runtime-resolved values. - Legacy v1 configs no longer crash after the GPU measurement: identity fields are emitted only for config_schema_version=2 configs, and the comparator falls back to the legacy (model_id, gpu_type) cohort when a record carries no identity fields (partial identity still raises). - 'recipe' removed from V2_OPTIONAL_METADATA_FIELDS: the generated recipe owns the key now; declaring it in a config fails validation loudly instead of being silently overwritten. - Docs: stale 'until those land' paragraph reconciled with what this branch actually lands; baseline_status documented.
This commit is contained in:
@@ -222,13 +222,17 @@ If a config declares `config_schema_version: 2`, loading fails clearly when any
|
||||
required v2 identity field is missing. If v2 identity or metadata fields are
|
||||
added without `config_schema_version: 2`, loading also fails so partial
|
||||
migrations do not silently run as v1 configs. Optional v2 metadata fields
|
||||
reserved for follow-up work, such as `recipe`, `metric_threshold_policy`, and
|
||||
`quality_metadata`, must be JSON objects when present.
|
||||
reserved for follow-up work, such as `metric_threshold_policy` and
|
||||
`quality_metadata`, must be JSON objects when present. (`recipe` is emitted
|
||||
by the harness and is not config-declarable.)
|
||||
|
||||
Recipe fingerprinting, hardware/software profile IDs, exact-identity
|
||||
comparison, metric-specific threshold policy behavior, promoted baselines, and
|
||||
dashboard regrouping are separate follow-up changes. Until those land, rolling
|
||||
baseline comparison remains keyed by `(model_id, gpu_type)`.
|
||||
comparison, and dashboard cohort grouping land with this change: v2 records
|
||||
compare only within their identity cohort, and a record that opens a NEW
|
||||
cohort is marked `baseline_status: "initialized_new_cohort"` (regression
|
||||
gating starts once that cohort accumulates history). Legacy v1 configs keep
|
||||
the `(model_id, gpu_type)` rolling-baseline comparison. Metric-specific
|
||||
threshold policies and promoted baselines remain separate follow-ups.
|
||||
|
||||
### Raw record (`results/perf_*.json`)
|
||||
|
||||
|
||||
@@ -178,6 +178,11 @@ def _comparison_identity_filters(record: dict[str, Any]) -> dict[str, str]:
|
||||
key for key in COMPARISON_IDENTITY_KEYS
|
||||
if key not in record or record[key] is None or (isinstance(record[key], str) and not record[key].strip())
|
||||
]
|
||||
if len(missing) == len(COMPARISON_IDENTITY_KEYS):
|
||||
# Legacy v1 record: no identity fields at all. Keep the pre-v2
|
||||
# (model_id, gpu_type) rolling-baseline comparison instead of
|
||||
# crashing — v1 configs are documented as loadable.
|
||||
return {}
|
||||
if missing:
|
||||
raise ValueError("Performance record missing required comparison identity fields: " + ", ".join(missing))
|
||||
return {
|
||||
|
||||
@@ -78,6 +78,15 @@ def recipe_fingerprint(recipe: Mapping[str, Any]) -> str:
|
||||
model = pruned.get("model")
|
||||
if isinstance(model, dict):
|
||||
model.pop("resolved_revision", None)
|
||||
attention = pruned.get("attention")
|
||||
if isinstance(attention, dict):
|
||||
# Same reasoning as resolved_revision: with requested_backend="auto",
|
||||
# a silent runtime fallback (e.g. a broken flash-attn wheel) must NOT
|
||||
# open a fresh ungated cohort and seed degraded numbers as the new
|
||||
# baseline — it should fail the regression gate in the old cohort.
|
||||
# Intentional backend changes are declared via requested_backend,
|
||||
# which stays in the hash.
|
||||
attention.pop("resolved_backend", None)
|
||||
return sha256_hexdigest(canonical_json(pruned))
|
||||
|
||||
|
||||
|
||||
@@ -112,16 +112,24 @@ def test_output_path_does_not_change_recipe_fingerprint():
|
||||
assert _fingerprint(cfg) == _fingerprint(with_output_path)
|
||||
|
||||
|
||||
def test_resolved_revision_does_not_change_recipe_fingerprint():
|
||||
# Runtime-resolved snapshot revisions are audit metadata: an upstream
|
||||
# repo commit (even a README-only edit) must not churn the cohort.
|
||||
def test_runtime_resolved_values_do_not_change_recipe_fingerprint():
|
||||
# Runtime-resolved values are audit metadata: an upstream repo commit
|
||||
# (even a README-only edit) or a silent attention-backend fallback must
|
||||
# not churn the cohort — that would open a fresh ungated cohort and let
|
||||
# degraded numbers seed a new baseline. Declared inputs (including
|
||||
# requested_backend) stay in the hash.
|
||||
cfg = _benchmark_config()
|
||||
recipe = build_recipe_from_benchmark_config(
|
||||
cfg, attention_backend="FLASH_ATTN", resolved_model_revision="aaaa1111")
|
||||
cfg, attention_backend="FLASH_ATTN",
|
||||
resolved_attention_backend="FLASH_ATTN",
|
||||
resolved_model_revision="aaaa1111")
|
||||
changed = build_recipe_from_benchmark_config(
|
||||
cfg, attention_backend="FLASH_ATTN", resolved_model_revision="bbbb2222")
|
||||
cfg, attention_backend="FLASH_ATTN",
|
||||
resolved_attention_backend="TORCH_SDPA",
|
||||
resolved_model_revision="bbbb2222")
|
||||
|
||||
assert recipe["model"]["resolved_revision"] == "aaaa1111" # kept for audit
|
||||
assert recipe["attention"]["resolved_backend"] == "FLASH_ATTN" # kept for audit
|
||||
assert recipe_fingerprint(recipe) == recipe_fingerprint(changed)
|
||||
|
||||
|
||||
|
||||
@@ -47,8 +47,11 @@ V2_REQUIRED_IDENTITY_FIELDS = (
|
||||
"variant_id",
|
||||
"benchmark_version",
|
||||
)
|
||||
# "recipe" is no longer config-declarable: the fingerprint follow-up landed,
|
||||
# and the generated recipe document owns that key in emitted records. A config
|
||||
# declaring it now fails validation loudly instead of being silently
|
||||
# overwritten by the generated one.
|
||||
V2_OPTIONAL_METADATA_FIELDS = (
|
||||
"recipe",
|
||||
"metric_threshold_policy",
|
||||
"quality_metadata",
|
||||
)
|
||||
@@ -315,6 +318,13 @@ def _benchmark_identity_fields(cfg):
|
||||
|
||||
|
||||
def _build_identity_fields(cfg, init_kwargs, prompt, runtime_identity):
|
||||
# Legacy v1 configs (no config_schema_version) stay on the legacy
|
||||
# (model_id, gpu_type) cohort: they lack the required identity fields,
|
||||
# and building a recipe for them would raise AFTER the GPU measurement
|
||||
# completed. The validator already enforces that v2 configs carry the
|
||||
# identity fields, so v2 records always get the full identity block.
|
||||
if cfg.get("config_schema_version") is None:
|
||||
return {}
|
||||
recipe = build_recipe_from_benchmark_config(
|
||||
cfg,
|
||||
resolved_attention_backend=runtime_identity.get("resolved_attention_backend"),
|
||||
|
||||
Reference in New Issue
Block a user