Skip to content

bench: preserve complete serving evidence and offline summaries - #131

Merged
MrDongsls merged 8 commits into
InferMatrixLab:mainfrom
linear3735:codex/decision-latency-evidence
Oct 23, 2026
Merged

MrDongsls merged 8 commits into
InferMatrixLab:mainfrom
linear3735:codex/decision-latency-evidence

Conversation

@linear3735

@linear3735 linear3735 commented Oct 10, 2026 •

Copy link
Copy Markdown

Required human contributor answers

The contributor explicitly confirmed the following four answers and the PR-necessity decision on 2026-10-23.

  1. What outcome are you trying to achieve?

    Answer: Make System1-Omni serving benchmark results traceable and reproducible offline, accounting for planned, attempted, successful, failed and unattempted requests, interruption state, and the boundaries between startup, warmup and measured requests.

  2. What is the highest-leverage variable for that outcome?

    Answer: The completeness of request-level evidence and consistency between online collection and offline validation.

  3. How will this work move that variable rather than create busywork?

    Answer: Extend the existing benchmark runner to preserve response body bytes, validate UTF-8/JSON strictly, bind protocol and validation versions, verify hashes and completion state, retain failed-request denominators, and add regressions for observed misclassification. The new byte evidence is HTTPX's body after HTTP content decoding, not compressed wire bytes or HTTP framing.

  4. Who is the customer or user, and what do they actually need?

    Answer: System1-Omni benchmark operators, maintainers and result reviewers. They need to verify complete request counts, failure reasons and measurement-clock boundaries, and recompute results from saved evidence without invoking the model again.

After answering the four questions: is a PR needed at all to achieve this outcome?

Human decision and rationale: Yes. This outcome depends on the repository's shared collector, archive format and offline validator; an upstream PR lets contributors and reviewers use one evidence contract. The scope is benchmark tooling, tests and documentation, with no claim of new inference speedups or GPU quality qualification.

Scope and need references: #130 and the maintainer's integration handoff.

For a bug fix: link actual observed failure evidence.

Evidence: psv666's observed UTF-8 reproduction shows that the old collector accepted a response containing an invalid UTF-8 byte (0xff) as a successful answer and lost that byte through replacement decoding. The current ef13e0c repair preserves body bytes and applies the same strict parser online and offline; the before/after regression results are recorded below. No private raw GPU archives are published by this change.

Purpose

Closes #130. Preserve all planned requests, raw responses and interruption state in the HTTP benchmark, recompute summaries offline, and document the latency leaderboard. Accuracy is maintained in System1-Agents.

The collector now retains HTTPX body bytes losslessly and rejects invalid UTF-8 before JSON/schema validation. Previously, response.text could replace invalid bytes with U+FFFD, accept an otherwise valid answer, and lose the original bytes. Online collection and offline replay now share the strict parser and wire adapter:

HTTPX response.content -> base64 + byte SHA256 -> strict UTF-8 / JSON -> typed answers
saved bytes           -> hash / display checks -> same parser and wire adapter

New runs bind response_format=httpx-content-base64-utf8-v1 in the checksummed config; the response text is display-only. Supported old archives without this field, or with httpx-text-v1, keep their saved-text/hash semantics and are never rewritten or silently reclassified. The new bytes are HTTPX's body after HTTP content decoding, not compressed wire bytes or HTTP framing.

This branch merges main f5ae5ac and preserves the now-merged #93 Score contract (2/3/4dp, independent rounding bounds, fsum, zero relative tolerance, exact level bounds and strict zero-fit metadata), together with #131's versioned validation/legacy semantics, typed JEV adapter, input preflight, completion/interruption denominators and checksums. Main's executable bit and Decider two-decimal guidance are retained; the duplicated CLI precision option introduced by auto-merge was removed. The measurement-clock boundaries and model/serving implementation are unchanged.

The separately owned #141 weekly completeness guard remains handed off there, not copied into this PR. #141's other resource/preflight/portability/numerical findings remain separate; its older composition result below is historical, not a claim that this PR closes every #141 issue.

Test Plan

System1-Omni Version / Commit: head ef13e0ccd144409304f256d91db7bf509bc31f7c, tree eb17ba391ff142e4eb72cfc11689ba47473edf6d. Current target main and full-PR merge-base: f5ae5acf65e2d68150aef944aae00f4dffc0da27; local integration merge a250a7d653b3f1660daa02bdd5b81d7340d61c58. Full-PR diff SHA-256: 60d3e7a200cd0352383cbff9bf14ff21b0ab2c4fdb5ed25ea60dd68e9898728d, recomputed with git diff --no-ext-diff --no-textconv --no-color f5ae5acf65e2d68150aef944aae00f4dffc0da27...ef13e0ccd144409304f256d91db7bf509bc31f7c | shasum -a 256.

The UTF-8 increment a250a7d..ef13e0c changes 5 files (+320/-20); patch SHA-256 14ec06816d6dcf4a7def9e6da9fdf5358e4acc1056a1e7cc9d34bab291ad190b. git merge-tree --write-tree f5ae5acf65e2d68150aef944aae00f4dffc0da27 ef13e0ccd144409304f256d91db7bf509bc31f7c exits 0 and produces exactly the candidate tree. The code/patch hashes stayed unchanged throughout both fresh reviews and the contributor's subsequent approval.

python -m unittest discover -s tests/benchmarks -p 'test_*.py' -v
python -m unittest discover -s tests/decider -p 'test_*.py' -v
python benchmarks/bench.py validate benchmarks/smoke.jsonl
python -m ruff check --isolated benchmarks/bench.py tests/benchmarks/test_bench.py tests/benchmarks/test_saved_protocols.py tests/benchmarks/test_response_bytes.py
python -m ruff format --isolated --check benchmarks/bench.py tests/benchmarks/test_response_bytes.py
python -m mkdocs build --strict

Test Result

Current head ef13e0c — UTF-8 repair and main integration

  • Before: a temporary archive of the actual integration commit a250a7d, with the new regression tests copied in, accepts invalid UTF-8 in an unused field as success under both protocols; three unknown response-format values are not rejected. The duplicate CLI option was already fixed in that baseline, so it does not confound these failures.
  • After: Python 3.11.15 / HTTPX 0.28.1: 103/103 benchmark tests and 16/16 Decider helper tests pass, zero skipped. This is the complete suite on the final merged tree, including main's existing tests. Four-request smoke, affected-file Ruff 0.15.2 isolated checks, Python compilation, production/new-test formatting, whitespace checks and strict MkDocs build pass. Existing whole-file formatting in test_bench.py was not expanded into unrelated cleanup; inherited main EOF whitespace notices are distinct from the clean final PR diff.
  • Eight new byte-evidence tests cover System One/JEV-VL online -> saved -> offline/CLI, Unicode and conflicting HTTP charset, empty vs missing body, BOM, 600/999, base64/hash/display tampering after outer re-signing, unknown formats, old saved-text behavior and failed warmup. Existing deep-JSON, typed/schema, numerical precision/zero-fit, interruption/torn-snapshot, completeness and latency tests remain.
  • Fresh independent Codex: PASS, Standards/Spec zero findings; independently reran 103+16, smoke and strict docs. Its 16 extra CPU probes cover four invalid UTF-8 forms in both protocols, valid U+FFFD/Unicode, gzip content-decoded byte preservation, actual old-collector negative cases and unchanged legacy replay.
  • Fresh native Kimi: PASS, no blocking findings; a new session with tools/MCP disabled, static review only, no test execution or independent hash computation. The caller verified matching source/patch bindings before and after; this is not claimed as execution by Kimi. Two nonblocking diagnostic/test observations needed no code changes.
  • Contributor-supplied separate independent recheck: reported PASS after its own patch/hash, main merge-tree, blob/mode/scope recomputation, full bench diff read and 103+16 rerun. That reviewer explicitly adopted Codex's real-old-archive probes rather than claiming to rerun them. This is separate from the implementation's recorded local checks, the fresh Codex execution and Kimi's static review; it is not maintainer approval.
  • No new GPU/model/Slurm run, historical leaderboard remeasurement or Rust/native/CUDA build was performed locally for this client-only change. Inherited main GPU/Rust code is not new authored code here. GitHub exact-head workflow results are tracked separately below.

Exact-head GitHub Actions

Both CI run 39910338895 and Docs run 39910338894 completed successfully. The API head_sha of each is exactly ef13e0ccd144409304f256d91db7bf509bc31f7c. Current checks: rust, benchmarks, cuda-contract and build SUCCESS; deploy SKIPPED. These are new-head results, separate from the old green runs below; skipped checks do not establish GPU/runtime qualification.

Historical validation through 82c108e (older source / composition identities)

Preflight-fix validation at 82c108e (2026-10-21)

  • Change (review P3-1): run() now applies the wire adapter to every case — main manifest and warmup manifest — before creating the output directory. An adapter-incompatible case is rejected up front with a ValueError, so no archive directory is created for a run that cannot execute any HTTP exchange, and a never-created archive can no longer be summarized as zero results.
  • Local: Python 3.14.6 with benchmarks/requirements.txt installed: 44/45 tests passed. The single failure is the pre-existing deep-JSON environment difference in test_run_saves_deep_json_and_nonstandard_http_failures (Python 3.14's JSON parser no longer recurses, so the probe no longer raises RecursionError); it is unrelated to this patch. The two new preflight regression tests pass. The same suite passed 43/43 on the reviewer's Python 3.12.8 before these two tests were added.
  • Exact-head GitHub Actions: CI run 39622915902: rust, benchmarks and cuda-contract SUCCESS; Docs run 39622915336: build SUCCESS, deploy skipped. This commit was implemented by an agent under the contributor's explicit fix-and-push instruction and has not yet received a fresh independent review. No GPU/model measurements; benchmark-client only.

Historical integration-contract validation at 8e42b4b2

  • Before: the original collector did not bind offline validation to saved precision/protocol/version; the old weekly aggregate accepted summaries without explicit complete evidence. In a temporary copy of the final collector plus only the original test: add weekly local GPU accuracy and latency checks #141 aggregate, all four invalid completeness flags (missing, false, string true, numeric 1) produced the expected failing assertions. The final source was not reverted.
  • After: Python 3.11.15 / HTTPX 0.28.1: 109/109 tests passed, zero skipped, on the exact local composition identified above. This includes both protocols, 2/3/4dp Score settings, strict types, warmup, CLI recomputation, raw hashes, interrupted/torn snapshots, deep JSON, HTTP 600/999 and CPU loopback weekly service lifecycle/cleanup. This is a composition result, not a claim that all 109 tests live on the bench: preserve complete serving evidence and offline summaries #131-only branch. The bench: preserve complete serving evidence and offline summaries #131 collector's own 43 tests also passed during local validation.
  • Fresh independent Codex: PASS on the frozen collector + companion and their integration interfaces; independently reran all 109 tests and compared the legacy reader with the old implementation in 4,320 deterministic cases, zero mismatches.
  • Fresh Kimi: PASS, no P0/P1/P2, separate new native session with tools disabled; static review only, no tests executed and no independent fetch/hash verification of the bench: validate rounded Score and isolated zero-fit results #93/test: add weekly local GPU accuracy and latency checks #141 source branches. Three nonblocking diagnostic/numerical-domain suggestions were recorded; they do not change the frozen patch. Both review inputs were bound to the same exact patch hashes and final source bytes.
  • Affected-file E4/E7/E9/F checks, compilation, manifest validation and diff whitespace checks passed; default Ruff passed on test_saved_protocols.py, test_score_wire.py and the companion test_weekly.py. bench.py passes whole-file formatting. The existing test_bench.py whole-file formatting/default-lint diagnostics and six existing weekly default Ruff diagnostics were not expanded into unrelated cleanup; the weekly diagnostics match exact test: add weekly local GPU accuracy and latency checks #141 baseline. This is not a claim of whole-repository lint cleanliness.
  • The local composition retained complete test: add weekly local GPU accuracy and latency checks #141 dependencies, including its fixture/data/service files. No real GPU/model/Slurm experiment or new quality/latency measurement was run. CPU loopback values demonstrate the benchmark tool's behavior only. No Rust/native source changed; those checks remain delegated to GitHub CI.
  • Exact-head GitHub Actions: CI run 39442536298 and Docs run 39442536269 both completed successfully at 8e42b4b298caeaadea6519bd7bb123c08993ece6: rust, benchmarks, cuda-contract and build passed; deploy was skipped. Both Actions run head_sha values were verified via the API. Previous green runs below belong to older commits.

Historical validation (not substitutes for the current-head checks)

Previously at 7ac2b11b, Python 3.12.13 gave 25 passing branch tests and 40 passing benchmark tests in the local merge with then-main. Smoke validation, Ruff 0.15.2 default rules and the strict MkDocs build passed. Default Ruff reports zero diagnostics in 0.15.2 and nine in 0.16.8/0.16.10; the nine diagnostics are on unchanged lines. The earlier report of nine diagnostics was correct for those newer versions.

The hard-kill regression constructs an inconsistent saved snapshot; it is not a real SIGKILL durability test. No new GPU measurements or Rust validation were run for this client-only repair. These checks are local; GitHub Actions for this revision are reported separately in the PR checks.

Independent Codex source review passed. Kimi accepted both P3 resolutions and
the numeric-overflow fix, reran 25 tests, and independently recomputed all six
historical rounds. Its remaining request was the collector-version disclosure
now included above. Those reviews cover the pre-boundary-fix code; reviews of the new patch are recorded separately below.

Docs-only commit 0a55fce06 ("docs: clarify pooled latency and dataset hash provenance", 2026-10-12) addresses the two pooled-percentile/dataset-hash documentation questions raised in review. At 0a55fce06, source and tests were unchanged since 7ac2b11b; those 25-test results are historical, not validation of the new boundary fix. The earlier 40-test integration result used main 7cbce19f117f23214c8acd8d874c2ab5f10c683e; separately, on 2026-10-13, all 40 benchmark tests passed on an export of the clean merge tree of head 0a55fce06 and main 3e9cc33a61ddf110d97512fb6f296cf540377c61, using Python 3.14.6 and HTTPX 0.28.1. GitHub Actions workflow runs CI and Docs both ran at 0a55fce06 and completed successfully. No GPU measurements were added.

Boundary-fix validation at 36c244b6

  • Same new regression tests on original head 0a55fce06: three expected errors—RecursionError for deep JSON, and ValueError: invalid HTTP status for 600/999. On this commit all cases pass; bool/string/99/1000 archived statuses remain rejected. Regression tests.
  • Python 3.11.15 / HTTPX 0.28.1: 27 tests passed, zero skipped. The contributor separately reported 27/27 on Python 3.12.13.
  • The historical integration of head 36c244b6bc8715af8a1958f5d55a6acd35c98edd and then-main 3e9cc33a61ddf110d97512fb6f296cf540377c61 produced tree 91bd568d5a9f6903002fa923812d971bd6db4fbd. It was equivalent to the previously exported 13c9decf tree plus the two-file boundary-fix patch, whose bytes matched. On that historical tree, Python 3.12.13 / HTTPX 0.28.1: 42 tests passed, zero skipped.
  • Ruff 0.15.2 isolated/no-cache, four-request smoke validation and git diff --check passed. An unchanged local archive from the real loopback HTTP 600 reproduction now recomputes as one completed failure without rewriting its evidence.
  • Fresh Codex review: PASS; independently reran 27 tests and reproduced the original three errors.
  • Kimi r2 review: PASS; separate fresh static review of patch SHA-256 016fcdc9bacbd3fb193e59b359721bf00d2efe4e51a724e5f87f7c90a3d356e2, with tools disabled. Its first attempt timed out; r2 completed successfully. It did not run tests.
  • Contributor-supplied second independent review: PASS; reported separate original-error reproduction, 27/27 branch tests, 42/42 integration tests, Ruff and diff hygiene. This record is distinct from Kimi r2.

The historical boundary-fix commit 36c244b6 changed two files (+89/-3), including three source-line replacements and 86 test lines. No new GPU/model runs or performance measurements were added; Rust validation was left to GitHub CI. Historical repeated-run data and the original collector requirement above remain unchanged. GitHub Actions for this exact head completed successfully: CI and Docs. The deploy job was skipped. These workflow results are separate from the local checks.

Historical archive reproduction

The six historical rounds were independently recomputed with the original
collector at 4b9cccbde841272ff5ace6aee3cf51610fe1324f: all exited 0 and matched
the reported p50/p95 values. To reproduce those summaries from an Omni checkout:

git show 4b9cccbde841272ff5ace6aee3cf51610fe1324f:benchmarks/bench.py > /tmp/omni-bench-4b9cccb.py
python /tmp/omni-bench-4b9cccb.py summary <saved-round-directory>

The revised python benchmarks/bench.py summary <saved-round-directory> applies
only to supported checksummed archives, using the saved contract or the documented legacy defaults. It
rejects these six historical rounds because they lack warmup_sha256; no checksums
were added to their saved evidence.

For pooled figures, validate the three rounds with the original collector, then
combine their successful measured latency_ms samples without deduplicating tasks.
Use statistics.median(x) for p50 and sorted x[math.ceil(0.95 * len(x)) - 1]
for nearest-rank p95; exclude warmups and failures, and report counts separately.
Do not average per-round percentiles. Pool only matching deployment, manifest and
timing/collection settings; an empty pool has null percentiles.

Demo / evidence

The maintained benchmarks/latency-leaderboard.md records the five-model campaign 415076 and its comparison rules. Its body-receipt clock and interpolated p95 remain separate from campaign 416135 below, whose clock includes body decoding and JSON/schema validation.

Deployment Successful / planned Pooled p50 / p95 ms
Open-Jev-9B 693 / 693 43.082 / 326.968
Open-Jev-27B-v1.1 693 / 693 69.459 / 1066.037

These are historical single-H800 observations at concurrency 1: 231 unique tasks, three rounds and five client warmups per round, with no measured failures. They are not same-model speedup or parity results. Image/VL, cache sequences and higher concurrency were not measured here.

The full PR at ef13e0c changes 653 production Python, 1,313 test and 259 documentation lines (additions plus deletions): 1,966 authored-code lines, 2,225 total across 6 files (+2,148/-77). The inherited main changes are not new authored Rust/CUDA implementation in this PR. Video: N/A for this HTTP accounting/validation change.

Self-review

The contributor approved the exact frozen ef13e0c diff after supplying a separate independent recheck. Fresh Codex/Kimi reviews cover the current merged tree and UTF-8 change. The checked items record assisted review and evidence checks; they do not establish maintainer approval. The contributor explicitly confirmed all four human contributor-intent answers and the PR-necessity decision on 2026-10-23; those answers are recorded above. Raw archive publication still requires separate authorization.

  • Independent Codex and Kimi reviews covered the diff; identified issues were addressed.
  • Architecture and scope were checked; changes are confined to benchmark tooling, tests and documentation.
  • Applicable checks were run; commands, results and unverified checks are reported above.
  • Claims and provenance were checked, including the original collector required for the six historical rounds.

Merge Danger

Door: two-way. Reverting the benchmark client/reader does not rewrite saved historical evidence. Do not add byte fields or rewrite old configs/hashes to claim a different collection contract.

Blast Radius: benchmarks. New response storage is explicitly versioned; the numeric validation version and legacy behavior remain separate. The human contributor-intent answers and PR-necessity decision are confirmed above. Applicable public-evidence requirements and maintainer approval remain separate readiness items.

Retain planned, attempted, failed and incomplete requests and recompute saved rounds without inference.

Changed lines: 886 authored code/test lines and 100 documentation lines (986 total additions + deletions). No third-party source copied.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review: PR #131 "bench: preserve complete serving evidence and offline summaries"

  • Base: origin/main @ 4a79980d8a75fd063cb3f8247e06215e288b18ac (merge-base equals base; PR is current with main, no drift)
  • Head: refs/remotes/remote-pr/131 @ 4b9cccbde841272ff5ace6aee3cf51610fe1324f, branch codex/decision-latency-evidence, author linear3735, OPEN/DRAFT. Remote head rechecked via git ls-remote immediately before this report: unchanged at 4b9cccbd.
  • Review provenance: skill .agents/skills/system1-omni-review/SKILL.md read from the local repo working tree (HEAD ca5f69b5c104ff2ed0c64ed2f195b49bec92e32c, which is ahead of origin/main); SHA-256 of bytes actually read = 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108, matching the pinned value for main@4a79980. References read and verified byte-identical (SHA-256) between main@4a79980 and the working tree: docs/architecture.md, CONTRIBUTING.md, .agents/skills/system1-omni-review/references/repository-contracts.md, .agents/skills/self-review/SKILL.md (committed-artifact-hygiene section). This is a first review of the complete PR. Per the skill, the PR's edit-free status of review instructions is noted; the PR does not touch .agents/.

Verdict

No P0/P1/P2 findings. 2 P3 findings and 4 notes. The PR is tooling + docs + tests only; it adds no committed run output or generated artifacts (the headline "preserve complete serving evidence" is implemented as run-time persistence in the output directory with checksums, not as repo commits — the correct direction per the artifact-hygiene contract).

Findings

P3-1 — Hard-kill window leaves a fully-flushed record unrecoverable by summarize_saved

  • File: benchmarks/bench.py:315-327 (worker), validated at benchmarks/bench.py:545 ("completed IDs do not match saved responses").
  • Trigger: the process is killed between responses.write(line) + hash update + state["responses_sha256"] update (line 320-323) and the subsequent state["completed_ids"].append(...) + save_state() (line 324-327). The response record for that request is flushed and its checksum is persisted, but completed_ids does not contain the ID.
  • Consequence: a later bench.py summary <dir> (and the in-run() recomputation of any resumed flow) rejects the entire run with ValueError("completed IDs do not match saved responses") even though the raw evidence for that request is internally consistent and complete. The conservative direction is correct (no invented evidence; raw responses.jsonl remains for manual offline scoring, as the README states), but the README's recovery promise ("After a hard kill, a valid start snapshot can distinguish unattempted IDs from attempted IDs without completed records") does not cover this mirror case: a completed record not yet claimed in completed_ids.
  • Evidence: code path read at head (worker sequence above; summarize_saved builds completed_ids from records and requires set equality with ids["completed_ids"]); verified by inspection, not by a kill-injection test. Suggested fix: accept records whose ID is attempted-but-not-claimed as additional completed evidence (they carry their own checksum), or document the rejection explicitly.

P3-2 — warmup.json readiness evidence is not covered by any persisted checksum

  • File: benchmarks/bench.py:424-445 (warmup loop and writes), benchmarks/bench.py:270-284 (measurement_state).
  • Trigger: post-hoc edit of warmup.json in a saved run directory.
  • Consequence: summarize_saved pins requests.jsonl, config.json, warmup-requests.jsonl and responses.jsonl via SHA-256 in completion.json, but never validates warmup.json. Readiness/first-inference evidence — the artifact that establishes "warmup failure stops the run" and the cold/warm boundary story — can be rewritten without detection, while every measured artifact is tamper-evident. Practical risk is low (warmup is excluded from load timing and the tool's hashes are explicitly "not authenticity"), but the PR's stated purpose is preserving serving evidence; the one saved file holding the warm boundary is the one not pinned.
  • Evidence: measurement_state fields enumerated at head lines 270-284; summarize_saved (lines 465-560) reads completion.json, config.json, requests.jsonl, warmup-requests.jsonl, responses.jsonl only — no warmup.json reference. Suggested fix: record a warmup_sha256 (when warmup ran) in completion.json.

Notes (not findings)

  1. bench.py summary on pre-PR saved runs fails with KeyError: 'latency_basis' surfaced as error: 'latency_basis' — terse but conservative, and benchmarks/README.md documents that older runs "remain usable by compare" only. No action required.
  2. An interrupted run's summary.json still carries non-null requests_per_second/decisions_per_second (computed over the partial measured wall). Not silent: stop_reason and evidence_complete: false sit in the same object and the README states interrupted runs "provide partial observations with explicit coverage, not complete traversals". Consumers must quote those flags.
  3. PR-description provenance discrepancy: "Default Ruff reports nine diagnostics" does not reproduce — ruff check (0.15.2, default rules, no repo config) reports zero on both changed Python files at head. The pinned selective check (--select E4,E7,E9,F) does pass, as claimed.
  4. The strict type echo requirement on every answer (benchmarks/bench.py:105-108) is a client-side wire-contract tightening. Verified it matches the actual serving stack: Cua-S1 native emits "type": "choice" (src/models/cua_s1/native/src/contract.rs:181), Open-Jev native emits "type": "noul"/"score" (src/models/open_jev/native/src/contract.rs:244,277), Laya native emits "type": question.kind (src/models/laya/src/decision.rs:111). External endpoints (e.g. vllm-jev) that omit the field will now count every request as invalid_response — visible, not winner-producing, and documented in benchmarks/README.md:73-82.

Evidence-completeness review (focus areas)

  • Raw vs summarized / denominators / errors: preserved and strengthened. responses.jsonl keeps the original response text, its SHA-256, HTTP status, validation result and latency per request; summary.json adds planned/attempted/unattempted/incomplete/failed counts for both requests and decisions with the invariants planned = attempted + unattempted and attempted = successful + failed + incomplete (benchmarks/bench.py:171-219), plus separate failed-latency p50/p95. Failures are excluded from success quality with denominators retained (quality(), count fields); an empty-error transport failure is still counted (regression test included). The prior zip(cases, records) mis-pairing risk in quality() is fixed by ID join.
  • No silent aggregation / winner production: compare now accepts different completion orders but still requires exact unique manifest coverage from both sides and failed == 0 to pass; tolerances remain caller-supplied required arguments, unchanged — nothing relaxed. Interrupted runs cannot be compared (coverage rejection) and cannot exit 0 (run() returns 1 unless evidence_complete). The warmup-failure path saves evidence that all measured requests are unattempted (verified by test).
  • Measurement-scope honesty: latency_basis=post_to_body_json_and_schema_validation is stamped into config and summary; a regression test proves body decode and validation are inside the clock. Warmup is saved separately and excluded; README states client warmup "is not a measurement of server process startup, model loading or service-internal warmup" and that client cancellation "does not prove the server stopped processing"; throughput denominators and the reorder/save exclusion from measured wall are documented.
  • Determinism/reproducibility of offline summaries: summarize_saved recomputes from saved inputs only, rejects checksum disagreement, duplicate/unknown/unattempted IDs, raw-vs-normalized disagreement, and invented timing (wall_seconds null without a terminal boundary). Decoded-text contract (BOM rejected, U+FFFD accepted iff the saved text is valid JSON) is implemented identically online and offline and covered by a regression test. I reproduced equivalence on a hand-built 302-record (see commands).
  • Numerical claim hygiene: no tolerance or budget changes; WIRE_DECIMALS/sum_allowance untouched; PR body labels its H800 table "different-deployment observations, not same-model speedup" and reports correctness per round. Author-reported measurements are unverified here (no GPU on this host); no benchmark outputs were committed in this PR, consistent with "raw artifacts not attached pending redistribution permission" in the PR description.

Changed-file inventory

File +/− Classification
benchmarks/README.md +92/−8 documentation (not in mkdocs nav; site links unaffected)
benchmarks/bench.py +349/−33 authored code
tests/benchmarks/test_bench.py +502/−2 authored tests
  • Total diff: 943 additions + 43 deletions = 986 lines. Authored code count per CONTRIBUTING.md (source + tests, excluding docs): 886 — below the 3,000-line threshold; extra large-change requirements not triggered.
  • No binary files, no generated run output, no fixture dumps added. Pre-existing committed artifacts under benchmarks/gdn/artifacts/20261003/ are untouched by this PR (out of scope, but relevant context for any future hygiene sweep).

Commands run and results

Personally run (this reviewer, on a git archive export of head 4b9cccbd under /tmp/pr-review-2026-10-10/head-checkout; no repo modification, no Rust toolchain invoked, no GPU):

  • python -m unittest discover -s tests/benchmarks -p 'test_*.py' -v (miniconda Python 3.x + httpx 0.28.1): 22/22 tests OK (17 new/extended in this PR: persistence/recompute, cancel, warmup-failure, independent warmup, latency-clock, type-validation cases).
  • python benchmarks/bench.py validate benchmarks/smoke.jsonl: "Validated 4 requests; SHA256 43228534...b2691" — manifest unchanged by the PR and still valid.
  • python -m mkdocs build --strict at head: success (only a pre-existing INFO for docs/benchmarks/cua-s1-cuda-graphs/README.md not in nav; that page exists identically at origin/main). This closes the author's "MkDocs was unavailable" gap for the changed tree.
  • ruff check --isolated --no-cache --select E4,E7,E9,F benchmarks/bench.py tests/benchmarks/test_bench.py: pass (matches author claim). Default-rule run: pass, 0 diagnostics (does not match the author's "nine diagnostics" note).
  • Ad-hoc 302-redirect probe through request_one + summarize_saved: online error_kind=http_302 (httpx 0.28.1 raise_for_status raises on 3xx) and offline recompute agrees — no online/offline asymmetry in the saved-record validation.
  • git diff <merge-base> head | shasum -a 256 = 5c426eea5b4e4b55aac4bee8106b7e84601fae7f84e1ac29aa0e81007861bf70 — matches the frozen diff SHA-256 declared in the PR body.

Author-reported (not independently verifiable here): six saved H800 rounds (Open-Jev-9B/27B × 3 traversals, 231/231 successful each, p50/p95 table), "six saved-round summaries exited 0", JevBench input provenance, Codex/Kimi static-review PASSes.

Observed CI: see below — none.

CI observed

  • gh pr checks 131 --repo ThinkFlowLab/system1-omni → "no checks reported on the 'codex/decision-latency-evidence' branch" (exit 1; retried after one transient network failure on a subsequent call).
  • gh api .../commits/4b9cccbd.../check-runs → total_count: 0; gh run list --commit 4b9cccbd... → empty.
  • Repo-wide gh run list: recent CI/Docs runs on other branches are stuck in queued for 10+ hours (workflow_dispatch), so the runner fleet appears unavailable; pull_request workflows for this head never started.
  • Both workflows at head (ci.yml, docs.yml) do trigger on pull_request to main, so the absence is environmental, not a config error. CI result for this head: no checks reported — an evidence gap, not a pass. The Rust job is irrelevant to this diff (no Rust files changed), and I re-ran its applicable non-Rust equivalents locally above.

Material unverified checks

  1. Any GPU/H800 serving evidence (all author-reported; not re-runnable here): throughput/latency table, 231/231 success claims, quality 179/231 and 198/231.
  2. Raw saved-round artifacts from the six measured traversals — intentionally not attached (redistribution permission pending); only the PR's summary numbers exist as evidence.
  3. The six "saved-round summaries exited 0" invocations of the new summary command — command and result author-reported.
  4. CI on the head SHA — never executed (0 check-runs; runners queued repo-wide).
  5. Cargo fmt/clippy/test/build — not applicable to this diff (no Rust changes) and prohibited on this host regardless; not run.
  6. Kill-injection durability of the new persistence path beyond what unit tests simulate (my P3-1 window is code-inspected, not fault-injected).

No approval or completed validation is implied. The PR remains draft with the contributor self-review checkboxes unchecked, which matches its current evidence state.

Document rejection of torn snapshots and add the latency leaderboard. Preserve historical runs for replay with their original collector.

Changed lines (additions plus deletions): 16 source, 72 tests, 95 documentation.
@linear3735
linear3735 marked this pull request as ready for review October 10, 2026 14:48

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-review at new head 7ac2b11b57: one new commit since my review at 4b9cccbde8 — "bench: verify warmup evidence and keep invalid responses isolated" (4 files, +171/−12).

Both findings from my review are addressed, verified in the diff:

  • Warmup evidence checksum (my P3-2) — fixed: measurement_state now records warmup_sha256 (safe: it only runs after warmup.json is written — checked both the replay and warmup-failure paths), and summarize_saved rejects a missing checksum (with an explicit "use the collector that produced this run" error for pre-change runs) or a mismatched file. Tests cover missing-key, deleted-file, and tamper inclusion of warmup.json.
  • Hard-kill mirror case (my P3-1) — addressed: a new test pins that the crash-window outcome (checksum-consistent responses.jsonl but shorter completed_ids) is rejected loudly ("completed IDs") with raw evidence retained — the conservative behavior the README should now be read as promising.
  • New hardening beyond my findings: number() now catches OverflowError, so a huge integer probability (10**400 is valid JSON) becomes an invalid_response failure with the raw response and its SHA-256 preserved instead of crashing the collector; test-pinned. The new latency-leaderboard.md follows the measurement-scope rules: pinned revisions/dataset SHA-256, clock boundaries stated, denominators preserved.

Personally run at this head: python -m unittest discover -s tests/benchmarks → 25 tests OK (weight-free). No CI has reported on this head. Verdict: COMMENTED (draft; zero actionable findings at this head).

@linear3735 linear3735 closed this Oct 11, 2026
@linear3735 linear3735 reopened this Oct 11, 2026
@MrDongsls

Copy link
Copy Markdown
Collaborator

Independent re-review at 7ac2b11b — no blocking findings. Three small points:

  1. Pooled percentiles have no in-repo command. Issue [Benchmark] Preserve complete serving latency evidence and offline summaries #130 asks to report per-round and pooled percentiles; the new summary sub-command produces per-round only — pooled figures exist solely in the PR/issue bodies. Either a --pooled flag or one README line explaining how pooled numbers are derived would close that acceptance gap.

  2. Two different hashes for the same dataset. Issue [Benchmark] Preserve complete serving latency evidence and offline summaries #130 quotes "expected input SHA-256 00d1f10c…" for public231, while the leaderboard pins "canonical dataset SHA-256 dc3995d8…" — different values, different scopes (input file vs dataset_hash), both pointing at JevBench 7ce310c. One sentence in the leaderboard tying the two together would prevent reader confusion.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head 0a55fce06: one new commit since my review at 7ac2b11b57 — "docs: clarify pooled latency and dataset hash provenance" (2 files, docs-only); both reviewed commits are patch-identical (git range-diff, named refs, cat-file verified). The commit is reviewed and clean: it documents the pooled-percentile methodology exactly per the repo's measurement-honesty rules (per-round validation first, pool only like-configured rounds, concatenate completed samples, nearest-rank p50/p95 formulas, never average round percentiles, per-round and pooled counts reported) and disambiguates two different dataset hashes (issue #130's exported-manifest input hash for the two-model run vs this campaign's canonical JevBench dataset_hash) that could otherwise be mistaken for interchangeable checksums. mkdocs build --strict passes on the full tree at this head; fork CI (rust/build/benchmarks/cuda-contract) is green at this exact head. Zero actionable findings; the PR is no longer a draft and the self-review checklist is complete (all four items checked).

One remaining readiness item before this can be approved: main's PR template (post-#138) also requires the four human contributor answers, the PR-necessity decision, and linked failure evidence for bug fixes, and says PRs stay draft until that work is complete. This body predates the template and does not contain the human answers section — that part of the gate cannot be self-attested. Verdict: COMMENTED (readiness gap only).

@linear3735

Copy link
Copy Markdown
Author

Thanks for the re-review — both documentation points are addressed in docs-only commit 0a55fce06 ("docs: clarify pooled latency and dataset hash provenance"):

  1. Pooled percentiles. benchmarks/README.md now documents the derivation for campaign 416135: validate each round with its original collector's summary; pool only rounds sharing deployment, manifest and timing/collection settings; concatenate latency_ms from responses.jsonl records with state = completed and error = null, keeping repetitions and excluding warmups and unsuccessful records. Sort the concatenated samples as x and set n = len(x); p50 is statistics.median(x), and p95 is x[math.ceil(0.95 * n) - 1] (zero-based nearest rank), with both null if the pool is empty. Never average round percentiles; report per-round and pooled counts alongside. This documents the calculation without adding a --pooled flag. Campaign 415076 retains its separately documented interpolated quantiles.

  2. The two dataset hashes. benchmarks/latency-leaderboard.md now distinguishes them explicitly: 00d1f10c… hashes the exported public231 JSONL request-manifest bytes for campaign Slurm 416135, while dc3995d8… hashes sorted canonical JevBench task records (dataset_hash) for campaign Slurm 415076. Both pin JevBench 7ce310c7; they hash different representations and are not interchangeable file checksums.

At current head 0a55fce06, the CI and Docs workflow runs completed successfully. The PR body's head / diff-SHA-256 / integration-main references have been refreshed; merge against main 3e9cc33a (after #110) is clean, with zero file overlap between the two PRs. All 40 benchmark tests passed on that merged benchmark snapshot on 2026-10-13. Source and tests have not changed since your re-review at 7ac2b11b; no new GPU measurements were added. The contributor-intent and applicable failure-evidence requirements noted in the subsequent review remain readiness items before approval.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head 36c244b6bc: one new commit since my review at 0a55fce06 — "bench: fix two response boundary cases" (2 files + tests, +86 test lines); prior commits patch-identical (range-diff, named refs, cat-file verified). Reviewed: both fixes are sound — RecursionError joins the invalid-response catch (a deeply-nested JSON body previously escaped as an uncaught exception instead of being recorded as invalid_response; test-pinned with a 10,000-deep array), and saved-run status validation accepts non-standard HTTP statuses ≤999 as failed requests instead of invalidating the whole saved evidence file (evidence preservation over rejection, matching this tool's design). Locally run at this head: full benchmarks suite passes. Fork CI rust/build/benchmarks/cuda-contract green at the exact head. Zero actionable findings.

Two items keep this at COMMENTED: (1) the four human contributor answers and PR-necessity decision are still absent from the body (the self-review checklist is complete — that part is done); (2) the carried unverified checks (GPU runs author-reported; Rust side CI-covered).

Copy link
Copy Markdown
Collaborator

Integration note: bind offline validation to the saved precision and protocol

When integrating #93/#141, save and validate the collector’s precision, wire protocol and validation version, and use the same adapter for online collection and offline recomputation.

JEV-VL normalization must preserve the requested answer type required by this collector’s typed-answer validation. Unknown saved formats should fail explicitly, while supported legacy defaults remain documented.

Weekly aggregation must consume evidence_complete; zero failed requests alone does not prove a completed traversal.

CPU coverage should include both protocols, supported precisions, interrupted/torn snapshots, raw-response hashes and CLI recomputation consistency. The deep-JSON and 600/999 status handling already present in this head should remain covered.


Reviewed head: 36c244b6bc8715af8a1958f5d55a6acd35c98edd.

@hsliuustc0106 hsliuustc0106 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head 82c108ede9: one new commit since my review at 8e42b4b298 — "bench: reject adapter-incompatible manifests before creating the archive" (bench.py + tests, +71/−10); the reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).

The commit fixes the P3 from my pass-53b review exactly: with --protocol jev-vl, run() now pre-flights wire_request over the manifest and warmup cases before args.output.mkdir, raising a startup error naming the offending case — so an adapter-incompatible manifest can no longer produce a saved archive that summarize_saved rejects afterwards. The new tests in test_saved_protocols.py pin the rejection. Locally run at this head: PYTHONPATH=. pytest tests/benchmarks → 45 passed. Fork CI at this exact head: benchmarks/build/cuda-contract/rust green.

Remaining before approval: the post-#138 template's four human contributor answers and PR-necessity decision are still absent from the body (the self-review checklist is complete). The P3 status=None save is fixed; no code findings remain. Verdict: COMMENTED (readiness item only).

@hsliuustc0106

Copy link
Copy Markdown
Contributor

Read-only diff review by bot (s1omni维护, 2026-10-21) - not an approval; bots do not approve or merge. Scope: read the diff and the new tests; nothing executed locally (no NVIDIA GPU / no model weights).

What checks out:

  • summarize_saved recomputes the summary from the saved inputs instead of trusting a stored summary: it verifies sha256 of config.json, completion.json, warmup.json, responses.jsonl, requests.jsonl and the warmup manifest, and rejects inconsistent attempted/completed/active id sets. That is the right evidence-preservation posture.
  • Measurement state records start/finish/wall time/stop reason and a rolling responses digest, and the summary CLI (summarize) reads only saved artifacts, so a summary can be re-derived offline.
  • New tests (test_bench.py 610 lines, test_saved_protocols.py 324 lines) cover persistence, corrupted checksums and protocol settings.

Findings:

  1. Merge-order dependency with bench: validate rounded Score and isolated zero-fit results #93: this branch changes benchmarks/bench.py, benchmarks/README.md, tests/benchmarks/test_score_wire.py and is not based on bench: validate rounded Score and isolated zero-fit results #93 (git merge-base --is-ancestor = false). bench: validate rounded Score and isolated zero-fit results #93 goes first; rebase this PR afterwards (add/add conflict otherwise).
  2. Backward compatibility: summarize_saved now raises saved warmup checksum missing; use the collector that produced this run for saved runs written before this change. If any archived benchmark evidence is documented/referenced (e.g. benchmarks/laya_mps/results/), offline summarization of those runs breaks. Either support the legacy layout or add a migration note to benchmarks/README.md.
  3. benchmarks/latency-leaderboard.md adds numbers; I could not verify them (no GPU, no weights). Make sure every row carries configuration + hardware + baseline and keep unmeasured cells explicitly marked, per the repo evidence rule.

CI quoted from 8e42b4b2: run 39442536298 success. No local run; CUDA/accuracy/latency not run.

With --protocol jev-vl, wire_request raises before any HTTP exchange for a
manifest the adapter cannot express (multi-question or non-choice cases).
request_one recorded that failure as status=None with error_kind
invalid_response, and summarize_saved then rejected the record ("response
without status must be a client failure"), leaving a saved archive the tool
itself could never summarize again. run() now pre-flights wire_request over
the manifest and warmup cases and raises a startup error naming the case
before the output directory exists, so every saved archive stays summarizable.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@linear3735

Copy link
Copy Markdown
Author

Reply to the P3-1 finding in the 2026-10-20 re-review at 8e42b4b2: fixed in 82c108ede95c495ec5b57eba52fbdd044fc59116 ("bench: reject adapter-incompatible manifests before creating the archive").

Chosen approach: the first of the two suggested fixes — pre-flight wire_request over all cases before creating the output directory. run() now applies the wire adapter to every case from the main manifest and the warmup manifest immediately after metadata checks and before args.output.mkdir(...) / any HTTP exchange. An incompatible case raises ValueError at startup ("case ... is incompatible with the ... wire protocol"), so the unreachable-archive state (a saved status=None, error_kind="invalid_response" record that summarize_saved later rejects) can no longer be produced by this path: no archive directory is created, and no misleading mid-run classification is recorded. summarize_saved's strict status-None rule (timeout/transport only) is unchanged — the second suggested fix, admitting invalid_response as a client-side classification, was not taken because it would relax the saved-evidence invariant for archives the tool already wrote.

Tests: two new regression tests in tests/benchmarks/test_saved_protocols.py cover both manifest positions: a main-manifest multi-question case and a warmup-manifest multi-question case are each rejected before the output directory is created (and the directory is asserted absent). 2 files, +71/−0.

Local verification (2026-10-21): Python 3.14.6 with benchmarks/requirements.txt installed: 44/45 tests pass. The one failure is the pre-existing deep-JSON environment difference in test_run_saves_deep_json_and_nonstandard_http_failures — Python 3.14's JSON parser no longer recurses, so the probe no longer raises RecursionError; it is unrelated to this patch (the same suite passed 43/43 on your Python 3.12.8 before these two tests were added). The two new tests pass.

Exact-head CI: CI run 39622915902 — rust, benchmarks, cuda-contract SUCCESS; Docs run 39622915336 — build SUCCESS, deploy skipped.

Process disclosure: this commit was implemented by an agent under the contributor's explicit fix-and-push instruction and has not yet received a fresh independent review; no GPU/model measurements are involved (benchmark-client only). The PR body's head/SHA/test-result sections are updated to match.

Levius-Fubuki added a commit to Levius-Fubuki/system1-omni that referenced this pull request Oct 22, 2026
Use typed Score responses, a complete run request and genuine JSONL input. Preserve all seven ScoreWire tests and assertions, with no production validator change.

Align fixture corrections with linear3735 PR InferMatrixLab#131 (82c108e); keep the collector integration separate.

Copy link
Copy Markdown
Collaborator

@linear3735 #93 has now merged into main as d577bb824a5dbf0f5e385cf829bb6b9025dabf8d. Please update this branch onto current main so we can move #131 forward. GitHub currently reports CONFLICTING / DIRTY; the overlap with #93 is in benchmarks/bench.py.

Reviewed head: 82c108ede95c495ec5b57eba52fbdd044fc59116. I inspected the collector, saved-summary/adapter paths, tests and documentation, and reran the benchmark suite in an isolated temporary checkout with Python 3.12.13 / HTTPX 0.28.1: 45/45 tests passed, including all seven ScoreWire tests; the four-request smoke validation also passed. The ScoreWire test file is byte-identical to #93's merged version. No additional correctness finding emerged from this pass; the updated combination still needs validation.

For the conflict resolution:

  • Preserve bench: validate rounded Score and isolated zero-fit results #93's explicit Score precision, analytical rounding allowance, math.fsum, zero relative tolerance, exact level bounds and strict isolated-zero-fit metadata checks. Keep the executable bit and the Decider --score-decimals 2 usage guidance from main.
  • Retain bench: preserve complete serving evidence and offline summaries #131's versioned protocol / score_decimals / validation_protocol contract and propagate it consistently through warmup, measured requests and offline replay. Preserve the explicitly selected legacy archive behavior rather than silently applying new rules to old evidence.
  • Keep the collector's interruption/completeness/checksum handling, JEV-VL typed normalization and preflight checks. Reconcile the changed function signatures and every caller; avoid resolving the whole file by discarding either side's behavior.

After updating, rerun the full benchmark suite from the updated tree (including main's existing tests), smoke validation and CI, and refresh the PR description's head/base and mergeability statements. The contributor-intent answers and PR-necessity decision identified in the previous review also remain outstanding in the current description.

This is a code/test review and integration handoff, not a formal approval of the future combined head. No source branch was modified, and no GPU/model/performance or historical benchmark-evidence revalidation was performed.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review posted at the head SHA 82c108ede9 to repair the review trail. My 2026-10-20 review verified this exact content (the commit 82c108ed "bench: reject adapter-incompatible manifests before creating the archive") but was recorded by GitHub against the prior head 8e42b4b298, so the trail showed no review at this head. Nothing new has landed since 82c108ede — this post re-establishes the record; the verification summary below restates the 2026-10-20 findings, re-confirmed today.

Status at this head (unchanged from the 2026-10-20 review)

  • The commit fixes the P3 from the pass-53b review exactly: with --protocol jev-vl, run() pre-flights wire_request over the manifest and warmup cases before args.output.mkdir, raising a startup error naming the offending case — an adapter-incompatible manifest can no longer produce a saved archive that summarize_saved rejects afterwards. The tests in tests/benchmarks/test_saved_protocols.py (+61) pin the rejection.
  • Locally run at this head on 2026-10-20: PYTHONPATH=. pytest tests/benchmarks → 45 passed.
  • CI at the exact head 82c108ede (re-observed today): benchmarks / build / cuda-contract / rust all pass (fork runs 39622915902, 39622915336).
  • Zero code findings. The earlier P3 (status=None save) and both pass-1 P3s (warmup checksum, hard-kill recovery) remain fixed.

Remaining before approval — unchanged

The post-#138 template's four human contributor answers and the PR-necessity decision are still absent from the body (re-checked today; the self-review checklist is complete). Per the standing gate, this keeps the verdict at COMMENTED despite zero findings.

Unverified

  • No Rust toolchain on this host: the PR touches Python benchmarks only; CI-corroborated compile status for the workspace at this head is observed green.
  • GPU benchmark collection is out of scope for this PR's offline tooling contract and untested here (no GPU).

Verdict

COMMENTED (trail-repair restatement; readiness item only — template answers).

@psv666

psv666 commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

[P2] Strict JSON validation is bypassed by replacement decoding, and the saved evidence loses the invalid bytes.

At benchmarks/bench.py:390–394, the collector saves response.text and then calls json.loads on it. HTTPX's text decoding replaces invalid UTF-8 rather than rejecting it. With HTTP 200 and these raw response bytes:

b'{"answers":{"q":{"type":"choice","choice":"A","probabilities":{"A":1},"note":"\xff"}}}'

httpx.Response(...).json() raises UnicodeDecodeError, but this head's request_one() records error_kind=None and a successful A answer. Its saved response contains U+FFFD in place of the invalid byte. Offline recomputation parses that replacement text too, so it cannot recover or detect the original encoding failure. This can report malformed server output as successful while claiming to preserve the response evidence.

Please validate the original bytes with strict JSON/encoding rules, preserve those bytes in the evidence format, and use the same validation in online collection and offline replay. Add an invalid-UTF-8 online → saved → offline regression; changing only the online parser leaves the saved evidence lossy.

Personally run on the reviewed head: 45 benchmark tests passed (python -m unittest discover -s tests/benchmarks -p 'test_*.py' -v), strict documentation build passed, and the additional HTTPX probe reproduced the result above (Python 3.11.15 / HTTPX 0.28.1). These are collector/CPU checks; no model or GPU run is needed for this finding.

The existing #93 integration handoff still applies: GitHub currently reports conflicts. The results here cover this PR's own head, not a future conflict resolution with main or #141.

Review scope and provenance
  • Reviewed head: 82c108ede95c495ec5b57eba52fbdd044fc59116; checked unchanged immediately before publication.
  • PR API base (main): 52c2967ee294ac6d17e528bd5fae3667713ec82f; diff merge-base: 4a79980d8a75fd063cb3f8247e06215e288b18ac. Validation above used an isolated archive of the reviewed head.
  • Authored code/test diff: +1870/−48; total text diff: +2104/−58 (generated fixtures/docs counted only in total).
  • Repository InferMatrixLab/system1-omni (ID 1386454853). Applied .agents/skills/system1-omni-review/SKILL.md from trusted main@5ba6a3ba668cb33028eb0bce2c6d17cdc53aebe7, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Loaded references: repository contracts, architecture, CONTRIBUTING, self-review artifact/demo rules, CI/docs workflows and the affected component documentation.

@Levius-Fubuki Levius-Fubuki left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent full review at latest head ef13e0ccd144409304f256d91db7bf509bc31f7c: no actionable findings. APPROVED.

This review covers the current six-file increment, including the latest UTF-8 repair and integration with main. Earlier comments against 82c108ed do not describe this head's state: the human contributor answers and PR-necessity decision are now present, the branch contains main, and byte preservation/strict UTF-8 validation is implemented online and offline.

I traced collection through HTTPX content-decoded bytes, base64/hash persistence, display-only replacement text, UTF-8 decoding, wire adaptation, typed/numerical validation, completion snapshots, denominator reconstruction and offline/CLI replay. The byte-format selector is separate from the numeric-validation version; old saved-text archives retain their declared semantics. Unknown selectors, byte/display/hash mismatches and inconsistent completion evidence are rejected. The #93 Score bounds, independent rounding allowance, math.fsum, zero relative tolerance and complete isolated-zero-fit metadata remain in the current validation path.

Personally run on a clean archive of this exact head, Python 3.11.15 / HTTPX 0.28.1 / Ruff 0.15.2:

  • python -m unittest discover -s tests/benchmarks -p 'test_*.py' -v: 103 passed, no failures/skips.
  • python -m unittest discover -s tests/decider -p 'test_*.py' -v: 16 passed, no failures/skips.
  • python benchmarks/bench.py validate benchmarks/smoke.jsonl: four requests validated.
  • Isolated Ruff checks on the four changed Python files: passed; formatting of production bench.py and the new byte-evidence test file: passed. Existing whole-file formatting of test_bench.py was not treated as newly required cleanup.
  • python -m mkdocs build --strict and full PR diff whitespace check: passed.
  • Additional end-to-end CPU probes using both System One and JEV-VL: valid responses succeed, invalid UTF-8 in an unused field is retained losslessly and counts as invalid_response both online and on saved replay.

A carried parser boundary, not a new finding: unused fields containing Python JSON extensions NaN/Infinity/-Infinity are still accepted. I tested the same System One bodies against actual target main and observed the same acceptance. This was not introduced by the UTF-8 repair; the changed README promises strict UTF-8 before JSON/schema validation, not rejection of every parser extension. The probes and baseline results are retained locally rather than misreported as a new regression.

Observed hosted checks are separate: exact-head CI 39910338895 and Docs 39910338894 succeeded; deploy was skipped. No unresolved inline threads were present.

Main and full-PR merge-base are f5ae5acf65e2d68150aef944aae00f4dffc0da27. Read-only merge preview is conflict-free and equals the candidate tree eb17ba391ff142e4eb72cfc11689ba47473edf6d. Complete scope: 1,966 changed source/test lines and 259 documentation lines, 2,225 total across six files (+2,148/-77). New test inputs are maintained CPU regressions; this PR commits no raw benchmark archive. All four self-review declarations and required contributor answers are present.

Material limits: no model/GPU/Slurm execution, real throughput/quality measurement or historical response-archive recomputation was performed. I read the immutable five-model report linked by the leaderboard and checked the transcribed values/scope, but the raw historical traces are not publicly available. Those measurements remain author-reported historical evidence, with the two campaigns' clocks and p95 methods kept distinct. This code/tooling approval does not authenticate those measurements or close #141's separate weekly-consumer issues. Rust/native sources are unchanged in this PR; workspace execution is observed through hosted CI, not claimed as locally rerun here.

Review provenance: repository ID 1386454853; trusted main@f5ae5acf .agents/skills/system1-omni-review/SKILL.md, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Actually read its repository-contract reference, architecture, CONTRIBUTING, self-review artifact/demo rules and CI/Docs workflows. Independently serialized full-index diff SHA-256: 34ee7c22c4507effe0a5c2680ba612d8e1f1417efd367709833d4f98fd908e68 (git diff --no-ext-diff --no-textconv --no-color --full-index <merge-base> <head>). The remote head is rechecked before submission. No merge performed.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #131 delta review — pass 87 (2026-10-23)

Scope: incremental delta on #131 from previously reviewed head 82c108ede to new live head ef13e0ccd144409304f256d91db7bf509bc31f7c. Review only; nothing posted.

Verdict: APPROVE (non-draft, zero findings, template answers now present). Prior open item (post-#138 human contributor answers + PR-necessity decision) is RESOLVED. Prior code findings: zero, unchanged.

Re-pinned state (source inspected)

  • git rev-parse origin/pr/131 → ef13e0ccd144409304f256d91db7bf509bc31f7c; gh pr view 131 → headRefOid identical, isDraft: false, state: OPEN, title "bench: preserve complete serving evidence and offline summaries".
  • cat-file -e verified for 82c108ede, a250a7d6, ef13e0cc, 5ba6a3ba.
  • git fetch origin main → main tip is now f5ae5acf "Merge pull request #100 from xiaoyu-xyz/laya-target" (moved past the pinned guess 5ba6a3ba/#98; the merge absorbed up to #100).
  • First-parent delta: ef13e0cc (authored) ← a250a7d6 (merge) ← 82c108ede. Confirmed via git log --first-parent and git rev-list --parents -n 1 a250a7d6.

1. Own-series integrity — PASS

git rev-parse a250a7d6^1 → 82c108ede95c495ec5b57eba52fbdd044fc59116, byte-identical to the previously reviewed head. The own first-parent series is untouched (no rebase; the only additions are the merge and one authored commit), so the range-diff gate is trivially satisfied (all-= by construction; p1 equality makes any re-diff vacuous).

2. Merge hygiene for a250a7d6 — PASS, resolution read in full

  • Parents: p1 = 82c108ede (PR side), p2 = f5ae5acf (main tip, #100).
  • git merge-tree --write-tree 82c108ede f5ae5acf → exit 1, one conflict: benchmarks/bench.py (auto-merge tree dad123f7).
  • Actual merge tree b6037914 differs from the auto-merge tree in exactly one file, benchmarks/bench.py, 62 deletions. Read every resolution hunk:
    • All six conflict regions take the PR-side (ours) version — the evolved collector with validation_protocol, wire_request, legacy-protocol read_answers branch, measurement_state/summarize_saved saved-replay path — and drop main's strictly simpler variants of the same regions.
    • Two dedup fixes: removed stale duplicate dict keys ("validation_protocol": "probabilities-4dp-model-score-v1" and "score_decimals": getattr(args, "score_decimals", 4) shadowed by the later correct "validation_protocol": version / "score_decimals": score_decimals) and one duplicate identical --score-decimals argparse registration. Both are behavior-preserving (Python dict literals and argparse both let the later definition win; definitions were identical).
  • No lost main-side content: git diff 82c108ede a250a7d6 -- benchmarks/bench.py is a mode-only change (100644→100755, main's 9168935c). Main's only bench.py commits in the range (de34df45, a33b43cd, 9168935c) contribute substance the PR side already contained (verified: score_allowance present at a250a7d6:benchmarks/bench.py:115); the 64 "+" lines of git diff 82c108ede f5ae5acf -- benchmarks/bench.py are all main's older/simpler variants already seen in the conflict hunks.
  • git grep '<<<<<<<\|>>>>>>>' a250a7d6 -- benchmarks/ → no leftover conflict markers.
  • Forward mergeability (body claim): git merge-tree --write-tree f5ae5acf ef13e0ccd → exit 0, tree eb17ba391… = head tree. Verified.

3. Entering content attribution — RESOLVED

  • Main-side commits entering: git rev-list 82c108ede..f5ae5acf --no-merges (≈27 commits: #98 GEMM handle, #100 laya TileLang target, d3 reference worker, cua-s1 docs/fixes, bench precision/exec-bit fixes, PR-template answers requirement, etc.).
  • Bulk identity: git diff origin/main a250a7d6 --stat collapses to 5 files, +1846/−75, all under benchmarks/ and tests/benchmarks/ — i.e., the entire +47775/−1244 delta vs the old head is absorbed main; the branch's own footprint vs main is the benchmark collector surface only.
  • Valen tests are main-side content, not branch-authored: tests/valen/* blobs on the PR head are byte-identical (same blob SHAs) to origin/main, and absent at 82c108ede. They enter via the merge and need no separate review. No branch-authored pytest outside tests/benchmarks/.

4. Authored commit ef13e0cc reviewed in full — zero findings

"bench: preserve response bytes and reject invalid UTF-8" — 5 files: benchmarks/bench.py (+58/−…), benchmarks/README.md, tests/benchmarks/test_bench.py, new tests/benchmarks/test_response_bytes.py (237 lines, 8 tests), tests/benchmarks/test_saved_protocols.py.

Contract checks (review contracts: raw evidence preserved; rejection paths must not corrupt archives):

  • Raw bytes preserved: request_one stores response.content as response_bytes_base64 (lossless), keeps response text as a display copy decoded with errors="replace", and hashes the raw bytes (hashlib.sha256(body)), not the decoded text. The checksum covers exactly what is persisted; response_body() on replay closes the round trip (base64 decode → byte sha256 → text/bytes consistency via the same replace-decode).
  • Invalid UTF-8 rejected, not replaced: response_payload() does strict decode("utf-8"); UnicodeDecodeError ⊂ ValueError, so online it lands in the existing except (ValueError, KeyError, TypeError, AttributeError, RecursionError) handler → error_kind="invalid_response" (record still carries status/bytes/text/hash — no archive corruption); offline in summarize_saved the same strict decode is wrapped in the identical catch and cross-checked against the live classification ("raw response and saved validation result disagree"). Error records (non-2xx, timeout, transport) never hit the strict decode.
  • Archive-format versioning: new response_format=httpx-content-base64-utf8-v1 is written into the checksummed run config; saved_response_format rejects unknown values; absent key or httpx-text-v1 keeps legacy saved-text/hash semantics (back-compat; response_digest retains its pre-existing None guard and type check, so legacy timeout records pass cleanly).
  • Corrupt-archive rejection: base64 validated with b64decode(..., validate=True) plus type check; "saved response text does not match its bytes"; "HTTP response is missing its saved bytes" (body None with status set); byte-sha mismatch. Persistence path writes records (including the base64 field) atomically with incremental sha256, unchanged in structure.
  • Tests are not vacuous: the rewritten test_bench.py case flips the old contract (invalid UTF-8 was previously pinned as ACCEPTED, error_kind=None) to rejection and adds byte round-trip + raw-byte hash assertions. The new test_response_bytes.py covers both protocols, conflicting HTTP charset ignored, empty body vs missing body distinctness, BOM, HTTP 600/999, tamper detection after re-signing the outer responses_sha256 (proves the inner byte-evidence checks are load-bearing), unknown formats (including None and non-string), legacy archive back-compat with byte-for-byte no-rewrite verification, and failed-warmup byte retention; CLI summary subprocess agreement asserted throughout. README documents the new contract accurately, including the integrity warning against retro-fitting byte fields onto old archives.

5. PR body — standing blocker RESOLVED; all pinned claims verified

  • Template answers (the only open item from the 2026-10-22 trail-repair review): NOW PRESENT. The body opens with "## Required human contributor answers" — all four post-#138 questions answered substantively, plus the explicit PR-necessity decision ("Yes", with rationale), scope references (#130, maintainer handoff comment), and observed-failure evidence (psv666's UTF-8 reproduction comment, linked). Dated 2026-10-23.
  • Evidence/Test-Plan re-pinned to ef13e0ccd (head + tree eb17ba391…, merge-base/merge f5ae5acf/a250a7d6); older sections are explicitly dated/labeled historical (pass-84 lesson respected — not stale pins).
  • Claim verification (commands run):
    • Tree: git rev-parse ef13e0ccd^{tree} = eb17ba391ff142e4eb72cfc11689ba47473edf6d ✓
    • merge-tree exit 0 → same tree ✓
    • Full-PR diff SHA-256 60d3e7a2… ✓ reproduced with git -c core.abbrev=7 diff --no-ext-diff --no-textconv --no-color f5ae5acf...ef13e0ccd | shasum -a 256
    • Increment stat +320/−20, 5 files ✓; patch SHA-256 14ec0681… ✓ reproduced with core.abbrev=7
    • Note (informational only): the quoted commands omit the abbreviation-length dependence; the hashes verify under core.abbrev=7 (my default auto-abbrev renders differently). Not a miscription — the values are correct for a pinned abbreviation width.

6. CI at the exact head — PASS (both repos)

  • gh pr checks 131: rust PASS (2m13s), benchmarks PASS (33s), cuda-contract PASS (9s), build PASS (13s), deploy SKIPPED.
  • Actions API, InferMatrixLab: runs 39910338895 (CI) and 39910338894 (Docs) — head_sha exactly ef13e0ccd144409304f256d91db7bf509bc31f7c, conclusion success, pull_request event on codex/decision-latency-evidence. Matches the body's claims.
  • Actions API, ThinkFlowLab: same run IDs mirrored, same head_sha, success. Newer runs at 4d39e717e87a belong to a different branch (runtime-bounded-admission), not this PR; PR head re-checked unchanged after.

7. Local checks — PASS (source inspected, commands run)

  • Worktree of origin/pr/131 at ef13e0cc; PYTHONPATH=. /Users/hsliu/miniconda3/bin/python -m pytest tests/benchmarks -q → 103 passed in 23.69s (matches the body's 103/103 claim; zero skips).
  • tests/decider: 16 passed (matches the body's 16/16 Decider helper claim).
  • Valen tests not run: main-side content, byte-identical to main, outside this pass's branch-authored scope. Worktree removed afterward.

Findings

  • Code findings: zero.
  • Evidence/body findings: zero (patch hashes verified; see informational note in §5).

Prior-finding status

  • Template answers + PR-necessity decision (only open item after 2026-10-22): RESOLVED — present, dated, substantive.
  • Manifest pre-flight P3 (fixed at 82c108e), warmup-checksum and hard-kill P3s (fixed earlier): remain fixed — a250a7d6^1 is byte-identical, so those fixes are untouched.
  • Code findings at previous heads: zero; remain zero at ef13e0ccd.

Verdict

APPROVE — non-draft, zero findings at the new head, template answers present, CI green at the exact head in both repos, all body evidence claims independently reproduced, local suites pass.

@MrDongsls
MrDongsls merged commit a893f0c into InferMatrixLab:main Oct 23, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Benchmark] Preserve complete serving latency evidence and offline summaries

5 participants