Repository navigation
H3 Ref2VA lossless follow-up: validate audio coverage and scorer identity - #318
amy-why-3459 wants to merge 4 commits into
Conversation
Agent-assisted-by: Codex Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Dong1017
left a comment
There was a problem hiding this comment.
Scoped static review of this draft's 15-file lossless-validator change against the current base and the #293 acceptance update. The new audio coverage and scorer-identity checks remain preparation, not demonstrated 5s/15s model-quality acceptance. One P2 integration finding concerns a concurrent evidence PR; no tests, device runs, or model evaluations were executed in this review.
| assert result["errors"] == [] | ||
| assert len(result["bundles"]) == 20 | ||
| assert result["checked_hashes"] == 425 | ||
| assert len(result["bundles"]) == 21 |
There was a problem hiding this comment.
P2 — concurrent evidence bundles will invalidate this fixed inventory assertion. This PR registers h3-ref2va-lossless-293 and changes the expected inventory to 21 bundles / 434 checked hashes. Open Draft PR #317 (current head a0677a0) independently registers h3-ref2va-910c-4logical-20261101-297 and changes the same test to 21 bundles / 528 hashes. If both land, the combined inventory has 22 bundles, so either unchanged assertion fails (or the two patches conflict). Please reconcile both bundle entries and recalculate the exact totals after the first integration, or use an increment/per-bundle assertion that still checks both manifests; add a combined-tree check before considering either branch ready. This is a merge-order/CI risk, not evidence that the new media validator fails in isolation.
There was a problem hiding this comment.
Incremental cross-PR update (Nov 2): New Draft #321 (ce031369, refs #320) independently adds h3-ref2va-4x910b1-320 and changes the same tools/evidence_inventory.json / tests/test_evidence_inventory.py assertion to 21 bundles / 431 hashes, on the same base 2baebf6e. This is a third concurrent registration alongside #317 (21 / 528) and this PR #318 (21 / 434), each starting from the base 20 / 425. If all three evidence bundles are retained, the combined inventory contains 23 bundles; checked hashes must be recalculated against that combined tree. The original merge-order/CI finding therefore now covers three PRs. Please coordinate integration and preserve all three manifests; no new verifier-runtime defect is implied.
There was a problem hiding this comment.
Merged current main in a2d7694. The inventory keeps every bundle from main, including kv-actor-qualification-20261031, and keeps h3-ref2va-lossless-293. tools/validate_evidence.py reports 22 bundles and 443 checked hashes for that combined tree. Both sides had written 21, so that equal integer was not the union.
f325d3c addresses this thread: test_checked_in_inventory no longer pins the shared census. It still requires a clean full-tree validation and checks this bundle's two manifests (13 digests). Drafts #317 and #321 are not on main, so their bundles were not added and those PRs were not edited.
There was a problem hiding this comment.
Scoped, non-voting follow-up at head f325d3c807c1e886e1e40cbc47ae16db9ba1eef0: this branch's fixed shared-census problem is addressed.
test_checked_in_inventory still runs the full-tree validator and requires no errors. It no longer pins repository-wide bundle/hash totals; instead it verifies this bundle's two registered manifests, all 13 digests, no unavailable entries for those manifests, and presence in the full inventory. Thus an unrelated valid bundle need not change this assertion.
The hosted run passed both Python 3.10/3.11 full checks and installed-package checks. Each full check reports 3,556 passed, 13 skipped and 284 subtests. The logs identify tested merge 2bc1209eb4de17823ee29be743a21137c14bc313, combining this head with 9dded7adac0ac835f6a73493309399e328a43ab2; PR API base metadata still reports 663f3b327998526a13aa31e72ad729025c18983c.
This clears the fixed-census portion only. The combined tree with open #317 and #321 has not been tested here; integration must preserve their registrations and recheck the resulting tree. I did not run code, replay validators or perform model/device work. No campaign, losslessness, final-combination or whole-PR acceptance is implied.
Keep every evidence bundle registered on main, including kv-actor-qualification-20261031, and keep h3-ref2va-lossless-293. tools/validate_evidence.py reports 22 bundles and 443 checked hashes for the combined tree; the identical per-side total of 21 does not. Agent-assisted-by: Grok 4.7 (conflict resolution) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
discussion_r4440576147: concurrent registrations overwrite the same bundle and hash totals, and equal integers auto-merge to the wrong combined census. Keep full-tree validation, and check the two h3-ref2va-lossless-293 manifests and their 13 digests directly. Agent-assisted-by: Grok 4.7 (review follow-up) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
Refs #293
The requester now requires lossless MiniMax-H3 Ref2VA acceleration with no audio-quality degradation at both 5 and 15 seconds. The accepted scope update excludes the previous lossy Cache-DiT result from this follow-up's acceptance.
This draft prepares the new campaign and repairs two media-validation gaps before candidate promotion. Two equally truncated audio tracks previously passed the historical validator; a synthetic reproduction confirms this. The new version checks expected audio duration, decoded sample count, start/end timestamps and continuity. It authenticates both LPIPS weight files and loads the verified bytes explicitly, without a download fallback. Complete scorer state matches the prior LPIPS 0.1.4 loader on CPU.
Historical evidence and validators remain unchanged. The new validator records exact audio/video equality separately from the inherited perceptual thresholds; unequal outputs that satisfy those thresholds remain INCONCLUSIVE. The old audio cosine gate is not a no-degradation certificate. Baseline repeatability and model-level audio preservation remain to be qualified.
The source-attributed screening plan compares existing AllGather, rank-local streaming and a 16-layer resident prefix with identical BF16 full computation. The first screening attempt completed one 5-second baseline at 400.943 seconds HTTP E2E with all eight workers exiting normally. The rank-local arm then failed before inference because the harness supplied both unified and legacy offload settings. The failure and raw-log digests are retained; no speedup can be inferred. Corrected actual shell argument assembly and the source configuration resolver pass CPU preflight. The resolver reveals that zero and 16 resident layers select different backend strategies, so these are complete configuration comparisons. A fresh matched run is queued. Engine improvements remain in the single vLLM-Omni PR #9698.
Validation: 29 focused CPU tests pass; diff-scoped Ruff passes; evidence inventory checks 438 hashes with no errors; offline synthetic media regression passes, including complete scorer-state equality and rejection of equally truncated tracks. An initial CPU container invocation failed because its UID had no passwd entry; setting the runtime user environment corrected it. No performance sample came from that failed validator invocation.
Remaining: complete lossless screening, freeze a baseline-repeatability adjudication protocol, independently qualify the selected final combination at 5 and 15 seconds, and publish per-optimization E2E benefits and sources. The corrected NPU screening is queued; no new lossless speedup or campaign PASS is claimed. Raw model/media/host logs and weight files remain external. Independent review has not been performed; acceptance stays with the requester.
Knowledge authorship declaration: not applicable to these campaign evidence and validator-test paths.
Agent-assisted-by: Codex