Repository navigation
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: f2d9b7c112
ℹ️ 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 #152 "models: make encoder structure configurable, ModernBERT core, Vela 0.3B profile"
Local reviewer report. Not posted to GitHub; no approval or merge judgment implied. Author: MrDongsls. NON-draft.
Verdict
No P0/P1 findings. 0 P0, 0 P1, 3 P2, 4 P3. The code migration itself is faithful and unusually well evidenced at the CPU layer (golden plan fixtures + a launch-trace parity test for the Laya migration; exact census equality for the Vela checkpoint; a dumped-reference YaRN gate). The material problems are process and coordination: the CONTRIBUTING large-code-change packaging requirements are only partially met in the PR body, the PR's copy of the Vela Python worker has diverged from the review-fixed head of #151 it says it builds on, and the entire device half (8+ new _h768 kernels, the Vela executor, the Laya bit-identity rerun) is compile-verified only — honestly disclosed by the author, but unverified.
Provenance
- Repository
ThinkFlowLab/system1-omni(ID 1386454853). PR #152 basemain, base SHAe66ff34996760a84ad56690f0ae4cf434e669e65(live, fetched 2026-10-20). Head repoMrDongsls/system1-omni, head SHA0d9af2a77de4f4b075f687987a1dff8c8c932aaf(live, fetched viagit fetch origin pull/152/head --forcebefore reporting). 17 commits; GitHub reports 90 files, +11095/−505; independently verified:git diff --stat origin/main...origin/pr/152= 90 files, 11095 insertions, 505 deletions. - Skill:
.agents/skills/system1-omni-review/SKILL.mdat the local main checkout (canonical, published in-repo). SHA-256 of bytes read:58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108— matches the pinned value. References actually read:docs/architecture.md,.agents/skills/system1-omni-review/references/repository-contracts.md,CONTRIBUTING.md, and the docs the PR adds/updates. - Host: macOS arm64, no Rust toolchain, no GPU. Python:
/Users/hsliu/miniconda3/bin/python. PR code was reviewed in a detached worktree at the head SHA under/tmp(git worktree add --detach); the repository working tree was not modified.
Findings
P2-1 — Large-code-change packaging requirements only partially met (readiness gap, not correctness)
- Where: PR description (body) vs
CONTRIBUTING.md§"Large code changes". Authored code is far above the 3,000-line threshold. - Trigger/consequence: CONTRIBUTING requires, for >3,000 changed code lines, that the PR description carry (a) the code-line count reported separately from total diff size, (b) a split rationale where the change stays together, and (c) a component map with suggested review order, plus per-area validation. The body has the self-review checklist (all four boxes checked), a good validation summary, and one sentence of stacking order ("Builds on the unmerged reference worker PR #151, which should merge first") — but no authored-vs-total count at all, no component map, no review order, and no explicit why-not-split rationale for a PR that bundles five workstreams (family core crate, Laya migration, qwen3_5 structure/gate split, dual-profile CUDA bundle ABI 1→2, Vela profile + the whole #151 worker).
- Independent counts (mine, from
git diff --numstat origin/main...origin/pr/152, classified per CONTRIBUTING): authored code ≈ 7,942 lines (Rust/Python/CUDA sources, root-tree tests, build/validation scripts; excludes docs 533, generated/static fixtures 3,110 —Cargo.lock,tests/laya/data/*.txt,tests/vela/fixtures/*.json,tests/vela/native/data/*.json, recipe example JSONs — and 15 lines of config JSON/txt). Total changed lines 11,600. Nothing in the body states either number. - The commit series itself is well factored (17 focused commits); this is a description/readiness gap. Per the review skill: size is not a correctness finding; missing preparation is a readiness gap.
P2-2 — This PR's copy of the Vela worker has diverged from #151's review-fixed head; merge order can silently revert review fixes
- Where: 14 files, including
src/models/vela/engine.py,recipe/vela/compare_reference.py,tests/vela/test_engine_manifest.py,tests/vela/test_postprocess.py,tests/vela/test_worker_server.py,recipe/vela/example-request-score.json(present in #151, absent here). - Personally verified: PR #151 head is
f7f33a973a0440d82860aef6fb27b0945a939091, whose extra commitf7f33a9("vela: close the package-gate gaps the review flagged", 6 files, +128/−21) is not an ancestor of this head.git diff origin/pr/151 origin/pr/152over the shared worker files shows substantive differences in both directions: #152 independently carries its own package-gate fix (1436bae, "refuse every importable suffix in package verification", different hunk set from #151's), adds endpoint-parity machinery and oracle provenance, and drops theexample-request-scorecase that #151 keeps; #151 hassame_result()-based order-sensitive comparison where #152 uses direct==. - Trigger/consequence: the body says #151 "should merge first". Whichever merges second cannot fast-forward: a careless conflict resolution on #152 would drop #151's reviewed package-gate fixes, or vice versa; the two copies of
engine.py's gate are not the same patch. Both PRs are non-draft. Needs an explicit rebase of #152 onto #151 with a stated reconciliation of the gate fix and the fixture-set difference (5 vs 8 comparison cases are described differently in the two PRs'docs/supported-models.mdrows).
P2-3 — Device half is compile-verified only: the new kernels, the Vela executor and the Laya bit-identity rerun have never executed
- Where:
src/backends/cuda/kernels/model_ops.cu(templatedembed_norm, newlaya_embed_h768),src/backends/cuda/tools/export.py(vela_exports:qkv_h768,out_h768,geglu_h768,down_h768,addln_h768,attn_full_h768,attn_local_h768,rope_h768withl_max=8192wrappers),src/models/vela/native/src/executor.rs(whole file),tests/vela/native/executor.rs(device-gated, skips withoutVELA_NATIVE_CHECKPOINT/VELA_NATIVE_LIBRARY). - The author discloses this accurately ("The sm_90a bundle is compile-verified only… Execution is unverified because no Hopper GPU was available and the local GPU is sm_120"), and CI's
cuda-contractis the weight-free contract checker. Per the review skill this is an evidence gap, not PASS: template instantiation (embed_norm<768>,stats<HalfLoad, 768>withD/4=192elements over a 128-thread stride), SASS presence, and the executor's buffer/launch path against a real library are unvalidated. The promised Laya device bit-identity rerun after the ABI 1→2 rebuild is also pending. Nothing here authorizes treating the Vela native path as working; the PR itself correctly keeps serving on the Python worker.
P3-1 — "every existing suite passes unedited" overstates: the existing Laya tests were edited
- Where: PR body ("The recorded golden plan fixtures still match byte for byte and every existing suite passes unedited") vs
tests/laya/batch.rs:26,40,43,48,59(packgained a vocab argument),tests/laya/artifacts.rs:52,67(fixture manifest"abi": 1→"abi": 2),tests/laya/model_cache.rs:26-27,83-84,151-152,164,175,tests/laya/model_dispatch.rs:26,51-56,tests/laya/model_fixture.rs:60-61(signature/structure threading). - The edits are mechanical (explicit
structure/limitsarguments, the real ABI bump) and the behavioral evidence is elsewhere and strong —tests/laya/model_dispatch.rs::prepared_plan_preserves_original_launch_trace_for_all_shape_selectorscross-checks the generated plan against the frozen CUDA-ABI launch trace, andtests/laya/plan.rspins the rendered plans totests/laya/data/encoder-plan*.txt. But "passes unedited" is factually wrong about the test files; the claim should name the mechanical updates.
P3-2 — Family adapter recognizes but silently ignores rope_theta
- Where:
src/models/modernbert/src/adapter.rs:171-172(onlyglobal_rope_theta/local_rope_thetaare read; absence yields the 10000.0 default withFieldOrigin::Default("HF ModernBERT rope_theta default")at line 159) vs line 314 ("rope_theta"listed as a known, i.e. consumed, key inunrecognized_keysaccounting). - Trigger/consequence: a HF-style ModernBERT config that carries its theta only in the generic
rope_thetakey parses as theta=10000.0 with provenance claiming "default" — the config's explicit value is ignored without any signal. Fails closed today: both registered profiles compare parsed vs frozen structures field-by-field (RegisteredProfile::admit,src/models/modernbert/src/gate.rs), so a wrong theta is rejected at startup rather than served. Consequence is limited to confusing provenance and a rejected-but-misdiagnosed config; still worth either reading the key as a fallback or reporting it inunrecognized_keys.
P3-3 — Vela executor maps the Types scratch slot onto the token-id buffer instead of failing
- Where:
src/models/vela/native/src/executor.rs:83(ScratchSlot::Types => &self.idsinScratch::buffer). - Trigger/consequence: unreachable today (the pinned
vela-2.0-0.3bprofile admits onlytype_embedding_rows: None, so the plan never emits thetypestep — asserted intests/vela/native/profile.rs). But a future type-carrying ModernBERT member on this executor would silently feed token ids where type ids belong rather than failing loudly. Preferanyhow::bail!("type scratch not supported by this profile").
P3-4 — Author-reported CPU results are pinned to 6ee98f8, three commits behind head 0d9af2a
- Where: PR body Test Plan ("
6ee98f8… on unmerged2812144") vs head0d9af2a(1436baepackage-suffix gate,1e8f4291token-id validation,0d9af2azero-interval rejection came after). - Consequence: the workspace-test pass and the YaRN deltas are attested at a SHA that lacks the last three fixes. CI at head (below) covers the Rust workspace, and I re-ran the Python suites at head myself (below), so the gap is narrow — but the body should be re-attested at the final head SHA. Also minor body hygiene: the "Required human contributor answers" template block appears twice (the second copy's Q3 answer is empty; first copy contains the typo "adapter adn inventory work").
Notes (not findings)
- Configurability without breakage — verified. The registered
laya-0.3.20profile (src/models/laya/src/profile.rs) reproduces main's hardcoded geometry exactly (1024/2624/28 layers/16 heads/head_dim 64, i%3==0 full schedule, local window 128, thetas 160000/10000default, vocab 50368, eps 1e-5, type rows 3, max_positions 512) and the same request envelope (power-of-two batch ≤16, len 16..=512 step 16).Config::loadnow parses →profile.admit()→ derivesEncoderConfigfrom the structure; admission is field-by-field equality with per-field diff messages, so a profile cannot inject arbitrary structure — a profile is a frozen expectation, the checkpoint config is the candidate, and anything else is rejected at startup. The migration keeps the exact kernel sequence and buffer sizes (m * 4 * D * 2≡m * 4096 * 2at D=1024); golden plan fixtures added at the plan-split commit (d5d9373) and unchanged since pin the rendered plans. - qwen3_5 split is behavior-preserving.
check_abi6(src/models/qwen3_5/native/src/structure.rs) replicates all 13 of main'sConfig::loadgate checks verbatim (silu, output gate, no attention bias, default rope, conv kernel 4, 256/128/128 head dims, rotary_half 32, finite theta, interleaved mrope, sections-sum, max_positions ≤ 2^24, head divisibility, hidden %8), parse→gate ordering and even the layer-count check's position are preserved, andconst _: () = assert!(ABI_VERSION == crate::structure::ABI_VERSION)(src/models/qwen3_5/native/src/cuda.rs:17) pins the Rust-side ABI to the loader's ABI-6 constant. - ModernBERT core is a structural implementation, not a weight port — by design.
omni-modernbertholds structure/geometry/plan/gate only (no CUDA, no weights, no tokenizer); checkpoint identity is enforced at the model-crate layer: the Vela executor validates the safetensors inventory against the structure (Weights::validate, exact 134+18 name/shape/dtype census equality, all-F32 storage asserted) and the profile pins every geometry field. This is infrastructure for #137's native slices; per-task-head execution deliberately stays out. - Vela 0.3B profile vs the real checkpoint. The canonical structure (hidden 768, intermediate 1152, 22 layers, 12 heads, vocab 256008, yarn factor 4 / original 8192 / β 32,1 / truncate, theta 160000 both kinds, max_positions 32768,
sans_pos) is derived at runtime from the bundle'sencoder_configand admitted against the frozen profile; the checked-intests/vela/native/data/pinned-checkpoint-census.json(134 encoder + 18 head tensors) andvela-config.jsonare the author-recorded header/config of pinned revisiond6f03aa9baca4f9017fa8f8e9c2764d3bed3f409. The YaRN generator is gated against a dumpedtransformers==5.19.0reference (asserted ≤1e-7 inverse-freq, ≤1e-4 served-window cos/sin; author-observed 5.8e-5). I could not independently verify the census against the real HF package (no model downloads on this host), so checkpoint fidelity rests on author-recorded fixtures — preserved in-repo, which is the right call. Head-side tensors (cls MLP, readout projections w_e..w_t, log_tau scalars) are accounted for but intentionally not executed; storage classes mirror the Laya baseline (embedding f16, GEMMs bf16, norms/scalars/biases f32) and are declared as the numerics choice to be judged by the later parity harness. - Architecture/ownership respected. Shared backend changes are minimal and backstop-only:
src/backends/cuda/src/lib.rswidens the shared launch guard 512→8192 (only consumers are laya — which re-checks the profile envelope before every launch — and the new vela crate; qwen3_5 uses its own ABI-6 loader), per-kernel wrappers keep their own bounds (512 for the 1024 symbols, 8192 for_h768), and the originalgeglukeeps its pinned lowering-hash guard while only the newgeglu_h768uses the unpinned register-patch path. Kernel eps stays hardcoded 1e-5 in CUDA while the structure carriesnorm_eps— consistent because profile admission pins eps; a future different-eps profile must treat that as a kernel decision.src/frontend/vela.pyfollows the establishedsrc/frontend/<model>.pyPython-worker convention (cua_s1.py, d3.py, valen.py at main); the Rust transport is untouched except a README kind-list update. Tests all live in the roottests/tree with explicit[[test]]paths per CONTRIBUTING. - Docs/registration complete. README supported-models row,
docs/supported-models.mdrow (correctly marked author-run/unverified for GPU), mkdocs nav entry,recipe/README.mdentry,src/models/vela/README.md,src/models/vela/native/README.md,src/models/modernbert/README.mdall present and consistent with the code I read;mkdocs build --strictpasses locally (exit 0). - Artifact hygiene. All 90 files are sources, tests, docs, or declared maintained fixtures/pinned evidence; the in-PR
specs/planning series was added and removed (62e3dbc) leaving nothing behind; no run output or editor debris found. The large fixture files (census 1006 lines, yarn-reference 1010 lines, three oracle fixtures) are maintained comparison baselines consumed by named tests — keep.
Commands run and results
Personally run (host: /Users/hsliu/miniconda3/bin/python, worktree at head 0d9af2a, PYTHONPATH=src):
| Command | Result |
|---|---|
pytest tests/vela -q |
118 passed in 10.9s |
python recipe/vela/test_compare_reference.py |
21/21 checks ok (script, not pytest) |
pytest tests/d3 -q (existing) |
53 passed |
pytest tests/valen -q (existing) |
29 passed |
pytest tests/benchmarks -q (existing) |
58 passed |
pytest tests/cuda/test_laya_build.py -q (existing) |
8 passed |
pytest tests/laya/test_contract.py -q (existing) |
20 skipped (checkpoint-gated by design) |
pytest tests/laya/test_bench.py -q (existing) |
24 passed / 7 failed — identical at origin/main in a clean worktree (ModuleNotFoundError: No module named 'laya'; pre-existing environment gap, not a PR regression) |
pytest tests/laya/test_worker.py -q (existing) |
5 passed / 41 failed / 21 errors — identical at origin/main (same missing module; pre-existing) |
mkdocs build --strict |
exit 0 |
shasum -a 256 .agents/skills/system1-omni-review/SKILL.md |
58be3bc6…dde108 (matches pin) |
git diff --stat origin/main...origin/pr/152 |
90 files, +11095/−505 (matches GitHub) |
Not runnable on this host (recorded, not PASS): cargo fmt/clippy/test/build — no Rust toolchain; covered at head by the CI rust and build jobs. Source inspected: the full diff (90 files), with close reads of the modernbert crate (all 6 files), laya profile/config/plan/model + the model.rs migration diff, qwen3_5 structure.rs/model.rs/cuda.rs diffs, vela native (all 7 files), CUDA .cu/export diffs, both new Rust test suites, the Vela Python worker/test surface, and all docs. Author-reported (not independently verifiable here): the checkpoint census and YaRN reference dump provenance, the 73-target workspace pass at 6ee98f8, sm_90a compile/SASS evidence.
CI observed at this exact head
gh pr checks 152 + check-runs API for 0d9af2a77de4f4b075f687987a1dff8c8c932aaf (all completed on that SHA): build ✅, rust ✅ (1m59s), cuda-contract ✅, benchmarks ✅, deploy ⏭ skipped. (One transient network timeout on the first gh call; retried successfully.)
Template readiness (post-#138 gate)
Present: the four required human answers (duplicated block, see P3-4), the "is a PR needed" answer, purpose, test plan with commands, test result with explicit unverified-GPU disclosure, demo/evidence section, and all four self-review boxes checked by the author. Missing against the large-change additions: authored/total counts, component map, review order, explicit split rationale (P2-1). The gate is formally satisfied; the large-change packaging is not.
Material unverified checks
- Any device execution of the
_h768kernels, the Vela encoder path, or the post-ABI-2 Laya bit-identity rerun (author-disclosed; needs an sm_90a host per the body's plan). - The pinned-checkpoint census and
transformers==5.19.0YaRN reference against the realvllm-sr/Vela-2.0-0.3B@d6f03aa9package (no downloads authorized on this host; author-recorded fixtures only). - Rust fmt/clippy/workspace/release on this host (no toolchain; CI green at head covers the workspace).
docs/supported-models.mdCPU-parity claims for the Vela worker (author-run on WSL2; evidence said to live in the integration PR — i.e. this one — and not re-derivable here).- Rebase outcome of #152 onto #151's
f7f33a9(P2-2): which package-gate patch and which comparison-fixture set survive.
The implementation series for docs/rfc/model-parameter-configuration.md, charter step 3 of the Vela 2.0 order (the 0.3B reference worker landed as 2812144; every native Vela slice waits on this). Five specs on branch model/configurable-base-model: - 01 ModernBERT family IR, source adapters, capability gate (pure Rust crate omni-modernbert, provenance, StructureIdentity) - 02 Laya native migrated onto the core, zero behavior change, plan-fixture bit-identity, task head stays Laya-side - 03 Vela 0.3B as the second ModernBERT profile: model-local weight mapping, YaRN rope tables fixture-grounded, kernel bundle regeneration with the laya ABI 1->2 decision, parity harness vs the pinned package - 04 task-head ownership boundary: final-hidden-at-positions contract, heads stay in the vela crate - 05 Qwen hybrid config/capability migration: shadow-normalizing facade, QwenAbi6 gate as data, PrefixLayout derivation, loader seams (prefix candidates + per-tensor dtype policy) for the hybrid consumer; ABI 6 untouched Stances recorded on the RFC's open items: Rust-internal IR only, explicit adapter selection, frozen ABI-6 schedule set, bundle fields in spec 03. Evidence base: laya/qwen source at 2812144, the pinned 0.3B package copy, research 017/019, the locked charter.
A pure-Rust family layer for ModernBERT encoders with no device dependencies, the first half of the model-parameter RFC's phase 2: - structure: one ModernBertStructure with family invariants (heads * head_dim == hidden, schedule length, bias-free gelu, rope/yarn bounds) and per-field provenance; canonical sorted-key JSON for identity digests - adapters: the Laya two-file bundle (explicit layer_types, per-kind rope_parameters) and any single-object HF ModernBERT config (schedule from global_attn_every_n_layers or explicit layer_types, global/local thetas, yarn rope_scaling, optional type vocab); unknown top-level keys are recorded, never guessed - geometry: derived tensor shapes (the 2*intermediate up-projection is derived, not semantic), the canonical encoder tensor inventory, rotary table sizing, and the (batch, len) encoder scratch formula - gate: backend limits and the registered laya-0.3.20 profile admitting structures with field-diff errors, plus StructureIdentity (sha256 over schema version, family, structure, backend, ABI) for future cache keys - plan: the named-kernel encoder plan (embed, per-layer qkv/rope/ attn/out/addln/geglu/down, final norm fused into the last layer, optional type add) with sequence anchors; symbol resolution stays with consuming model crates Fifteen CPU tests: adapter provenance and rejection matrices for both sources, invariant failures naming the field, profile admit/reject with actual-vs-expected diffs, request-envelope bounds, identity stability and sensitivity, inventory counts for the two known profiles (Laya 1024/28-layer and the pinned Vela-2.0-0.3B encoder_config 768/22-layer with yarn factor 4), and plan fixtures for both schedules. No consumer changes; Laya migrates in the next step. Signed-off-by: Mr.Dong <1042542469@qq.com>
The encoder-plus-head plan moves from prepare_encoder into plan::plan_encoder, a pure function of the original_rope flag that emits kernel names and buffer roles with no CUDA or weight pointers. prepare_encoder becomes a resolver that maps each planned step to a loaded kernel symbol and device pointer, preserving the exact step sequence. The full sequence for both rope variants is recorded as golden fixtures (251 steps: embed, 28 layers with full attention every third layer, type add, two head layers, and the debug dumps) and pinned by a CPU test, so any later change to the plan construction has to survive a byte comparison against today's behavior. No execution change: same kernels, same order, same arguments. Signed-off-by: Mr.Dong <1042542469@qq.com>
The native executor consumes the family layer added two commits ago; behavior is unchanged and every existing suite stays green. - config: Config::load becomes a facade over the Laya bundle adapter and the registered laya-0.3.20 profile, projecting the admitted structure onto the public EncoderConfig fields; the agent bundle checks stay in the Laya crate. Config carries the admitted structure for everything downstream. - plan: plan_encoder now generates the encoder half from the family core (structure-driven layer count, schedule and widths), hooks this bundle's debug dumps to the sequence anchors, and appends the Laya-owned task-head plan. The recorded golden fixtures still match byte for byte, so the executed sequence is provably identical. - weights: the encoder tensor inventory comes from the family geometry; the task head keeps its own inventory (head layers, scorer, action head, legacy temperature buffer). - model: workspace scratch sizes derive from the structure (ids, lens, x, y, qkv, o, geglu gate) with the task-side buffers appended; the rotary table size check and the vocab bound in infer come from the admitted structure instead of literals. - packing: the vocabulary bound is a pack() argument supplied by the processor from the admitted structure; the pad token stays a tokenizer constant. The 1024 hidden width now survives only where it is task geometry (head ffn widths, scorer and action-head dims) or the task inventory. The CUDA bundle, its ABI and the storage-dtype policy are untouched. Signed-off-by: Mr.Dong <1042542469@qq.com>
The family crate gains a structure layer that separates what a checkpoint declares from what the kernels can run, the qwen-side half of the model-parameter RFC's phase 5. No gate constant, kernel, or ABI changes; the accepted structure set is frozen and fixture-proven identical. - structure::read_structure parses config.json (root or text_config) into QwenHybridStructure with per-field provenance, including the source-declared behavior fields the gate pins (hidden_act, output gate, attention bias, rope type, conv kernel, mrope interleave); parsing judges nothing - structure::check_abi6 is the frozen kernel capability set as typed checks with the original error surface: silu, output gate, no bias, default rope, conv kernel 4, head dims 256/128/128, 32 rotary dims, interleaved mrope summing to 32, position bounds, head divisibility, hidden % 8 - Config::load becomes a facade over adapter plus gate and projects the admitted structure onto its unchanged public fields; the new Config::from_structure is the seam a consumer with its own bundle layout (a Vela hybrid member whose Qwen fields live at backbone_config) uses instead of reading a directory - structure::PrefixLayout derives the per-layer prefix-state byte sizes (full-attention K/V rows, GDN float32 [H,K,V] states, conv tails) from the structure, CPU-checkable against recorded numbers - structure::structure_identity digests schema version, family, backend, ABI 6 and the canonical structure; prefix-cache records must carry it and re-capture on any change Tests: adapter provenance and broken-source rejections, the frozen reject matrix (15 single-field violations), the three Vela hybrid backbone geometries (1024/3584/24, 2560/9216/32, 4096/12288/32) passing the gate as pure CPU fixtures, the open-jev 27B checkpoint passing unchanged through the facade, prefix-layout numbers, and identity stability. The weight-loader dtype/prefix seams for the Vela consumer stay with that consumer's slice; nothing here changes the arena or the ABI. Signed-off-by: Mr.Dong <1042542469@qq.com>
The 0.3B becomes the second ModernBERT profile on the family core, the CPU-verifiable half of the RFC's phase 3. The checkpoint's own files are the evidence base throughout. - config: reads the pinned vela2-unified bundle root and parses its encoder_config through the family adapter; foreign roots or encoder model types are rejected before any structure exists - profile: the registered vela-2.0-0.3b structure (768/1152, 22 layers with full attention every third, 12 heads, both rope thetas 160000, yarn factor 4 from 8192, vocab 256008, no type embedding, 32768 positions) against the regenerated-bundle target limits — hidden 768, yarn tables, batches to 4, lengths to 8192 - weights: whole-checkpoint accounting against the structure — the encoder inventory comes from the family geometry and the 18 head-side tensors (readout projections, classifier MLP, calibration scalars) are recorded from the pinned safetensors header census, which is committed as a fixture; missing, extra, shape and dtype drift all fail naming both sides. Storage classes mirror the serving conventions (embedding f16, norms f32, projections bf16) - rope: the YaRN inverse frequencies and attention scaling exactly as the reference runtime computes them (transformers 5.19.0 yarn initialization, f64 internally, narrowed to f32), with the mscale baked into the cos/sin tables the way the reference does. A fixture dumped from the pinned transformers over the pinned encoder_config gates the generator: inverse frequencies to 1e-7, the scaling to 1e-12, cos/sin to 1e-4 inside the served 8192 window and 5e-4 over the full 32768-extension (f32-power ulp noise amplified by position; measured, not assumed) Census equality is asserted against the family geometry: the pinned checkpoint's 152 tensors are exactly the 134 derived encoder tensors plus the 18 recorded head tensors, names and shapes matching one for one. The encoder execution, kernel bundle regeneration and the laya ABI bump stay fenced until a TileLang toolchain can build the bundle; no kernel or ABI change is in this commit. Signed-off-by: Mr.Dong <1042542469@qq.com>
The regenerated bundle serves two profiles of the encoder family in one library: the existing 1024-hidden Laya kernels under their unchanged names, and the Vela 2.0 0.3B geometry under _h768-suffixed symbols (qkv 2304x768, out 768x768, geglu 1152, down 768x1152, addln, full and sliding attention at 12 heads, rope at 12 heads). The 768 entries carry their own wrapper length bound of 8192 tokens; the Laya symbols keep their 512 bound exactly, so the existing profile's acceptance surface does not move. - model_ops.cu: the embedding LayerNorm becomes a width template with 1024 and 768 instantiations; the 1024 lowering keeps the pinned PyTorch-matching reduction order. laya_embed_h768 joins laya_embed - export.py: per-entry wrapper guards and the vela instantiation set; the GEGLU register patch stays sha-pinned for the Laya shape and removes the calls only when the 768 lowering emits them in the same accepted shape - abi_version 2 across laya.backend.json, build.py and export_tables (new symbols and a regenerated library); the Rust-side bundle validation and artifacts fixtures follow - omni-cuda: the Rust launch envelope widens to 8192 tokens; the per-symbol C guards remain the real capability check Verified locally without a Hopper device (this machine is sm_120 and cannot execute sm_90a code): TileLang lowering of all 22 kernels with the strict lowering checks passing, nvcc sm_90a compilation and linking of the full library, both embed_norm instantiations present in SASS, every _h768 symbol exported, the contract checker clean, and the CPU test suites green. Execution on Hopper and the Laya bit-identity regression run when the bundle is rebuilt there; the export must run with TILELANG_DEFAULT_TARGET set to cuda sm_90a on any non-Hopper host until the in-flight target pinning lands. Signed-off-by: Mr.Dong <1042542469@qq.com>
The executor (cuda feature) completes the profile slice: it admits the structure, loads the pinned checkpoint with the whole-file inventory check, uploads every encoder tensor under the serving storage classes (embedding f16, norms f32, projections bf16), uploads the zero-bias buffers and the YaRN cos/sin tables the rope generator produces, and resolves the family core plan against the bundle's _h768 symbols with the per-request encoder scratch from the family geometry. One sequence per call; the final hidden of the last layer is read from the post-final-norm working buffer in the bundle's bf16 precision, gathered at the caller's requested positions. A device-gated test runs the encoder end to end only when VELA_NATIVE_CHECKPOINT and VELA_NATIVE_LIBRARY point at a bundle and checkpoint on hardware the sm_90a library can execute on; it skips with a note everywhere else. Parity against the pinned CPU-FP32 reference, with the tolerance anchored to the upstream noise floor, runs with that same harness on a Hopper host. Signed-off-by: Mr.Dong <1042542469@qq.com>
The durable record is the code, its tests and the commit messages; the local planning docs stay out of the repository. Signed-off-by: Mr.Dong <1042542469@qq.com>
The package gate now refuses any directory carrying importable code, not just stray .py files, closing the namespace-package shadowing path into the pinned runtime. compare_reference.py compares the worker's verbatim /health, /ready, /v1/models and 401 bodies against the live reference app, runs an eight-case default comparison (the three recipe examples plus five synthetic rejections), and --export freezes a passing run's request, response and predict-level answers with provenance as the oracle fixture for the later native slices. Worker tests, the recipe docs and the support-matrix row follow. Signed-off-by: Mr.Dong <1042542469@qq.com>
The family crate keeps only the generic single-object config adapter; the Laya two-file dialect and its registered laya-0.3.20 profile move into omni-laya, matching the model-owned loading rule. HF-side keys the Laya worker never consumed (max_position_embeddings, head_dim) are recorded as unrecognized instead of silently gated. The vela crate keeps its registered profile beside its census inventory, narrows the storage classes so the learned scalars and head biases stay f32 (rounding them to bf16 would surface as unexplained parity drift), and shares one pinned config fixture across its tests. Crate READMEs and the workspace member list follow. Signed-off-by: Mr.Dong <1042542469@qq.com>
cuda.rs pins its ABI_VERSION to structure::ABI_VERSION at compile time, so the identity records written from the structure side cannot silently disagree with the bundle the loader accepts. Signed-off-by: Mr.Dong <1042542469@qq.com>
Signed-off-by: Mr.Dong <1042542469@qq.com>
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>
A stock HF ModernBERT config may carry its theta only as the generic rope_theta; the adapter listed that key as consumed but never read it, so such a config parsed as the 10000.0 default while provenance claimed a default, and the profile gate then rejected the structure with a misleading diff. The generic key now fills both kinds when the per-kind key is absent, with source provenance naming rope_theta; the per-kind keys keep precedence. Signed-off-by: Mr.Dong <1042542469@qq.com>
The executor mapped the Types scratch slot onto the token-id buffer. No admitted profile emits the type step today (the 0.3B profile pins type_embedding_rows to None), but a future type-carrying member on this executor would have silently fed token ids where type ids belong; the slot now fails loudly as a plan/profile mismatch. Signed-off-by: Mr.Dong <1042542469@qq.com>
0d9af2a to
dcc6dac
Compare
|
@hsliuustc0106 thanks — everything actionable is addressed at
CPU suites at head: fmt/clippy clean, 73 workspace targets 0 failures, feature suites pass, contract checker 0 errors (its ABI-define warning is unchanged at base), pytest 121, compare script ok. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Incremental re-check at new head dcc6dac03d: three new commits since my review at 0d9af2a77d; the prior 17-commit series is patch-identical through the rewrite (git range-diff all =, named refs, cat-file verified). The body now also carries the large-change packaging my review required: authored ≈7,562 / total 92 files +11,344/−505 stated separately with the exclusion convention, the "why it stays one PR" split rationale, and a component map with review order — P2-1 fixed.
The other two P2s and both P3s from my review, verified in the diffs:
- P2-2 (divergence from #151) — fixed:
f013512reconciles the worker with the #151 review head while keeping this branch's stronger every-importable-suffix package gate (the reconciliation message documents which side won and why); the #151-vs-#152 worker diff no longer carries competing gate patches. - P3 (rope_theta known-but-ignored) — fixed:
6bfbd71falls back to the generic HF ModernBERT key withFieldOriginprovenance recorded, instead of listing the field as known while silently ignoring it. - P3 (ScratchSlot::Types aliasing the ids buffer) — fixed:
dcc6dacmakesbuffer()returnResultand refuse the type slot instead of silently aliasing — fail-loud rather than fail-wrong. - P2-3 (device half never executed) — remains open, author-disclosed: the 8 exported
_h768kernels, the Vela executor, and the Laya bit-identity rerun are still compile-verified only; no GPU access exists to close it and the body discloses this.
Also reviewed in the delta: f828d17 (reject a zero global attention interval) and 3426893 (assert structure ABI matches the loaded library) are within the rewritten-series verification; the new tests in the 13-file delta cover the rope fallback and the scratch-slot refusal.
Locally run at this head (previous pass): 118 vela + 53 d3 + 29 valen + 58 benchmarks tests pass; mkdocs build --strict exit 0. Fork CI at this exact head: rust/build/cuda-contract/benchmarks green.
One actionable finding remains — the author-disclosed device-execution gap (P2-3), which cannot close without GPU access. Verdict: COMMENTED (single remaining item, disclosed). When device qualification lands (here or in a follow-up), this PR is approval-ready on the record as it stands.
Main moved the qwen3_5 CUDA ABI 6->8 with the JEMM stack: take main's ABI_VERSION and follow it on the structure side (ABI_VERSION 8, check_abi6 -> check_abi), keeping the compile-time binding between the loader and the identity records. Workspace members, qwen3_5 deps and test targets are unions of both sides; nothing else is touched.
822b17e to
5745213
Compare
hsliuustc0106
left a comment
There was a problem hiding this comment.
COMMENTED at 5745213ce44ba8b127706450d2db07476597cee0
Incremental re-check at new head 5745213ce. Prior review: COMMENTED at dcc6dac03d5ea29df3a9e61961172096fff90d70 (2026-10-20). Head pin verified: git rev-parse origin/pr/152 = 5745213ce44ba8b127706450d2db07476597cee0 = gh pr view 152 --json headRefOid. Non-draft; title unchanged.
Delta isolation
git log --first-parent dcc6dac03..origin/pr/152 contains exactly one commit: 5745213c "Merge upstream/main into model/configurable-base-model" (Mr.Dong, 2026-10-22). Zero new authored commits. Parents: A = dcc6dac03 (my previously reviewed head, byte-for-byte — 5745213c^1 == dcc6dac03), B = d577bb82 (main side).
Candidates named for this pass — 12847a4a (test(bench) Score fixtures), 260075d4 (docs(bench) Decider score precision), 9168935c (fix(bench) executable permission) — are all in origin/main history (git log origin/main positions 3/4/25): main-side content absorbed by the merge, out of scope except for their effect on conflict resolutions (none — they touch benchmarks, not the conflicted files).
Merge hygiene
NOT a pure auto-merge. git merge-tree --write-tree dcc6dac03 d577bb82 → auto tree ba3eb010 with content conflicts in Cargo.toml, src/models/qwen3_5/native/Cargo.toml, src/models/qwen3_5/native/src/cuda.rs. Actual merge tree d6e957ea differs from the auto tree in 6 files; I read every resolution hunk:
Cargo.toml: clean union of workspace members — branch addsmodernbert,vela/native; main addsjemm/native.src/models/qwen3_5/native/Cargo.toml: clean union — branch'ssha2,tempfiledev-dep andstructuretest registration + main'sserde deriveanddevice_featurestest registration. All four test targets resolve to files that exist in the merged tree.cuda.rs: main'sABI_VERSION = 8combined with the branch's compile-time cross-assertABI_VERSION == crate::structure::ABI_VERSION(now pins 8==8 — a future one-sided bump fails compilation).structure.rs/model.rs/tests/qwen3_5/structure.rs: branch's structure-gate content kept, renamedcheck_abi6→ version-neutralcheck_abiand pinnedABI_VERSION6 → 8 to match main's kernel ABI. Main never had acheck_abi(grep atd577bb82: no definition) — so no competing gate; the rename strips the version-suffixed name the bump invalidated.git grep check_abi6and ABI-6 constants over the merged tree: zero stale references.
Main's qwen3_5 evolution since the merge-base e66ff349 (+5,489 lines: vision, lora, image_preprocess, tests) is preserved — the merged Config and structure.rs carry mrope_section, image_token_id, partial_rotary_factor; the vision/lora files are taken wholesale from main. No main-side feature loss found.
The resolution's one semantic decision — the branch's frozen qwen3_5 gate now pins ABI 8 (main's bump came via 98d0465d, vision-features-resident) — is coherent and CI-verified (see below), but it is undocumented in the PR body; see P3 below.
Own-series integrity
Intact by construction: 5745213c^1 is byte-identical to dcc6dac03 (verified with rev-parse), so the previously reviewed 17-commit series was not rebased or rewritten — no range-diff needed; every own-series commit is patch-identical. Footprint vs main moved from the prior stated 92 files +11,344/−505 to 103 files +11,370/−724 (git diff origin/main..origin/pr/152 --stat). The change is explained by main advancing (bench fixes, jemm, qwen3_5 vision/lora) — the vs-main qwen3_5 delta now shows divergence (model.rs −161, kernels.rs −116) instead of pure addition. The kernels.rs −116 is not a resolution loss: the missing fixed_gemm_rows_do_not_depend_on_m test landed on main in 5ba6a3ba, after the merge's main-side parent d577bb82 (verified by grepping kernels.rs at both). Note: the PR is now one commit behind main (5ba6a3ba qwen3_5 GEMM handle); the next sync should pick it up.
Prior-finding status (re-verified against the merged tree, not copied forward)
- P2-3 (device-execution gap) — remains open, author-disclosed. The 8
_h768kernels are still present (executor.rs, 10 references; bundle described in body), the Vela executor and the Laya bit-identity rerun are still compile-verified only. Body keeps the disclosure: "The sm_90a bundle is compile-verified only… Execution is unverified… There is no GPU evidence yet." - P3 rope_theta known-but-ignored fix — intact: generic-HF fallback with
FieldOrigin::Default("HF ModernBERT rope_theta default")provenance atsrc/models/modernbert/src/adapter.rs:153-181. - P3 ScratchSlot::Types aliasing fix — intact:
buffer()returnsResultand the type slotbail!s — "type-embedding scratch is not supported by the admitted profile" (src/models/vela/native/src/executor.rs:79-92). - P2-2 #151 reconciliation — intact: the every-importable-suffix package gate survives in
src/models/vela/engine.py:57,150,169; body still documents the reconciliation split and the #151-must-merge-first ordering. - Template gate — SATISFIED: the body now opens with the four required human contributor answers plus the "is a PR needed" and bug-fix-evidence sections. This was the standing COMMENTED-vs-approval-eligible gate; it is now met. What keeps this at COMMENTED is only the open P2-3.
New finding
- P3 — PR body no longer describes the head content (
structure.rs:16,cuda.rs:15-19; body §Purpose qwen3_5 bullet and §Test Result). Trigger: merge5745213c. The body still says the qwen3_5 split makes "No gate, kernel or ABI-6 change" and pins all evidence "atdcc6dac", but the merge resolution changed the branch's gate from ABI-6/check_abi6to ABI-8/check_abi— a semantic alignment with main's kernel ABI that no author statement covers (e.g., whether the frozen capability set was re-checked against the ABI-8 kernel surface). Code-wise the direction is conservative (the gate rejects; the compile-time assert pins 8==8; the three Vela backbone + open_jev 27B acceptance fixtures pass in CI), so this is a documentation/attestation gap, not a correctness defect. Consequence: a reviewer reading the body would believe the branch still pins ABI 6.
Commands and results
git rev-parse origin/pr/152,gh pr view 152 --json headRefOid,isDraft,title— pin match, non-draft.git fetch origin main;git log --first-parent dcc6dac03..origin/pr/152— single merge, zero authored commits.git merge-tree --write-tree dcc6dac03 d577bb82vs5745213c^{tree}— differ; 6 files diffed and every hunk read (source inspected).git grepover merged tree: nocheck_abi6, no ABI-6 constants;Cargo.lockcontains all four new member packages (omni-jemm-native,omni-modernbert,omni-vela-native,omni-qwen3-5-native).git diff origin/main..origin/pr/152 --stat— 103 files, +11,370/−724; qwen3_5 sub-diff inspected for main-side feature loss (none found).gh pr checks 152— CI and Docs runs green; both runs'head_sha=5745213ce44ba8b127706450d2db07476597cee0(verified via API). Observed CI: rust job ran Format/Clippy/Test/Build, all success; cuda-contract and benchmarks success. So the resolution compiles — including the compile-time ABI assert — and workspace tests (the CPU structure-gate fixtures included) pass at the exact head.- Local at a worktree of
5745213c:PYTHONPATH=src python -m pytest tests/vela -q→ 121 passed in 7.82s (matches the body's claim). Worktree removed after.
Unverified checks (source inspected + CI-observed vs not)
- No cargo and no GPU on this host: full workspace
cargo fmt/clippy/testat the head rests on the fork CI run pinned to5745213ce(observed green), not local execution.mkdocs build --strictandpytest tests/benchmarkswere not rerun locally this pass — the resolution touches neither docs nor benchmarks, and the CI Docs/benchmarks jobs are green at the exact head; prior pass verified both locally atdcc6dac. - Device execution of the
_h768kernels, the Vela executor, and the Laya bit-identity rerun remain unverified anywhere (P2-3, author-disclosed). - Whether the frozen gate's capability set was re-derived against main's ABI-8 kernel surface is author-unstated (part of the P3 above); CPU acceptance fixtures and the compile-time assert are the current evidence.
Verdict
COMMENTED. The delta is a single, well-resolved merge of main: unions are clean, the ABI-8 alignment is coherent and CI-verified at the exact head, the own series is intact by construction, and all four prior fixes plus the template answers hold. Two items keep it from approval: the standing, author-disclosed P2-3 device-execution gap (needs a Hopper host), and the new P3 body-staleness — the body should be refreshed for the merge (re-pin evidence at 5745213ce, and state the gate's ABI-8 alignment and its rationale; optionally note the pending 5ba6a3ba sync). Once the device qualification lands, this PR is approval-ready on the record as it stands.
Main moved the qwen3_5 CUDA ABI 8->9 with the fixed-algorithm GEMM handle: take main's ABI_VERSION and follow it on the structure side (ABI_VERSION 9), keeping the compile-time binding between the loader and the identity records. A pure version re-pin: the checked capability set is unchanged, so the gate admits and rejects exactly what it did at ABI 8. Identity digests change with the recorded abi_version, so prefix-cache records are re-captured, per the identity contract. Nothing else is touched.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review verdict: COMMENTED at 80a30e11f87460e223cc18e479d7781c33839e60
Incremental re-check at new head 80a30e11f. Prior review: COMMENTED at 5745213ce44ba8b127706450d2db07476597cee0 (2026-10-22). Head pin verified: git rev-parse origin/pr/152 = 80a30e11f87460e223cc18e479d7781c33839e60 = gh pr view 152 --json headRefOid. Non-draft, OPEN, title unchanged.
Delta isolation
git log --first-parent 5745213ce..origin/pr/152 contains exactly one commit: 80a30e11 "Merge upstream/main into model/configurable-base-model" (Mr.Dong, 2026-10-22). Zero new authored commits. Parents: A = 5745213ce (my previously reviewed head, byte-identical — 80a30e11f^1 == 5745213ce), B = 5ba6a3ba6 = current main head. Delta vs the branch parent: 15 files, +224/−31.
Main-side commits entering
git rev-list 5745213ce..5ba6a3ba6 --no-merges → exactly one: 5ba6a3ba "qwen3_5: add a GEMM handle with one algorithm per weight shape (#98)" (Tianyao Wu). Parent of 5ba6a3ba is d577bb82 — the prior merge's main-side parent — so the merge-base is exact and the three-way merge is minimal. Read in full (git show, 13 files +219/−26):
- Adds
cs1_gemm_create_fixed(workspace_bytes, reference_m)toops.h/gemm.cu/theapi!table in the model crate'scuda.rs: an opt-in handle that pins one cuBLASLt algorithm per weight shape (N, K, ldy, fp32, bias), chosen at the reference M among algorithms withCUBLASLT_REDUCTION_SCHEME_NONE(verified after selection by readingSPLITK_NUM/REDUCTION_SCHEMEback). "The default handle is unchanged" — the defaultcs1_gemm_createpath and its heuristic behavior are untouched. - Bumps
CS1_ABI_VERSION8→9 (ops.h),abi_version8→9 (qwen3_5.backend.json),ABI_VERSION8→9 (model-cratecuda.rs). (The commit message says "Bump the ABI to 7" — stale relative to main's advancement; the actual diff bumps 8→9. Main-side history, out of scope.) - Adds the GPU-gated test
fixed_gemm_rows_do_not_depend_on_mintests/qwen3_5/kernels.rs(116 lines,#[ignore = "needs a GPU and CUA_S1_CUDA_LIB"]): 15 projection shapes across the 4B/9B/27B backbones, byte-for-byte row equality across 16 sub-M/at calls, NaN-canvas full-write check, and f64 tolerance on first/last rows. Docs/recipe ABI mentions updated.
Interaction with the branch's content: none beyond the version constants. The branch's qwen3_5 changes are the structure split (structure.rs gate + parsing) and do not touch GEMM call paths; the merged tree keeps main's kernel/executor content wholesale. The only coupling is the ABI version pin, which the author aligned (see below). The new fixed handle is additive and opt-in — no branch-side code calls cs1_gemm_create_fixed, and the branch's check_abi capability set (structure-field rejections) is orthogonal to algorithm selection. The body's claim "identity digests move with the recorded abi_version, so prefix-cache records are re-captured" checks out: structure_identity embeds abi_version: ABI_VERSION (structure.rs:425), so the bump changes digests by construction.
Merge hygiene
NOT a pure auto-merge. git merge-tree --write-tree 5745213ce 5ba6a3ba6 → auto tree 7c37f8b8 with one content conflict in src/models/qwen3_5/native/src/cuda.rs (docs/supported-models.md auto-merged). Actual tree 64ad081e differs from the auto tree in 3 files; I read every hunk:
cuda.rs: clean union — main'sABI_VERSION: u32 = 9combined with the branch's compile-time cross-assertconst _: () = assert!(ABI_VERSION == crate::structure::ABI_VERSION as u32)and its comment. The conflict was branch-side assert+8 vs main-side bare 9; taking 9 keeps the assert meaningful.structure.rs(branch-only file, manually edited on top of the auto result):ABI_VERSION: usize = 8→9plus four doc-comment updates ("frozen ABI-8" → "frozen ABI-9"). Required — without it the cross-assert fails compilation.tests/qwen3_5/structure.rs:assert_eq!(a.abi_version, 8)→9. Consistent.
This is exactly the prior merge's 6→8 pattern, now 8→9. Post-merge greps: no check_abi6, no ABI-6 code constants; no stale ABI-8 pins in src/tests (remaining ABI-8 mentions are source-pinned historical benchmark docs for the cua_s1/jemm backbones, and one pre-existing "legacy ABI-6 libraries" capability note in cuda/reference.rs:1 — informational, main-side). check_abi present at structure.rs:315; runtime ABI check at cuda.rs:342-343.
Main's 5ba6a3ba content is fully absorbed: tests/qwen3_5/kernels.rs, gemm.cu, ops.h, qwen3_5.backend.json in the PR tree are now identical to origin/main (empty sub-diff), and fixed_gemm_rows_do_not_depend_on_m is present on both sides.
Own-series integrity
Intact by construction: 80a30e11f^1 is byte-identical to 5745213ce, so the previously reviewed series was not rebased or rewritten; every own-series commit is patch-identical. Footprint vs main is back to 92 files +11,344/−505 (git diff origin/main..origin/pr/152 --stat) — identical to the pre-merge totals quoted in the body and at dcc6dac. The prior review's qwen3_5 divergence (model.rs −161, kernels.rs −116) is resolved: the kernels.rs divergence is gone (file now identical to main), and the remaining qwen3_5 sub-diff vs main is exactly the branch's intended split — structure.rs +428 (new), tests/qwen3_5/structure.rs +266 (new), model.rs −161 (content moved into the split), cuda.rs +4 (cross-assert), lib.rs +1, Cargo.toml +8. No main-side feature loss.
Prior-finding status (re-verified against the current body and tree)
- P3 (body staleness) — RESOLVED. The body was refreshed at merge
80a30e11: the Test Plan pins evidence at80a30e11; the stale "No gate, kernel or ABI-6 change" claim is replaced by an explicit narrative covering both merge alignments —5745213c's 6→8 re-pin (check_abi6→check_abi, cross-assert kept, "version re-pin only: the checked capability set is unchanged") and80a30e11's 8→9 re-pin for5ba6a3ba("another version re-pin, capability set unchanged; identity digests move with the recorded abi_version"). I verified the capability-set-unchanged claim against the tree: thecheck_abigate code differs between5745213ceand80a30e11fonly in the version constant and doc comments. The Test Result section also now reports the author's re-run at80a30e11(fmt/clippy clean, 83 workspace test targets 0 failures, contract checker 0 errors) — author-reported. - P2-3 (device-execution gap) — remains open, author-disclosed. Body still states: "The sm_90a bundle is compile-verified only… Execution is unverified because no Hopper GPU was available and the local GPU is sm_120… There is no GPU evidence yet." The 8
_h768kernels, the Vela executor device tests and the Laya bit-identity rerun remain compile-verified only. This keeps the verdict at COMMENTED regardless of everything else. - rope_theta known-but-ignored fix — intact: generic-HF
rope_thetafallback withFieldOrigin::Default("HF ModernBERT rope_theta default")provenance (src/models/modernbert/src/adapter.rs:153-181). - ScratchSlot::Types aliasing fix — intact:
buffer()returnsResultand the type slotbail!s — "type-embedding scratch is not supported by the admitted profile" (src/models/vela/native/src/executor.rs:79-92). - #151 reconciliation — intact: the every-importable-suffix package gate survives in
src/models/vela/engine.py(IMPORTABLE_SUFFIXES = SOURCE+BYTECODE+EXTENSION, applied to unexpected files and__init__-bearing dirs in the package root). - Template gate — SATISFIED: the four required human contributor answers plus "is a PR needed" and the bug-fix-evidence sections are present and unchanged.
CI at the exact head (observed)
gh pr checks 152: rust, build, cuda-contract, benchmarks pass; deploy skips. Head SHA verified via the Actions API: both runs (CI 39802782538, Docs 39802782547) report head_sha = 80a30e11f87460e223cc18e479d7781c33839e60, conclusion success, completed. So the resolution compiles — including the compile-time 9==9 cross-assert — and workspace tests (the CPU structure-gate fixtures, now asserting ABI 9) pass at the exact head.
Local checks (run at a worktree of 80a30e11f)
PYTHONPATH=src python -m pytest tests/vela -q→ 121 passed in 7.75s (matches the body's claim). Worktree removed after.tests/benchmarksnot rerun locally: the delta touches no benchmark content (15 files, all qwen3_5/docs/recipe), and the CI benchmarks job is green at the exact head.
Commands and results
git rev-parse origin/pr/152;gh pr view 152 --json headRefOid,isDraft,state,title— pin match, non-draft.git rev-parse 80a30e11f^1 ^2—5745213ce/5ba6a3ba6;git rev-list 5745213ce..5ba6a3ba6 --no-merges— single commit5ba6a3ba;git rev-parse 5ba6a3ba6^—d577bb82(exact merge-base).git show 5ba6a3ba— read in full (gemm.cu, ops.h, backend.json, cuda.rs, kernels.rs test, docs).git merge-tree --write-tree 5745213ce 5ba6a3ba6vs80a30e11f^{tree}— differ (7c37f8b8vs64ad081e); conflict incuda.rs; all 3 resolution hunks read (source inspected).git grepat80a30e11f: nocheck_abi6/ABI-6 code refs, no stale ABI-8 pins in src/tests;check_abiatstructure.rs:315; cross-assert atcuda.rs:19; test fixtures assert 9.git diff origin/main..origin/pr/152 --stat— 92 files, +11,344/−505; qwen3_5 sub-diff = the branch's split only; kernels.rs empty sub-diff.gh pr checks 152+gh api .../actions/runs/{39802782538,39802782547}— green at the exact head (observed CI).- Worktree of
80a30e11f:pytest tests/vela -q→ 121 passed (commands run). No cargo on this host.
Unverified checks
- No cargo and no GPU on this host: the author-reported
cargo fmt/clippy/testre-run at80a30e11(83 targets, 0 failures) rests on author attestation plus the fork CI run pinned to80a30e11f(observed green: Format/Clippy/Test/Build within the rust job). Not locally executed. - GPU behavior of main's new
fixed_gemm_rows_do_not_depend_on_mtest, like the_h768kernels, is unverified anywhere (the test is#[ignore]-gated; it is main-side content the PR absorbs unchanged, and it can only run on a CUDA host). The P2-3 gap covers the branch's own device content. - The sm_90a bundle execution, Vela executor device tests and Laya bit-identity rerun remain unverified (P2-3, author-disclosed).
Verdict
COMMENTED. The delta is a single, well-resolved merge of main's 5ba6a3ba: the GEMM-handle change is opt-in and its only coupling to the branch is the ABI version, which the author re-pinned 8→9 with the cross-assert intact (compile-enforced, CI-green at the exact head); the own series is intact by construction; the footprint vs main is back to the authored-only totals; the kernels.rs divergence flagged last pass is resolved. The prior P3 body-staleness is resolved — the body now documents both ABI re-pins and re-pins evidence at 80a30e11, and its "capability set unchanged" claim was verified against the tree. Zero new findings. The only open item is the standing, author-disclosed P2-3 device-execution gap (sm_90a bundle, Vela executor device tests, Laya bit-identity rerun — all need a Hopper host), which continues to block APPROVE. No new findings; prior P3 closed.
|
[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 The copied Vela worker needs this fix together with #151. Personally run: 121 Vela Python tests passed, 99 affected Rust tests passed with 28 ignored, and the full Rust workspace checks passed: fmt, strict all-target Clippy, 302 tests including doctests passed / 51 ignored, and release build. Strict docs passed. The large-change split rationale and component review order are present. I inspected the structure/profile gates, plan/scratch/weight mappings and Laya migration, but did not execute the regenerated Laya ABI2 bundle or the pinned Vela native encoder. The Qwen ABI9 library used elsewhere in this review is a different backend and cannot qualify that change. Native numerical parity and Laya device-regression evidence remain an explicit handoff; CPU/plan success does not close it. Review provenance and scope
|
|
Fixed in |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review verdict: COMMENTED at 28187c804f436e54b4fda4edd879181aa39aef20 (origin/pr/152 = gh pr view 152 --json headRefOid = the commit 28187c80). Non-draft, OPEN, title unchanged. Prior review: COMMENTED at 80a30e11f (2026-10-22).
Delta isolation
git log --first-parent 80a30e11f..28187c804 contains exactly one commit: 28187c80 "vela: refuse existing bytecode caches at the package gate" (Mr.Dong, 2026-10-24; 4 files, +60/−20). Parent check: 28187c80^ == 80a30e11f byte-identical (exact SHA) — the previously reviewed series (including both reviewed merges of main) was not rebased or rewritten; every own-series commit remains patch-identical. All SHAs verified with git cat-file -e. Read in full (git show).
The new commit: the same bytecode-cache fix as #151, ported in parallel
This is #151's 3ca677b9 re-implemented on this branch's base. Same commit title, different patch — this branch's engine.py carries its own stronger gate, so the port is branch-specific:
- Refusal: any root entry named
__pycache__(file or dir), checked before the suffix chain (src/models/vela/engine.py:171-174), message "unexpected__pycache__in the package root: an existing bytecode cache is not verifiable against the pin", raised asValueError("package verification failed: …")(engine.py:186) — same refusal set, gate position and error channel as #151. - Notable context change: this branch's docstring previously justified leaving
__pycache__alone ("the interpreter's cache for the sources beside it, never reached byimportby name … left alone"). The commit correctly reverses that rationale — an unchecked-hash cache is imported without consulting the source it claims to be compiled from — and keeps the branch'scarries_codenested-code refusal (carries_codestill excludes__pycache__paths from "code", so the explicit root refusal is the single source of truth for caches;engine.py:151-155). - Import guard:
import vela2_inferencewrapped insys.dont_write_bytecode = Truewithfinally-restore (engine.py:275-284) — same pattern and rationale as #151 (a cache written by the gate's own import would fail every later gate run). - Tests:
test_an_existing_bytecode_cache_cannot_bypass_the_gate(tests/vela/test_engine_manifest.py:211-234) is slightly stronger than #151's — it first asserts the clean package passes, then poisons with anUNCHECKED_HASHcache and restores the source before expecting theValueError. The layout test no longer seeds a__pycache__. Suite green locally at this head: 122 passed (prior review measured 121 at80a30e11f; the +1 is the new test). - Docs: recipe table, refusal narrative and troubleshooting updated (
recipe/vela/reference.md,src/models/vela/README.md).
Cross-PR status (shared fix — read together with #151 and #156): all three implementations are semantically equivalent (same refusal set, same pre-import gate position, same ValueError channel, same dont_write_bytecode guard, same poisoned-unchecked-hash test shape, docs on all three surfaces) but textually divergent: different message strings, different test names/bodies, different README wording, and this branch's suffix constant is IMPORTABLE_SUFFIXES (SOURCE+BYTECODE+EXTENSION) vs #151/#156's content-equivalent IMPORTABLE_FILE_SUFFIXES. Ancestry verified: 108aec653 (#151's prior head) is an ancestor of this head; #151's new head 3ca677b97 is not — the merge-base of the two vela heads is 108aec653. See findings.
Prior-finding status (re-verified against the current body and tree)
- P2-3 (device-execution gap) — still open, author-disclosed. The body still states the sm_90a bundle is compile-verified only, execution unverified (no Hopper GPU; local GPU sm_120), "There is no GPU evidence yet", and the Laya bit-identity rerun plus the Vela executor device tests await such a host. Carried; continues to block APPROVE independent of everything below.
- rope_theta known-but-ignored provenance,
ScratchSlot::Typesaliasing fix, #151 reconciliation (every-importable-suffix gate) — spot-checked intact at this head (IMPORTABLE_SUFFIXESatengine.py:60; the new commit only extends the same gate). - Template gate — SATISFIED: the four required human contributor answers, PR-needed and bug-fix sections present.
Body / evidence check at the new head
The head SHA in the Test Plan was updated to 28187c80, but the surrounding narrative was not refreshed for the new authored commit (see P3 below). The body's last test-evidence claims are for earlier heads: "All CPU checks pass at dcc6dac, the last authored commit" and pytest re-runs at 5745213ce/80a30e11 — dated historical claims, but "the last authored commit" is now false.
CI at the exact head (observed)
gh pr checks 152: rust, build, cuda-contract, benchmarks pass (2m23s / 15s / 6s / 32s); deploy skips. Head SHA verified through the Actions API: both CI (run 40000686317) and Docs (run 40000686101) report head_sha = 28187c804f43…, 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 28187c804; no cargo/GPU on this host)
PYTHONPATH=. python -m pytest tests/benchmarks -q→ 65 passed (this branch's larger benchmark set).PYTHONPATH=src python -m pytest tests/vela -q→ 122 passed (the fix itself is green at the head, including the new test).- Worktree removed after.
Findings
- P3 — body/evidence not refreshed for the new authored commit. The Test Plan's Version line reads "
28187c80… No authored change sincedcc6dac" — false at this head (28187c80is authored); "thedcc6dacseries rebased on #151's108aec6" cites a head that #151 has moved past. The Test Result's pytest/Rust claims are pinned todcc6dac/5745213ce/80a30e11, not to28187c80. Consequence: the body's central evidence statement ("all CPU checks pass at the last authored commit") no longer refers to the head a maintainer would merge. Local re-run at this head shows the Python side green (122 passed; benchmarks 65 passed) and CI is green at the exact head, so this is a refresh, not a defect — but the refresh is required before the standing claims mean what they say. - P3 — parallel implementation of a fix on a branch that requires #151 to merge first. The body mandates "#151 … must merge first", and this branch is rebased on #151's old head (
108aec653), with the bytecode-cache fix re-implemented here rather than absorbed (#151's3ca677b97is not an ancestor; merge-base108aec653). Trigger: #151 merges as reviewed. Consequence: this branch now textually conflicts with it inengine.py, both doc surfaces and the test file, and a casual reconciliation would leave two divergent refusal messages/tests for the same gate. Needs the branch's established reconciliation-commit pattern (as done forf013512) once #151 lands. - P3 — the documented parity workflow re-poisons the package for the gate (same as #151):
recipe/vela/compare_reference.py:295bare-importsvela2_serveafter the executor's guarded import, writing__pycache__into the package root; the next worker start is refused (engine.py:171) until manual deletion. One-line tool fix or a recovery hint in the docs.
Notes (informational):
- Root-scoped refusal is sound for the shadowing threat model; this branch's
carries_codeadditionally refuses nested importable code, so this branch's gate is strictly stronger than #151/#156's. - Legitimate precompiled deployments with root bytecode caches are refused by design; disclosed in the recipe and README.
Unverified
- No cargo and no GPU on this host: the Rust-side claims for the new head rest on the CI run pinned to
28187c804(observed green: Format/Clippy/Test/Build within the rust job) — not locally executed. The P2-3 device-execution gap (sm_90a bundle execution, Vela executor device tests, Laya bit-identity rerun) remains unverified anywhere, author-disclosed.
Verdict
COMMENTED. The delta itself is a correct, well-tested port of the bytecode-cache refusal — semantically identical to #151's, with a slightly stronger test and an honest reversal of this branch's earlier "caches are safe" rationale, and CI is green at the exact head. It does not change the standing verdict: the author-disclosed P2-3 device-execution gap still blocks APPROVE, and the three P3s (body refresh for the new authored commit, the #151 reconciliation now pending, the recipe tool cache poisoning) should be addressed in the same pass.
|
Addressed in
|
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent review — PR #152 "models: make encoder structure configurable, ModernBERT core, Vela 0.3B profile" — incremental delta re-check
Review scope provenance. Local repo /Users/hsliu/thinkflowlab/system1-omni. Prior review: COMMENTED (pass-93, 2026-10-24) at head 28187c804f436e54b4fda4edd879181aa39aef20 — three P3s (body not refreshed; #151 reconciliation conflict risk; compare_reference.py:295 cache poisoning) plus the standing P2-3 device-execution gap blocking APPROVE. Live head at this re-check: 8d7be88ac271d4a983ef46fe2999dc31a71a6861 (= gh pr view 152 --json headRefOid). Non-draft, OPEN, title unchanged, base main.
Own-series integrity (gate 1)
git log --first-parent --oneline 28187c804..8d7be88ac contains exactly one commit: 8d7be88a "vela: keep bytecode writing off for the recipe's runtime imports" (1 file, +10/−2). Parent check: 8d7be88a^ == 28187c804f436e54b4fda4edd879181aa39aef20 byte-identical (exact full SHA) — the previously reviewed series (both reviewed main merges and 28187c80 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 8d7be88a — the #151 recipe fix ported, closes pass-93 P3(c)
- The fix:
recipe/vela/compare_reference.pymain()(:294-303) wraps both runtime imports —vela2_inferenceand the barevela2_servethat pass-93 flagged at:295— in asys.dont_write_bytecode = Truewindow with try/finally restore of the saved value. The package-gate refusal atengine.py:171can no longer be tripped by this tool's own documented workflow. - Completeness: single
vela2_*import site in the tool (grep-verified);sysimported at:41; the executor construction above self-guards via this branch'sengine.pywrapper (unchanged); the post-windowfrom frontend.vela import …targets a different top-level package, outside the gate's scope (source inspected). - Port equivalence with #151's
2f94be51(hunk-level diff performed): semantically identical — same window, same imports, same try/finally, byte-identical comment; the only differences are the saved variable's name and surrounding branch context. Neither port is patch-identical to the other, consistent with the branches' established parallel-implementation pattern.
Prior findings — status at the new head
- P2-3 (device-execution gap) — still open, author-disclosed. The body still states the sm_90a bundle is compile-verified only ("Execution is unverified because no Hopper GPU was available"), "There is no GPU evidence yet", and the Laya bit-identity rerun plus the Vela executor device tests await such a host. Carried; continues to block APPROVE independent of everything below.
- Pass-93 P3(a) (body/evidence not refreshed) — RESOLVED. The Test Plan's Version line now reads "
8d7be88a… plus two authored follow-ups after the reviews —28187c80… and8d7be88a…" — the false "No authored change sincedcc6dac" is gone and both follow-ups are correctly described ("5 files, all Python/docs … no Rust or CUDA change" — the 5-file/+70/−22 footprint verified bygit diff --numstat 80a30e11f..8d7be88ac). The Test Result gains a fresh paragraph pinning re-runs at the head8d7be88a: pytest tests/vela 122 passed, benchmarks 65 passed,test_compare_reference.pyok — all three counts independently reproduced locally this pass. - Pass-93 P3(b) (reconciliation conflict with #151) — now coordinated; downgraded to standing note. The body's refreshed paragraph ends: "#151 reconciliation: this branch still mandates #151 merging first; once it lands, the two parallel gate/test implementations will be reconciled with an explicit commit, as
f013512did for the earlier #151 rebase." The textual conflict is real until then (this branch is still rebased on #151's old head108aec653;3ca677b97is not an ancestor), but the resolution path is now explicit in the body, so it is a tracked obligation rather than an unmanaged risk. - Pass-93 P3(c) (
compare_reference.py:295cache poisoning) — RESOLVED in code by this commit and disclosed in the refreshed body paragraph.
New finding
P3 — the Large-change packaging totals were not refreshed and now understate the PR's diff.
Where: body, "Large-change packaging" section: "the total diff is 92 files, +11,344/−505 … Measured with git diff --numstat d577bb82..5745213ce (main-side merge-base to head; …)", and the authored headline "≈7,562 added lines across 67 files".
Trigger→consequence: the head has since advanced by merge 80a30e11 (main's ABI-9 GEMM-handle content, main-side) and the two authored follow-ups (+70/−22 in 5 already-counted files). Recomputed at the head: git diff --numstat d577bb82..8d7be88ac = 103 files, +11,611/−531 — 11 files and +270 lines more than the body states, and the parenthetical "main-side merge-base to head" is false as written (+70) has no packaging-strategy consequence (still far above the 3,000-line threshold).5745213ce is not the head). A maintainer sizing the PR from this section understates the merge footprint; the authored-count staleness (
Mitigations observed: the headline does reproduce exactly at its cited range (d577bb82..5745213ce = 92 files, +11,344/−505 — recomputed), the follow-ups are separately quantified elsewhere in the same body, and the sections a maintainer reads for evidence (Version, Test Result) are correctly refreshed — a refresh item, not a defect.
Body / evidence check at the new head
Template complete: the four required human contributor answers, the PR-necessity and bug-fix sections, and all four self-review checkboxes present. Version/Commit and Test Result re-pinned to 8d7be88a (see prior-finding 2); the "dated historical" claims (dcc6dac, 5745213ce, 80a30e11) are explicitly framed as earlier-head runs, which is acceptable. One residual: the packaging staleness above (new P3).
Commands run and results (this host: macOS arm64, miniconda Python 3.12.8, no cargo/GPU)
- Worktree at
8d7be88ac(removed after):PYTHONPATH=src python -m pytest tests/vela -q→ 122 passed (matches the body's refreshed claim). python recipe/vela/test_compare_reference.py→compare_reference tests: ok.PYTHONPATH=. python -m pytest tests/benchmarks -q→ 65 passed (matches the body).- Series forensics: parent-hash equality, first-parent log,
cat-file -e, follow-up numstat (80a30e11f..8d7be88ac= 5 files, +70/−22), hunk-level port diff vs #151, packaging recompute (above).
CI observed at the exact head
gh pr checks 152 (ThinkFlowLab): rust pass (2m02s), build pass, benchmarks pass, cuda-contract pass; deploy skipped (main-only). Actions API queried at the full SHA 8d7be88ac271… in both ThinkFlowLab and InferMatrixLab: CI and Docs both completed / success with head_sha matching the PR head exactly.
Unverified
- No cargo and no GPU on this host: the Rust-side claims for the new head rest on the CI run pinned to
8d7be88ac(observed green) — not locally executed; the body's own framing ("no Rust or CUDA change" in the follow-ups) makes the earlier-head Rust claims transfer, and spot-checks confirm the delta touches onlyrecipe/vela/compare_reference.py. - The P2-3 device-execution gap (sm_90a bundle execution, Vela executor device tests, Laya bit-identity rerun) remains unverified anywhere, author-disclosed.
Verdict
COMMENTED. The delta is a correct port of #151's recipe fix (verified semantically identical), and all three pass-93 P3s are now resolved or coordinated — the body refresh is genuine and its refreshed counts reproduce locally. Two things keep this at COMMENTED: the standing author-disclosed P2-3 device-execution gap (no Hopper-host execution evidence; blocks APPROVE regardless of anything else), and one new P3 (stale Large-change packaging totals vs the actual head diff — a body edit). With a Hopper-host evidence pass and the packaging refresh plus the post-#151 reconciliation commit, the PR remains on the approval track.
|
Validated on a Hopper host (1×H100 80GB SXM5, sm_90a), head Environment: driver 580.126.20, CUDA 13.1 (nvcc V13.1.80), rustc 1.99.1, Python 3.12.3, torch 2.11.0+cu130, tilelang 0.1.14, laya 0.3.20. (Newer than the recorded CUDA 13.0 / rustc 1.98.1 / cu128 stack; everything below is green on it regardless.) Bundle rebuild per Laya bit-identity on device (
Vela 0.3B on device (
@MrDongsls this should cover the sm_90a execution gap in the Test Result section. |
Thank you! this closes the device-execution gap. Recorded at |
|
The H100 validation at unchanged head The remaining integration step is the dependency already stated in the body: resolve #151 first, then reconcile its final loader/auth fixes with this branch’s parallel manifest gate and tests in an explicit commit. In particular, retain the sourceless/nested-namespace regressions and non-ASCII bearer behavior from #151’s Please then reduce the diff against current main, update the component/size inventory, and publish current-head Rust/Python/Docs CI with a precise executable delta from the measured Agent-assisted readiness follow-up using the repository review skill pinned at trusted main |
Required human contributor answers
Model structure becomes checkpoint-driven instead of hardcoded per crate; a second ModernBERT profile (Vela 2.0 0.3B) loads and executes through the same core.
One admitted structure per profile, capability-checked before any device work.
Laya's migration is proven byte-identical by golden plan fixtures instead of asserted.
Model integrators adding Vela 2.0 members; they need a new profile to be adapter and inventory work, not a loader copy.
After answering the four questions: is a PR needed at all to achieve this outcome?
Yes.
For a bug fix: link actual observed failure evidence.
N/A, not a bug fix.
Purpose
The base-model configurability prerequisite for #137's native slices. Builds on the unmerged reference worker PR #151, which must merge first: this branch is rebased on that PR's current head (
108aec6) and carries an explicit reconciliation commit (f013512) that keeps this branch's broader every-importable-suffix package gate while restoring the #151 head's review fixes (the resolve-basedput_first_on_sys_pathpromotion, thetoken_budgetexecutor option, the sparse-file weight stands in the manifest tests) and wiring the score example that head added into the nine-case default comparison, the oracle fixture test and the support-matrix row. Once #151 merges, this PR's diff collapses to its own commits.omni-modernbert: a pure-Rust crate with no CUDA and no weights. It holds the structure with per-field provenance and family invariants, source adapters for the Laya bundle and for any HF ModernBERT config including yarn scaling, derived geometry and the canonical encoder inventory, backend limits with the registeredlaya-0.3.20profile,StructureIdentity, and the named-kernel encoder plan.omni-vela-native: the pinned bundle is parsed, admitted and validated against the pinned checkpoint's safetensors census, which holds exactly the 134 geometry-derived encoder tensors plus 18 head-side tensors with matching names and shapes. The YaRN generator mirrors the reference and is gated by fixtures dumped from the recipe-pinned transformers. The executor behind thecudafeature uploads under the serving storage classes and returns the final hidden at requested positions in the bundle's bf16 precision._h768kernels join with their own 8192-token bound, and Laya symbols keep their existing acceptance surface.QwenHybridStructure, the frozen ABI-6 checks become typed gate checks,Config::from_structureis the seam for consumers whose Qwen fields live elsewhere, andPrefixLayoutderives the per-layer prefix-state sizes. The three Vela hybrid backbones pass the gate as CPU fixtures. The authored series changes no gate check, kernel or capability; merge5745213cre-pins the split onto main's kernels, which moved to ABI 8 with the vision-features-resident change (98d0465d) —check_abi6renamed to version-neutralcheck_abi,ABI_VERSION6 → 8, the compile-time cross-assert kept. That is a version re-pin only: the checked capability set is unchanged, so the gate admits and rejects exactly what it did at ABI 6. Merge80a30e11re-pins 8→9 the same way for main's fixed-algorithm GEMM handle (5ba6a3ba): another version re-pin, capability set unchanged; identity digests move with the recordedabi_version, so prefix-cache records are re-captured per the identity contract.Large-change packaging
This PR is above the 3,000-authored-line threshold in
CONTRIBUTING.md, so:git diff --numstat d577bb82..8d7be88acfrom main-side merge-base to head. The growth over the5745213cemeasurement is main's GEMM-handle content absorbed by merge80a30e11plus 58 authored lines from the two follow-ups. Excluded from the authored count as docs/generated/static: 536 lines of docs, 3,229 lines of pinned fixtures and generated files (Cargo.lock,tests/**/data,tests/vela/fixtures, recipe example JSONs) and 17 lines of config.ModernBertStructure. The Laya migration is the evidence that the new core reproducesmain's behavior (byte-identical golden plans plus the frozen launch-trace parity test), so landing the core without it would put an untested crate onmain, and landing it separately would re-review the same diff twice. The CUDA bundle's ABI 1→2 bump must land together with the_h768kernels and the Vela profile because the bundle is regenerated as one artifact. The qwen3_5 split is the second consumer of the same structure/gate pattern and has no behavior to review on its own.src/models/modernbert/— family core: structure with per-field provenance, HF and bundle adapters, geometry and limits, the plan builder, the gate.src/models/laya/+tests/laya/— migration onto the core; the golden plan fixtures and the launch-trace parity test pin behavior.src/models/qwen3_5/— structure parsing split from the ABI-6 capability gate; CPU-fixture driven.src/backends/cuda/— ABI 1→2, the width-templated embedding LayerNorm, the eight_h768kernels, export tooling.src/models/vela/— the 0.3B profile crate (native/), the Python worker parity hardening shared with Vela: serve Vela 2.0 0.3B through a Python reference worker #151, the frontend shim.docs/supported-models.md, READMEs, mkdocs nav, recipe docs) — read last.Test Plan
System1-Omni Version / Commit:
8d7be88aonmodel/configurable-base-model: thedcc6dacseries rebased on #151's108aec6, plus merges5745213cand80a30e11of main, plus two authored follow-ups after the reviews —28187c80(refuse existing bytecode caches at the package gate; the #151 fix re-implemented for this branch's own stronger gate) and8d7be88a(keep bytecode writing off for the recipe tool's runtime imports, the same recipe fix as #151's2f94be5).On a sm_90a host, rebuild the bundle per
recipe/laya/native/README.md, rerun the Laya device suites for bit-identity, then runVELA_NATIVE_CHECKPOINT=<pinned 0.3B checkout> VELA_NATIVE_LIBRARY=<bundle>/liblaya_cuda.so cargo test -p omni-vela-native --features cuda --test vela_executor.Test Result
All CPU checks pass at
dcc6dac:cargo fmtandclippyclean; 73 workspace test targets with 0 failures (this includes the byte-identical golden plan fixtures, the launch-trace parity test, the YaRN gates within 1e-7 of the pinned transformers on inverse frequencies and within 5.8e-5 on cos/sin inside the served window, and the exact checkpoint census equality);omni-layawithcuda,serveandomni-vela-nativewithcudapass (the executor device tests skip without theVELA_NATIVE_*pins); the contract checker reports 0 errors (its one ABI-define warning is unchanged at the basee66ff34);pytest tests/vela -q122 passed;recipe/vela/test_compare_reference.pyall checks ok.Since then the head advanced only by merges of main. At
5745213ceCI is green on that exact head (the rust, build, cuda-contract and benchmarks jobs; deploy skips), andPYTHONPATH=src pytest tests/vela -qre-ran at the merged head with 122 passed. main's5ba6a3ba(a qwen3_5 GEMM handle, CUDA ABI 8→9) is synced by merge80a30e11, a structure-side version re-pin only;cargo fmt,clippy,cargo test --workspace --locked(83 test targets, 0 failures; main-side additions grew the count from 73) and the contract checker (0 errors, the same one ABI-define warning) re-ran green at that head.Two authored follow-ups land after
80a30e11(5 files, all Python/docs:src/models/vela/engine.py,tests/vela/test_engine_manifest.py,recipe/vela/compare_reference.py, two doc surfaces — no Rust or CUDA change, so the Rust/workspace claims above still describe the tree):28187c80refuses an existing__pycache__under the package root before import and imports the runtime with bytecode writing disabled (the #151 fix on this branch's own gate), and8d7be88aextends the same no-bytecode window over the recipe tool's two runtime imports socompare_reference.pycan no longer write the cache the gate refuses. Re-run at the head8d7be88a:PYTHONPATH=src pytest tests/vela -q122 passed,PYTHONPATH=. pytest tests/benchmarks -q65 passed,recipe/vela/test_compare_reference.pyall checks ok. #151 reconciliation: this branch still mandates #151 merging first; once it lands, the two parallel gate/test implementations will be reconciled with an explicit commit, asf013512did for the earlier #151 rebase.The existing Laya suites were updated mechanically for the new seams —
packgained a vocab argument, fixture manifests bumped"abi"1→2, and structure/limits thread through the test helpers — so "passes unedited" would be the wrong claim; the behavioral evidence is the byte-identical golden plan fixtures and the frozen launch-trace comparison.The sm_90a bundle was compile-verified only author-side since the local GPU is sm_120. Executed and green on a Hopper host at
8d7be88aby jyizheng on 1×H100 80GB SXM5 with CUDA 13.1, rustc 1.99.1 and torch 2.11.0+cu130, newer than the recorded stack and green regardless. The bundle rebuilt per the recipe with the contract checker at 0 errors. Laya bit-identity withLAYA_RAW_LOGITS=1is bitwise equal for repeated graph runs and for graph against--eager, the only diff beingengine_wall_msat 26.0 vs 106.6 ms.omni-laya --features cuda,serveis green andtests/cudais 8 OK. Vela 0.3B through the shared ABI-2 bundle passesvela_executorand the fullomni-vela-nativecuda suite 13 passed 0 failed, withtests/vela122 passed.Demo / evidence
CPU fixtures reproduce with the commands above. Upstream facts come from pinned reads only: the census from the
model.safetensorsheader atd6f03aa9, and the YaRN reference fromtransformers==5.19.0. GPU evidence: the Hopper run recorded in Test Result at8d7be88a.Self-review