From dc8a6616963f8231b6efba4832a03e596254c17b Mon Sep 17 00:00:00 2001 From: SolitaryThinker Date: Sun, 5 Jul 2026 07:20:00 -0700 Subject: [PATCH] [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. --- docs/contributing/performance_benchmarks.md | 14 +++++++++----- .../tests/performance/compare_baseline.py | 5 +++++ fastvideo/tests/performance/identity.py | 9 +++++++++ fastvideo/tests/performance/test_identity.py | 18 +++++++++++++----- .../performance/test_inference_performance.py | 12 +++++++++++- 5 files changed, 47 insertions(+), 11 deletions(-) diff --git a/docs/contributing/performance_benchmarks.md b/docs/contributing/performance_benchmarks.md index 01476f4d7..a111d0f5f 100644 --- a/docs/contributing/performance_benchmarks.md +++ b/docs/contributing/performance_benchmarks.md @@ -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`) diff --git a/fastvideo/tests/performance/compare_baseline.py b/fastvideo/tests/performance/compare_baseline.py index 2a8502fea..77bfceb2c 100644 --- a/fastvideo/tests/performance/compare_baseline.py +++ b/fastvideo/tests/performance/compare_baseline.py @@ -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 { diff --git a/fastvideo/tests/performance/identity.py b/fastvideo/tests/performance/identity.py index 0cdd55e57..4e643a58b 100644 --- a/fastvideo/tests/performance/identity.py +++ b/fastvideo/tests/performance/identity.py @@ -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)) diff --git a/fastvideo/tests/performance/test_identity.py b/fastvideo/tests/performance/test_identity.py index a4d66bca9..01959c179 100644 --- a/fastvideo/tests/performance/test_identity.py +++ b/fastvideo/tests/performance/test_identity.py @@ -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) diff --git a/fastvideo/tests/performance/test_inference_performance.py b/fastvideo/tests/performance/test_inference_performance.py index 5a1bb409f..c4d43f9d9 100644 --- a/fastvideo/tests/performance/test_inference_performance.py +++ b/fastvideo/tests/performance/test_inference_performance.py @@ -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"),