Repository navigation
Vela: serve Vela 2.0 0.3B through a Python reference worker - #151
Conversation
A Python reference worker on /v1/systemone that runs the pinned vllm-sr/Vela-2.0-0.3B@d6f03aa9 package's own runtime for text state and all five question kinds (choice/noul/score plus the span/set extensions), served forward-untouched with the upstream whole-request 422/413 error surface, per-member calibration, upstream state truncation, and the /v1/models /health /ready bodies verbatim. The hybrid members and a native executor are future work reusing this wire module. - src/models/vela: shared wire module (envelope preamble with upstream's detail triples, SystemOneError mapping, package gate with a 7-file full-hash pin table, response finishing and the confidence formula mirrors) - src/frontend/vela.py: worker with single-flight 503 lock, warmup before readiness, VELA2_API_KEY 401 gate - recipe/vela: requirements, three example requests (card PII, set threshold override, CJK offsets), compare_reference.py driving the pinned vela2_serve app and the worker pipeline over one engine with exact-match parity - tests/vela: 94 CPU-only contract tests (no weights, no torch) and the frozen oracle fixtures as the reference baseline - docs: supported-models rows, README, recipe index, mkdocs nav Validated on CPU FP32: startup with all-kind warmup, the example and rejection parity fixtures, exact-match reference parity, byte-identical Rust-frontend forwarding.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2812144ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- force the verified package root to the front of sys.path: a PYTHONPATH copy listed later left foreign directories winning the runtime import (verify_no_foreign_runtime only guards sys.modules, empty pre-import) - refuse every importable module form in the package-root gate, not just source .py: a sourceless .pyc or extension module executes just the same once the root goes first on sys.path - compare responses through order-preserving JSON text: dict equality ignored insertion order, so key-order drift passed as parity - add example-request-score.json so the default parity run covers the fifth answer kind with a successful 200 score case - stand in for the 1.2 GB weights with sparse files in the manifest tests instead of full heap allocations
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent review — PR #151 "Vela: serve Vela 2.0 0.3B through a Python reference worker"
Reviewer: independent code review per .agents/skills/system1-omni-review/SKILL.md. Not posted to GitHub; no repo modifications. Written locally to /tmp/pr-review-2026-10-15/body-151.md.
Verdict
3 findings: 0 P0, 0 P1, 1 P2, 2 P3. No correctness or gate-integrity defect found in the code at the reviewed head. The findings are evidence-provenance items: the recorded author-run parity evidence predates the stricter comparison tool introduced by commit 2, and the PR body cites a commit and test count that match no commit on the PR. The package gate at this head is complete and stronger than main's approved D3 gate; all weight-free tests pass locally; all CI green at the exact head.
Base / head / provenance
- Base:
main@e66ff34996760a84ad56690f0ae4cf434e669e65(live, fetched 2026-10-20; merge-base identical, so the PR is based on the current tip). - Head:
f7f33a973a0440d82860aef6fb27b0945a939091(liveorigin/pr/151, fetched withgit fetch origin "pull/151/head:refs/remotes/origin/pr/151" --force; GitHubheadRefOidconfirmsf7f33a97…). - Note on the requested expectation: the task's expected head
0d9af2a77de4f4b075f687987a1dff8c8c932aafis a different commit ("modernbert: reject a zero global attention interval") that is not part of this PR. The live head matches the task's own commit list (2812144 → f7f33a9). The PR is additionally cited in its own body at0bbc44c, which does not exist in the fetched object store (finding P3-2). - Commits:
2812144"vela: add the 2.0 0.3B reference worker",f7f33a9"vela: close the package-gate gaps the review flagged". Open, NON-draft, authorMrDongsls. - Size: 29 files, +3636/−1 (verified via
git diff --stat). - Skill load record:
.agents/skills/system1-omni-review/SKILL.mdread in full; SHA-256 of bytes read =58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108(matches the trusted value). Published skill at the trusted local checkout; references read:references/repository-contracts.md,docs/architecture.md(at head),CONTRIBUTING.md(at head),src/frontend/README.mdcontract via repository-contracts, D3/Valen sources onorigin/mainfor pattern comparison.
Findings
P2-1 — The "exact match including JSON key order" parity claim was produced by a tool version that could not check key order; no recorded run at the reviewed head
- Where:
docs/supported-models.md:27("three 200s byte-equal incl. key order"), PR body Test Result ("exact match on status, body and JSON key order"), and the author-run evidence cited at commit0bbc44c. - Trigger → consequence: commit 1's
recipe/vela/compare_reference.pycompared withworker == reference(dict equality, order-insensitive) even though its own docstring claimed "Key order is part of the match" (verified:git show 2812144:recipe/vela/compare_reference.py,compare_case). A reader trusting the recorded parity run concludes key-order parity was observed; in fact no recorded run could have detected answer/threshold key reordering. Commit 2 fixes the tool (same_resultnow comparesjson.dumpstext, head lines 183–191 — verified order-preserving, and its regression covers reordering) and adds the score fixture, but no author re-run at the head is recorded, and the evidence commit0bbc44cis not on the PR. Thedocs/supported-models.md:27row also describes the old default set ("the five charter fixtures … three 200s"); the head default set is nine cases with four 200s. - Personally verified: commit-1 tool used
==(inspected); head tool is order-preserving and its 20-check regression passes locally; fixture values intests/vela/fixtures/oracle-*.jsonare internally consistent with the body's card claims (domain 0.7272/0.2171/0.0557, conf 0.5101, PERSON 8–17, EMAIL 19–40, "up to 6 grams" 16–29, thresholds 0.46046…/0.55, usage 471 — all confirmed by re-parsing the fixtures). - Ask: re-run
compare_reference.py --exportat (or after)f7f33a97and update the body/row to cite the live head and the actual case count, or soften the key-order wording to what the recorded run established.
P3-1 — PR body test count stale: "92 tests/vela tests" vs 98 at the head (94 at commit 1)
- Where: PR body, Purpose section.
- Trigger → consequence: a reader reproducing at the head collects 98 tests (personally run:
98 passed), not the claimed 92; commit 1 already had 94 (personally counted). Grew, not shrank — no correctness impact — but the number matches no commit on the PR, which weakens the honest-labeling standard the D3 approvals set.
P3-2 — Evidence commit 0bbc44c is not part of the PR
- Where: PR body, Test Plan ("System1-Omni Version / Commit:
0bbc44conmodel/vela2-reference-serving… all checks above pass at this head"). - Trigger → consequence:
git cat-file -t 0bbc44cfails in a clone holdingorigin/main,origin/pr/151, and full history; the cited "head" is a pre-force-push local commit. Per the skill ("recheck evidence after a head change"), author-run evidence should be re-pinned tof7f33a97(and, per P2-1, the parity portion re-run). The four Rust gates, mkdocs strict build and benchmarks are separately covered by green CI at the live head, so the residual gap is exactly the author-side CPU parity/warmup evidence.
What commit 2 ("close the package-gate gaps the review flagged") changed, and whose review it was
The review was the repository's automated chatgpt-codex-connector[bot] Codex review (review id 5585054025, submitted 2026-10-20T12:36:27Z, on commit 2812144). It raised 5 findings; commit 2 addresses all 5 (each verified in the diff):
- P1 (bot) sys.path fronting → new
put_first_on_sys_path(src/models/vela/engine.py:195) drops equivalent resolved entries and inserts the verified root at index 0; regressiontest_the_package_root_is_forced_to_the_front_of_sys_path. - P1 (bot) importable formats →
IMPORTABLE_FILE_SUFFIXES= source +.pyc+ extension suffixes; refusal atengine.py:155; parametrized regression fortorch.pyc/.soforms. - P2 (bot) order-insensitive comparison →
same_resultcompares serialized text (compare_reference.py:183–191) + regression cases. - P2 (bot) no score fixture →
recipe/vela/example-request-score.jsonadded to the default parity set (default_cases, verified 9 cases) + shape regression. - P2 (bot) 1.2 GB test allocations →
sparse_weightstruncates a sparse file (test_engine_manifest.py), verified locally: the suite runs in ~7 s with no large allocation.
Contract fidelity (review focus 1) — OK
- Production answers come from the pinned package's own code:
engine.system_onecallsvela2_inference.system_one_request(engine.py:272);postprocess.finish_responseonly validates and passes the runtime's envelope through; the mirrored formulas (choice_confidence,score_confidence,expected_level) are CPU test spec only, exactly the approved D3/Valen pattern. - No foreign rules imposed: no Cua-S1 26-letter restriction; per-kind limits (choice 2–255, score 2–10, span/set 1–255) are the runtime's, documented as such (
src/models/vela/README.md:58-66). - Error surface mirrors the model's own contract: wire layer reproduces only the upstream preamble triples (
protocol.envelope_error:{"detail": [{"loc": ["body", *loc], "msg", "type"}]}, missing → extra → model, upstream order) and defers all deeper validation to the runtime;preprocess.to_request_errormapsSystemOneError422/413 (too_long);map_runtime_errormaps residualValueErrorto 422 as explicit worker policy, documented in the README (README.md:124-128). - Slice-scoped differences are declared, not smuggled: 8 MiB body cap (
protocol.py:31), minimal upstream-verbatim/health, upstream bearer gate (VELA2_API_KEY).
Runtime-gate integrity (review focus 2) — OK, stronger than the approved D3 gate
verify_package (engine.py:208-212), all pre-import, verified by reading the code and by the locally run tests:
- config structural identity (
vela2-unified/Vela2Model/ max_length 8192 / 8 marker ids) checked before hashing, so a foreign config fails under its own name; - checked-in 7-file SHA-256 table (
PINNED_FILES_SHA256) hashed in full (_sha256_safe, 8 MiB chunks) — the package ships no manifest, so the table is transcribed and pinned perREFERENCE_REVISION d6f03aa9…; a later self-consistent snapshot fails the checked-in pin (test exists); - refusal of unlisted importable entries in the package root — all file forms (source/sourceless/extension) and
__init__.pypackages (the D3-standard gate, here in the post-Codex form); model.safetensorsby exact pinned size (1,236,473,252 bytes), with the honest documented limitation that same-size bit corruption is not caught (README:38-41);- new vs. D3-on-main:
verify_no_foreign_runtime(sys.modules guard) andput_first_on_sys_path(dedup + front) — main'ssrc/models/d3/engine.pystill has the.py-only refusal and theif root_str not in sys.pathinsertion, i.e. both Codex P1 gaps fixed here for Vela remain open on the approved D3/Valen workers (note, not a defect of this PR; a follow-up issue to port the two fixes is worthwhile).
Bounds and transport (review focus 3) — OK
src/frontend/vela.py do_POST: route 404 → Transfer-Encoding 411 → exactly-one Content-Length 411 → integer/<0/>MAX_BODY 413 pre-read → Content-Type 415 → bearer 401 (hmac.compare_digest) → non-blocking lock, 503 busy → read with short-read 400 and 408 on timeout → catch-all 500 with type-only log; parse_body re-checks the cap and rejects duplicate keys, non-finite constants, lone surrogates/non-UTF-8, RecursionError. Structurally identical to the approved D3 worker (src/frontend/d3.py on main), plus the upstream-verbatim /health, /ready, /v1/models bodies. No proxies/redirects/retries: the worker is stdlib http.server bound to 127.0.0.1 by default; the Rust frontend is untouched by this PR. log_message suppressed (no prompt/path/header logging).
Minor note (not a finding): transport status (411/413/415) is decided before the 401 bearer check, whereas upstream FastAPI would run an auth dependency first; on the edge case "wrong Content-Type + wrong key with VELA2_API_KEY set" the worker answers 415 where upstream answers 401. Slice-scoped and covered by the "transport errors follow the shared worker discipline" wording; a one-line note in the model README would remove all ambiguity.
Tests (review focus 5) — run locally, all pass
Weight-free (no torch, no weights, no network) with a contract fake (FakeVela2 re-implementing only the runtime surface the tests exercise; deeper paths pinned by the author-side oracle fixtures). Gate tests pin the refusals: tampered/missing files, foreign config, self-consistent snapshot, unsafe names, unlisted importables (all suffix forms), unlisted packages, foreign runtime import, sys.path fronting, exact weights size.
Docs (review focus 4) — complete
recipe/vela/reference.md (scope, pinned artifacts + re-pin commands, install, launch, all four example requests, error behavior incl. the truncation divergence, validation commands, parity procedure, limits); README table row with honest "CPU FP32 validated (author-run, evidence in the integration PR)" labeling and owner; docs/supported-models.md row with Author-run labeling and owner @MrDongsls (wording issue in P2-1); mkdocs.yml nav entry; recipe/README.md entry. mkdocs build --strict green in CI at the head.
Template readiness (post-#138 gate)
Complete on its face: four human contributor answers with substantive content; explicit PR-necessity answer ("Yes, the integration must land…"); bug-fix evidence N/A justified (new model integration); Purpose/Test Plan/Test Result/Demo sections filled; self-review checklist 4/4 checked; PR is non-draft as required for approval. Split rationale present ("First integration slice of #137") with a component map (Purpose breakdown + Ownership-boundary section) and validation across components (Python suites + four Rust gates + mkdocs). Authored vs total: total +3636/−1 over 29 files; code-only ≈ 2,312 lines (837 production Python + 1,475 test Python; 2,599 if the 287-line recipe comparison tool is counted as code), docs ≈ 472, JSON data ≈ 546, config 16 — under the 3,000 changed-code-lines threshold; total diff is above it, and the required preparation is present anyway.
Artifact hygiene (all 29 files)
Clean. New files are: 5 production Python, 7 test Python, 1 recipe tool + its regression test, 4 example requests, requirements.txt (pinned versions), 3 oracle fixtures (4–8 KB each, machine-generated only by the documented --export path after a passing comparison — maintained fixtures with provenance, per the D3 pattern), 2 READMEs, 4 doc/config edits. No weights, no binaries, no caches, no one-off run output. (__pycache__ seen under recipe/vela/ exists only in my /tmp export from running the regression test; it is not tracked by the PR.)
Commands run and results
Source inspected (all at f7f33a97 unless stated): the 29 changed files in full or in relevant part; src/models/d3/engine.py, src/frontend/d3.py on origin/main (pattern baseline); commit-1 versions of engine.py and compare_reference.py; .github/workflows/{ci,docs}.yml; PR template; Codex review + 5 inline comments (API).
Personally run (miniconda Python 3.12, macOS arm64, no GPU, no Rust toolchain):
PYTHONPATH=src python -m pytest tests/vela -q→ 98 passed in 7.2 s (body claims 92 — see P3-1).python recipe/vela/test_compare_reference.py→ 20/20 checks ok, exit 0.PYTHONPATH=src python -m compileall -q src/models/vela src/frontend/vela.py tests/vela→ OK.PYTHONPATH=src python -m pytest tests/d3 tests/valen tests/frontend -q→ 82 passed (no regression in neighbor suites).git cat-file -t 0bbc44c→ not found (P3-2);git logfor0d9af2a7…→ unrelated modernbert commit.- Fixture re-parse confirming the body's card/set/CJK claims (values above).
Observed CI (via gh pr checks 151 + run API, both runs head_sha=f7f33a973a0440d82860aef6fb27b0945a939091, conclusions success): CI → build pass (16 s), rust pass (1 m 56 s), cuda-contract pass (8 s), benchmarks pass (36 s); Docs → mkdocs --strict build pass, deploy skipped (non-main ref).
Author-reported (not independently verifiable on this host): pinned-package download and hash re-check; FP32 load/warmup-before-bind; five-case exact parity vs the pinned vela2_serve.py app incl. key order; byte-identical frontend forwarding; ~0.5 s warm single-request wall time (single observation, honestly labeled); environment x86 WSL2 / Python 3.12.3 / torch 2.11.0 / transformers 5.19.0.
Material unverified checks
- Reference parity at the reviewed head (P2-1): the order-preserving comparison and the score fixture have no recorded author run; the frozen oracle fixtures predate commit 2's tool fix. This is the one check I would ask the author to re-run before merge.
- The pinned package
vllm-sr/Vela-2.0-0.3B@d6f03aa9…was not downloaded; the 7-file SHA-256 table,EXPECTED_WEIGHTS_BYTES, upstream body verbatimness (/health,/v1/models, 401 message),create_app(...)signature and the upstream preamble/error precedence are author-side claims pinned by the author-run fixtures, not locally reproducible here. - Rust gates verified via CI observation only (no local toolchain by host constraint); no GPU anywhere in this slice's evidence, correctly claimed as none.
- Multi-violation error precedence (e.g., missing field AND wrong model in one request) is not covered by any compared fixture; single-violation cases are. Inherent to fixture-based pinning, same as the approved D3 pattern.
|
Thanks for the thorough review — all three findings and the minor note are addressed in P2-1 — took your first option: re-ran
Same environment as the recorded run (x86 WSL2, 24 cores, Python 3.12.3, torch 2.11.0, transformers 5.19.0, pinned package P3-1 — re-ran at the head: P3-2 — body re-pinned to |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Incremental re-check at new head 108aec653955: one new commit since my review at f7f33a973a0 — "vela: re-record the parity evidence the review flagged" (3 files: supported-models row, model README, +105-line score oracle fixture); the two reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).
The P2 from my review is resolved, verified at this head: the supported-models row now records exact-match reference parity on all nine cases with four 200s byte-equal including key order, plus agreed 422/413 — established by the order-sensitive comparison tool at this head (the fix from the PR's second commit), with the re-recorded run adding the missing score fixture. The model README adds the honest transport-vs-auth precedence divergence note.
Two P3s remain (both minor, carried class): the PR body's test count still says 92 (head has 98) and the evidence commit 0bbc44c it cites is still not on the PR. Locally run at this head: PYTHONPATH=src pytest tests/vela → 98 passed; compare tool self-script ok. CI at this exact head: 4/4 pass.
Verdict: COMMENTED (two carried P3s). Standing unverified: CPU FP32 parity/warmup at this exact head is author-run (no GPU/weights here; the package isn't downloadable from this host); Rust gates via CI observation.
The rebase onto model/vela2-reference-serving kept this branch's side of the divergence (the every-importable-suffix package gate with namespace directories, and the endpoint-parity comparison); this commit restores what the reference worker's own review fixes added so nothing from that head is silently reverted: put_first_on_sys_path returns as the resolve-based sys.path promotion that drops equivalent entries, the token_budget executor option comes back, the manifest tests stand in for the 1.2 GB weights with sparse files again, the forced-to-front sys.path test returns, and the score example the InferMatrixLab#151 head added (four recipe examples, nine default comparison cases) is wired into the oracle fixture test and the support-matrix row. Signed-off-by: Mr.Dong <1042542469@qq.com>
|
Read-only diff review by bot ( What checks out:
Findings:
CI quoted from |
|
The changes you requested are addressed in #156, which is already approved. PTAL. |
|
[P1] Existing bytecode caches can bypass the verified-source gate. The package-root scan permits This probe uses a small synthetic file table, so it needs no weights; the same scan is used by the full package gate before import. Hashing every named Please reject/validate the relevant existing caches, or explicitly execute the verified source. Merely setting Personally run: 98 weight-free CPU tests passed ( Review provenance and scope
|
|
Fixed in |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review verdict: COMMENTED at 3ca677b973db8e06c61eab7584a903943c858fc2 (origin/pr/151 = gh pr view 151 --json headRefOid = the commit 3ca677b9). Non-draft, OPEN, title unchanged. Prior review: COMMENTED at 108aec653 (2026-10-20).
Delta isolation
git log --first-parent 108aec653..3ca677b97 contains exactly one commit: 3ca677b9 "vela: refuse existing bytecode caches at the package gate" (Mr.Dong, 2026-10-24; 4 files, +50/−15). Parent check: 3ca677b9^ == 108aec653 byte-identical (exact SHA), so the previously reviewed series was not rebased or rewritten — no range-diff needed. All SHAs verified with git cat-file -e (prior head, new head, commit). Read in full (git show).
The new commit: bytecode-cache refusal at the package gate
What is refused: any entry named __pycache__ in the package root — file or directory, checked before the importable-suffix chain (src/models/vela/engine.py:156-159), so a root cache is now fatal where the prior tree explicitly allowed it (the old test_unimportable_package_root_entries_still_pass seeded a __pycache__ and asserted it passed). Refusal is root-scoped; subdirectory caches (onnx/__pycache__) are not refused — inert by themselves, since CPython only reads a __pycache__ cache as a companion to an existing source file, and importable sources in non-package directories are a separate, pre-existing question (see notes).
Why: a cache is not verifiable against the pin, and an unchecked-hash .pyc is imported by CPython without consulting the source beside it — so a poisoned cache next to hash-verified sources would execute unverified bytecode after a clean verify_files pass. The new test demonstrates exactly this bypass: compile vela2_inference.py from poison into __pycache__/vela2_inference.<cache_tag>.pyc with PycInvalidationMode.UNCHECKED_HASH, restore the manifest-matching source, and the gate must refuse (tests/vela/test_engine_manifest.py:202-220).
Where: the package gate (verify_files), which runs before anything is imported — unchanged positioning; the problem raised as ValueError("package verification failed: …") (engine.py:165), same channel as the other refusals.
Self-consistency guard: the gate's own strictness would otherwise break the worker — the import vela2_inference inside Vela2Executor would write a __pycache__ and make every later gate run of the same package fail. The commit wraps that import in sys.dont_write_bytecode = True with a finally-restore (engine.py:249-256). Correct pattern.
Disclosed impact: yes — a legitimate precompiled deployment shipping root bytecode caches would now be refused; this is stated in all three doc surfaces (recipe table and troubleshooting in recipe/vela/reference.md, src/models/vela/README.md, and the verify_files docstring). For a pinned HF-snapshot threat model (caches are not part of the pinned revision) refusing is the right default. Recovery (delete __pycache__) is not spelled out; see finding below.
Tests pin it: the poisoned-cache refusal test above, plus the layout test updated to no longer allow a cache. Suite green locally: PYTHONPATH=src pytest tests/vela -q → 99 passed at this exact head.
Cross-PR note (shared fix — read together with #152 and #156)
#151 is the origin. #152's 28187c80 and #156's 2b262f74 are ports of the same fix. All three are semantically equivalent — same refusal set (any root entry named __pycache__, checked first), same gate position (pre-import), same error channel (ValueError), same dont_write_bytecode import guard, same poisoned-unchecked-hash test shape, docs updated on all three surfaces. They are not patch-identical: refusal message strings differ ("existing bytecode cache pycache/ …" vs #152's "unexpected pycache in the package root: …" vs #156's "unexpected bytecode cache pycache/ …"), test names/assertions differ, and #152's branch carries its own stronger gate (carries_code nested-code refusal, IMPORTABLE_SUFFIXES) — its docstring previously argued __pycache__ was safe ("never reached by import by name"); this commit correctly reverses that rationale on the unchecked-hash bypass. Ancestry verified: 108aec653 (#151's prior head) is an ancestor of #152's head, 3ca677b97 is not — merge-base of the two new vela heads is 108aec653. So #152 is a parallel implementation from #151's old head; consequence tracked in the #152 body. Suffix sets are content-equivalent across all three (SOURCE | {.pyc} | EXTENSION).
Prior-finding status (re-verified against the current body and tree)
- P2 (parity evidence) — was already resolved at
108aec653(prior review). - P3 (body test count 92 vs 98) — RESOLVED. Body now says 99 in both Purpose and Test Result; locally collected and run at this head: 99 passed — matches.
- P3 (evidence commit
0bbc44ccited but not on the PR) — RESOLVED. The current body no longer cites0bbc44c; the Version/Commit line pins3ca677b(3ca677b973db…) onmodel/vela2-reference-serving, which is the PR head, and claims "all checks above pass at this head". - Template gate — SATISFIED: the four required human contributor answers, the PR-needed answer and the bug-fix N/A line are present; Demo/evidence and Self-review sections intact.
CI at the exact head (observed)
gh pr checks 151: rust, build, cuda-contract, benchmarks pass (2m12s / 18s / 5s / 29s); deploy skips. Head SHA verified through the Actions API: both CI (run 40000482876) and Docs (run 40000483203) report head_sha = 3ca677b973db…, conclusion success, completed (runs live on InferMatrixLab/system1-omni, which mirrors this repo; the fork MrDongsls/system1-omni has no Actions runs at this SHA).
Local checks (run at a worktree of 3ca677b97; no cargo/GPU on this host)
PYTHONPATH=. python -m pytest tests/benchmarks -q→ 58 passed.PYTHONPATH=src python -m pytest tests/vela -q→ 99 passed (matches the body's claim; includes the new refusal test).- Worktree removed after.
Findings
- P3 — the documented parity workflow re-poisons the package for the gate.
recipe/vela/compare_reference.py:253does a bareimport vela2_serveinmain()afterVela2Executorconstruction. The executor's import is guarded, but this one is not (theimport vela2_inferenceon the line above is asys.moduleshit;vela2_serveis a fresh import with bytecode writing back on), so Python writes__pycache__/vela2_serve.<tag>.pycinto the package root. Trigger: run the recipe's owncompare_reference.py, then start the worker against the same package. Consequence:engine.py:156refuses the package at the next startup until the cache is deleted manually — and the refusal message ("existing bytecode cache … not verifiable against the pin") does not tell the user deletion is the fix. One-line fix in the tool (extend thedont_write_bytecodewindow over both imports) or a recovery hint in the docs/troubleshooting.
Notes (informational):
- The refusal is root-scoped; a nested
__pycache__without importable sources beside it cannot be imported by name, so this is sound for the shadowing threat model. (#152's branch additionally refuses nested importable code viacarries_code; this branch's gate does not scan nested code — pre-existing difference between the branches, not introduced here.) sys.dont_write_bytecodeis a process-global mutated for the duration of one import statement at single-threaded startup; no practical hazard.- Parity/oracle evidence in the body was recorded at an earlier head; this delta does not touch the serving path (gate + import flags only), so that evidence stands for this head.
mkdocs build --strict,compilealland the Rust gates in the body's Test Plan are author-attested at this head; CI (observed) covers the Rust gates and benchmarks.
Unverified
- CPU FP32 serving behavior with the real pinned package (gate pass + warmup + parity at this exact head) is author-run; the package is not downloadable from this host. The delta does not alter the serving path, and CI + local CPU suites are green at the exact head.
Verdict
COMMENTED. The delta is a correct, well-tested closing of a real bypass (unchecked-hash caches execute without consulting verified sources), self-consistent with its own gate, disclosed in the docs, and pinned by a targeted regression test; both prior P3s are resolved. One new P3: the recipe's own comparison tool writes the very cache the new gate refuses. This keeps the PR at COMMENTED; with that one-line follow-up it is otherwise approve-shaped.
|
Fixed in |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent review — PR #151 "Vela: serve Vela 2.0 0.3B through a Python reference worker" — incremental delta re-check
Review scope provenance. Local repo /Users/hsliu/thinkflowlab/system1-omni. Prior review: COMMENTED (pass-93, 2026-10-24) at head 3ca677b973db8e06c61eab7584a903943c858fc2 — one P3 (recipe/vela/compare_reference.py:253 bare-imports vela2_serve outside the package-gate's bytecode guard, so the documented parity workflow writes the __pycache__ the new gate then refuses). Live head at this re-check: 2f94be51713e1893315804096e2cc5d6bae657a6 (= gh pr view 151 --json headRefOid). Non-draft, OPEN, title unchanged, base main.
Own-series integrity (gate 1)
git log --first-parent --oneline 3ca677b97..2f94be517 contains exactly one commit: 2f94be51 "vela: keep bytecode writing off for the recipe's runtime imports" (1 file, +10/−2). Parent check: 2f94be51^ == 3ca677b973db8e06c61eab7584a903943c858fc2 byte-identical (exact full SHA) — the previously reviewed series (gate commit 3ca677b9 included) was not rebased or rewritten; no range-diff needed. All relevant SHAs verified with git cat-file -e. Commit read in full (git show).
The authored commit 2f94be51 — closes pass-93's P3 exactly as suggested
- The fix: in
recipe/vela/compare_reference.pymain()(:252-261), both runtime imports —import vela2_inferenceand the bareimport vela2_servethat pass-93 flagged — now sit inside asys.dont_write_bytecode = Truewindow with try/finally restore of the saved previous value. The barevela2_serveimport is inside/after the guard, exactly the one-line shape the pass-93 review asked for ("extend thedont_write_bytecodewindow over both imports"). The comment states the rationale (the gate refuses existing caches;vela2_serveis a fresh import that would otherwise write the cache the next worker start rejects). - Completeness: grep confirms this is the only
vela2_*import site in the tool;sysis imported at module level (:38). TheVela2Executorconstruction on the line above self-guards its own import (engine.py'sdont_write_bytecodewrapper from3ca677b9, unchanged).vela2_inference/vela2_servelive in the packaged HF artifact, not this repo, and their module-level sibling imports execute duringimport vela2_serve— i.e., inside the window. The code after the window (create_app,TestClient) imports no further model-package modules (source inspected). - Semantics preserved: with the window closed the flag is restored, so nothing else in the process changes behavior; for all-finite normal operation the change is a no-op apart from suppressing cache writes during the two imports. Suite behavior confirms: 99/99 vela tests and the full compare_reference battery pass unchanged at this head (below).
- No new test pins the tool-level fix — consistent with this repo's conventions (
main()of the recipe tool needs the real downloaded package; the standalonetest_compare_reference.pycoverscompare_caselogic only). Note-level; the engine-side gate regression test from3ca677b9remains the pinned protection.
Cross-PR port status (read together with #152)
#152's 8d7be88a is a parallel port of this fix. Hunk-level diff of the two: semantically identical — same window, same two imports, same try/finally restore, byte-identical comment text; the only textual differences are the saved variable's name (previous_dont_write_bytecode here vs dont_write_bytecode in #152) and each branch's surrounding context lines. Ancestry: 3ca677b97 (this head) is not an ancestor of 8d7be88ac; #152's branch is rebased on this PR's old head 108aec653, so the two branches still conflict textually in compare_reference.py (adjacent lines) plus engine.py/tests/docs from the prior 3ca677b9/28187c80 pair. #152's body explicitly commits to resolving that with an explicit reconciliation commit after this PR merges (the f013512 precedent), so the conflict is coordinated, not incidental — standing note, no finding on either side this pass.
Prior findings — status at the new head
- Pass-93 P3 (recipe tool re-poisons the package for the gate) — RESOLVED, verified in full above.
- All earlier findings remain resolved: P2 parity evidence (resolved at
108aec653), body test count (99 — re-verified locally, matches), evidence-commit pinning (Version line now pins2f94be5/ full SHA, "all checks above pass at this head").
Body / evidence check at the new head
Template complete: the four required human contributor answers, the PR-necessity answer, the bug-fix N/A, and all four self-review checkboxes present. Version/Commit re-pinned: "2f94be5 (2f94be51713e…) on model/vela2-reference-serving … all checks above pass at this head." Test Result's "99 tests/vela tests … pass at the head" is accurate at the new head (the delta adds no test; count verified locally). The Purpose narrative of compare_reference.py remains true of the changed file.
Commands run and results (this host: macOS arm64, miniconda Python 3.12.8, no cargo/GPU)
- Worktree at
2f94be517(removed after):PYTHONPATH=src python -m pytest tests/vela -q→ 99 passed (matches the body's claim). python recipe/vela/test_compare_reference.py→compare_reference tests: ok.PYTHONPATH=. python -m pytest tests/benchmarks -q→ 58 passed (unchanged from pass-93).- Series forensics: parent-hash equality, first-parent log,
cat-file -e, hunk-level port diff vs #152 (above).
CI observed at the exact head
gh pr checks 151 (ThinkFlowLab): rust pass (2m07s), build pass, benchmarks pass, cuda-contract pass; deploy skipped (main-only). Actions API queried at the full SHA 2f94be51713e… in both ThinkFlowLab and InferMatrixLab: CI and Docs both completed / success with head_sha matching the PR head exactly. CI does not execute tests/vela.
Notes (informational)
- Residual theory: a lazy import of a packaged
vela2_*sibling triggered inside a request handler would run after the window is restored and could write a cache — but the artifact is out of repo, the packaged modules' imports observed at module level all fire inside the window, and the executor's import stays self-guarded; no in-repo code path regains the hazard. - The one-sided coordination: this PR's body does not mention #152's parallel implementation; #152's body carries the reconciliation commitment and the merge-order mandate, which is where it belongs.
- CPU FP32 serving behavior with the real pinned package remains author-run (package not downloadable from this host); unchanged disclosure, and the delta does not touch the serving path (tool import flags only).
Unverified
- Nothing new. The fix is in the documented parity workflow's tool; running that workflow end-to-end requires the pinned HF package (author-run only, as before).
Verdict
APPROVED — restored from pass-93's COMMENTED. The single open P3 is fixed exactly as suggested, with identical semantics to the (reviewed-together) #152 port, zero new findings, template complete and re-pinned to the head, CI green at the exact head in both orgs, and all local suites green with unchanged counts. The #152 reconciliation after this merges is coordinated in #152's body and tracked there as a standing note.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent local re-review at 2f94be51713e1893315804096e2cc5d6bae657a6, base/merge-base e66ff34996760a84ad56690f0ae4cf434e669e65. Scope: 30 files, +3788/-1; authored Python including tests +2640. The head is unchanged from the prior approval; these are additional reproduced cases that approval did not cover. Head rechecked before submission.
Requesting changes for two reproduced defects:
- P1 — package pin gate accepts a sourceless dependency package. An unlisted
torch/__init__.pycpassesverify_files, then Python can import its unverified bytecode after the checkpoint root is promoted ontosys.path. No hashed file needs to change. The current no-bytecode-writing window does not disable bytecode reads. - P2 — non-ASCII invalid bearer header terminates the handler. With an ASCII API key,
Authorization: Bearer <byte 0xff>causeshmac.compare_digestto raiseTypeError; a real HTTP request gets a disconnected socket rather than JSON 401. A correct bearer first returned 200 in the same local server probe.
Minimal CPU-only reproduction of the pin-gate issue, from the PR checkout with PYTHONPATH=src:
import hashlib, importlib.machinery, importlib.util, py_compile, tempfile
from pathlib import Path
from models.vela.engine import verify_files
with tempfile.TemporaryDirectory() as directory:
top = Path(directory)
root = top / 'checkpoint'
root.mkdir()
pinned = root / 'vela2_inference.py'
pinned.write_text('PINNED = True\n')
table = {pinned.name: hashlib.sha256(pinned.read_bytes()).hexdigest()}
(root / 'torch').mkdir()
source = top / 'init_source.py'
source.write_text('UNVERIFIED_MARKER = 7301\n')
py_compile.compile(str(source), cfile=str(root / 'torch/__init__.pyc'), doraise=True)
source.unlink()
verify_files(root, table) # unexpectedly accepts the unlisted package
spec = importlib.machinery.PathFinder.find_spec('torch', [str(root)])
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
assert module.UNVERIFIED_MARKER == 7301 # unverified code executedThe injectable pin table is the same mechanism used by the committed verification tests; in a real otherwise-valid checkpoint, adding this unlisted directory likewise leaves the pinned hashes untouched.
Agent-executed local validation on Linux x86_64, Python 3.12.13:
CUDA_VISIBLE_DEVICES='' PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=src python -m pytest tests/vela tests/d3 tests/valen tests/frontend -q: 181 passed.python recipe/vela/test_compare_reference.py: 20 helper checks passed; this does not run a model forward.python -m compileall -q src/models/vela src/frontend/vela.py tests/vela: passed, bytecode routed outside the checkout.- All four stored oracle requests matched their maintained example inputs and passed production
finish_responsereconstruction. python -m mkdocs build --strict --site-dir <external directory>with the pinned MkDocs dependencies: passed. An initial run in an incomplete existing docs environment failed on missingpymdownx; the pinned-environment rerun passed.- Two extra regression cases failed on the reviewed source, demonstrating the gate bypass and real HTTP disconnect above.
git diff --check: passed.
Same-head CI and Docs succeeded separately. No real model artifacts were downloaded or loaded, and no model warmup, reference numerical campaign or GPU execution was run. The pin table's upstream byte identities were not independently fetched. A full Rust rebuild was not repeated for this Python-only change. Fake-worker HTTP tests and stored fixtures do not qualify real inference.
Review provenance: agent-assisted review using .agents/skills/system1-omni-review/SKILL.md at trusted main a893f0cf55135623fb093f401d8ce4713330d801, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Architecture, repository-contracts reference, CONTRIBUTING, artifact hygiene and affected CI/component contracts were read. Commands below were executed by the agent; this does not attest personal human execution.
| elif entry.is_dir() and (entry / "__init__.py").is_file(): | ||
| problems.append(f"unexpected importable package {entry.name}/ in the package root") |
There was a problem hiding this comment.
[P1] Reject sourceless dependency packages before promoting the package root. This directory check recognizes only __init__.py. An unlisted torch/__init__.pyc therefore passes verify_files; on a fresh worker, Python can then resolve torch to that package and execute its unverified bytecode after this root is placed first on sys.path. I reproduced acceptance and actual harmless-marker execution through Python's package loader without changing a pinned file. sys.dont_write_bytecode only prevents writes and does not close this read path. Recognize all supported package initializer suffixes (bytecode and extensions as well as source), or inspect unlisted directories for importable code, and add a sourceless-package regression.
| got = self.headers.get("authorization", "") | ||
| if hmac.compare_digest(got, f"Bearer {key}"): |
There was a problem hiding this comment.
[P2] Treat non-ASCII invalid bearer text as unauthorized. With an ASCII API key configured, an HTTP header containing byte 0xff is decoded into a non-ASCII string and hmac.compare_digest raises TypeError: comparing strings with non-ASCII characters is not supported. _unauthorized() is outside the POST exception mapping and is also called directly by GET /v1/models, so the thread terminates without the documented JSON 401. Reproduced over a real localhost connection after confirming the correct bearer returns 200. Compare safely encoded bytes or reject invalid header text, and cover both authenticated routes.
The requested-changes review reproduced two defects at this head; both fixes are pinned by regression tests under tests/vela: - engine: the package gate's directory check only recognized packages with a source __init__.py, so an unlisted dependency directory whose __init__ was bytecode or an extension (the review's torch/__init__.pyc) passed verify_files untouched and then executed unverified after the package root went first on sys.path. A directory is now refused when it carries any importable entry at any depth: an __init__ in every module form (source, sourceless, extension) or a namespace package's nested modules. The pinned layout's own __init__-less directories (onnx/, assets/, LICENSES/, figures/ with data files only) still pass, pinned by the existing test. - frontend: a bearer header carrying bytes outside ASCII (a raw 0xff) decodes to non-ASCII text, which hmac.compare_digest refuses to compare — the TypeError killed the handler and the client saw a dropped connection instead of the upstream 401 body. The comparison failure now answers the same JSON 401 as any other wrong bearer. Checks: pytest tests/vela (103 passed) and the review's full selection tests/vela tests/d3 tests/valen tests/frontend (185 passed; 181 at the reviewed head plus these four regressions) + compileall clean.
|
Thanks for the re-review — both reproduced defects are fixed in
Validation at |
|
The author’s follow-up at current head To make the next review conclusive, please resolve current-main conflicts while retaining those fixes and regressions, then provide the resolved head and a small mapping of fix commit → changed paths → regression. Rerun Keep the pinned real-model evidence separately identified. A merge that changes only integration/docs does not itself invalidate unchanged model measurements; if loader/executor behavior changes during resolution, spell out that delta and rerun its affected gate. This is a readiness follow-up, not independent recertification of the author’s new test counts. #152 should reconcile with this final reference-worker version before landing. Agent-assisted readiness follow-up using the repository review skill pinned at trusted main |
The review reproduced a gate bypass the depth walk cannot see: rglob does not descend into a symlinked subdirectory, but import does, so a real directory whose only entry is a symlink to a directory with a module passed verify_files and the module executed unverified after the package root went first on sys.path. Directory symlinks are now refused outright at the package root and inside the depth walk, with the same unverified-content rationale as __pycache__; file symlinks keep their existing treatment (hashed in full when table-named, refused by suffix otherwise), and the pinned layout's data directories still pass, pinned by the existing test. Checks: pytest tests/vela (105 passed) and the review's full selection tests/vela tests/d3 tests/valen tests/frontend (187 passed; 185 at the reviewed head plus these two regressions), recipe helper test ok, compileall and mkdocs --strict clean.
Semantic resolution of the three registry conflicts (README.md, docs/supported-models.md, recipe/README.md): both sides rewrote the D3 (Decision 3.0) entries — this branch around the older text-state wording, main for media serving (InferMatrixLab#148) — so main's media wording wins for D3 and this branch's Vela 2.0 0.3B rows and sentences are kept after it. mkdocs.yml merged cleanly with both nav entries. Checks on the merged tree: pytest tests/vela tests/d3 tests/valen tests/frontend (200 passed), both recipe helper tests ok, compileall clean, mkdocs --strict clean, cargo fmt --check clean.
|
Conflicts with current main are resolved in One more fix landed before the merge. An independent review of the depth-walk fix at Fix commit → changed paths → regression, on the resolved head
Checks on the combination at |
…itions Semantic resolution of the four registry conflicts (README.md, docs/supported-models.md, mkdocs.yml, recipe/README.md): main added the Clef and pplx-decider-v1.1-27b reference workers while this branch added Vela 2.0 0.3B, so both sides' rows, sentences and nav entries are kept. The merge touches none of this branch's vela code — src/models/vela, src/frontend/vela.py, recipe/vela and tests/vela are byte-identical to the PR head. Checks on the merged tree: pytest tests/vela tests/d3 tests/valen tests/frontend (218 passed), both recipe helper tests ok, compileall clean, mkdocs build --strict clean, cargo fmt --check clean, cargo clippy --workspace --locked --all-targets -D warnings clean, cargo test --workspace --locked clean.
Required human contributor answers
Serve Vela 2.0 0.3B in system1-omni behind the existing Rust frontend, on the checkpoint's own pinned runtime, with its full SystemOne contract, all five question kinds and the whole-request error surface.
Contract fidelity, the worker must match the pinned upstream runtime exactly, because it is the baseline every later native Vela implementation is checked against.
The worker executes the checkpoint's own implementation instead of rewrite it, and this PR freezes the exact-match oracle fixtures the native slices must reproduce.
Users who want the Vela 2.0 family in system1-omni, and native integration needs the reference baseline first.
After answering the four questions: is a PR needed at all to achieve this outcome?
Yes, the integration must land for system1-omni to serve Vela 2.0.
For a bug fix: link actual observed failure evidence.
N/A: new model integration.
Purpose
First integration slice of #137: serve the pinned
vllm-sr/Vela-2.0-0.3Batd6f03aa9…behind the Rust frontend through a reference worker. All five question kinds over text/JSON state —choice,noul,score, plus thespan/setextensions with their Noul views and the top-levelspans/sets/thresholds. The envelope and error surface are upstream's verbatim: exactlystate|model|questions, whole-request 422{"detail": [{"loc", "msg", "type"}]}on any invalid field, 413 only when questions+schema alone exceed the 8,192-token window, 401 only whenVELA2_API_KEYis set, no per-question errors.src/models/vela/: shared wire module in the valen/d3 layout, member-neutral for the hybrid slices.protocol.pyowns the envelope preamble with upstream's exact detail triples and leaves deeper validation to the pinned runtime, so error precedence stays upstream's;preprocess.pymaps the runtime's errors to 422/413;engine.pygates the package at startup — config identity plus a checked-in 7-file SHA-256 table hashed before anything is imported — then runs the pinned runtime;postprocess.pyowns response finishing and mirrors the confidence formulas for the CPU tests.src/frontend/vela.py: stdlibhttp.serverworker; all-kind warmup before bind, single-flight execution with 503 busy, upstream's/health/ready/v1/modelsbodies,VELA2_API_KEYbearer gate.recipe/vela/:reference.md, pinnedrequirements.txt, three example requests, andcompare_reference.py— each fixture runs through both the pinnedvela2_serve.pyapp and the worker pipeline over one shared engine;--exportfreezes the oracle fixtures.tests/vela/: 99 CPU-only tests with a contract fake — no torch, weights or network — plus the four exported oracle fixtures.span/setare served forward-untouched, answering [New Model]: Support the Vela 2.0 series #137's feedback Q3 with the package's own behavior; over-limit state text is truncated exactly as the reference truncates it — Vela's upstream truncates where D3's does not, so the issue's "without silent truncation" phrasing does not apply here, and the guardrail risk is documented. Slice-scoped differences: an 8 MiB body cap and upstream's minimal/healthbody.Test Plan
CPU-side, from the repository root:
PYTHONPATH=src .venv/bin/python -m pytest tests/vela -q PYTHONPATH=src .venv/bin/python -m compileall -q src/models/vela src/frontend/vela.py tests/vela .venv/bin/python recipe/vela/test_compare_reference.py PYTHONPATH=src .venv/bin/python -m pytest tests/d3 tests/valen tests/frontend -q cargo fmt --all --check cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings cargo test --workspace --locked --features omni-laya/serve cargo build --workspace --release --locked --features omni-laya/serve .venv/bin/mkdocs build --strictAuthor-side on CPU FP32 — the reference oracle setting, no GPU needed:
System1-Omni Version / Commit:
2f94be5(2f94be51713e1893315804096e2cc5d6bae657a6) onmodel/vela2-reference-serving, cut frommain@e66ff34; all checks above pass at this head.Test Result
tests/velatests, compileall, thecompare_referenceregression,mkdocs build --strictand the four Rust gates pass at the head; the d3/valen/frontend suites pass unchanged. CI runs the Rust gates andtests/benchmarksonly —tests/velais run manually per the recipe.d6f03aa9…: the package gate, FP32 load and warmup complete before the port binds;/health/ready/v1/modelsreturn the upstream bodies verbatim.domain=healthat 0.7272 / 0.2171 / 0.0557, confidence 0.5101; PERSON "Tom Baker" at 8–17, the e-mail at 19–40, the unsupported "up to 6 grams" at 16–29; thresholds echo 0.46046 from the PII length rule and 0.55;usage.input_tokens471.billing0.99900 andshipping0.99228 overlogin0.00175; the CJK fixture returns code-point offsets, 张伟 at 2–4; the score fixture answerstone0.7845 (confidence 0.2319) andurgency0.4324 (confidence 0.5419) withusage.input_tokens61; invalid span and set questions reject the whole request with the upstream 422 triples; the 255-option schema rejects with 413too_long.vela2_serve.pyapp over one shared engine, nine cases (the four example requests, four 200s; five synthetic rejections): exact match on status, body and JSON key order; the four oracle fixtures are committed with this PR, and the three from the pre-f7f33a9run reproduce byte-identically. The same four requests through the Rust frontend are byte-identical.Not covered: the hybrid members 0.8B/4B/9B, native Rust/CUDA execution, GPU bf16-autocast serving, the ONNX backend, concurrent requests, #61 ratification.
Demo / evidence
recipe/vela/reference.md; full-precision responses are frozen intests/vela/fixtures/oracle-*.json, exported by the same run that produced the parity evidence.Self-review
Before marking this PR ready for review or requesting maintainer review, complete the self-review checklist.
Keep the PR in draft while this work is incomplete.
For agent assistance, use the optional precheck-pr skill.