Repository navigation
cuda: declare the qwen3_5 backend - #77
Conversation
The job failed on its own branch, in five seconds:
ImportError: Start directory is not importable: 'src/backends/cuda/tests'
The checker and its tests live in ThinkFlowLab#25, so on ThinkFlowLab#77's tree there is nothing to run.
The guard I wrote checked for `check_contract.py` alone and ran *after* the test
step, so the first step failed before the guard was reached and the message
explaining the dependency never printed.
Now a first step decides whether the prerequisites are present and the three
steps run only if they are; otherwise the job reports a notice naming ThinkFlowLab#25 and
passes. That is what makes either merge order work: green on this branch before
ThinkFlowLab#25 lands, and running by itself once it does.
Verified both ways: with ThinkFlowLab#25's files absent all three steps skip, and with them
present 66 checker tests, 66 kernel tests and the contract check all pass.
|
Checked |
d1a0a12 to
b1a886a
Compare
The job failed on its own branch, in five seconds:
ImportError: Start directory is not importable: 'src/backends/cuda/tests'
The checker and its tests live in ThinkFlowLab#25, so on ThinkFlowLab#77's tree there is nothing to run.
The guard I wrote checked for `check_contract.py` alone and ran *after* the test
step, so the first step failed before the guard was reached and the message
explaining the dependency never printed.
Now a first step decides whether the prerequisites are present and the three
steps run only if they are; otherwise the job reports a notice naming ThinkFlowLab#25 and
passes. That is what makes either merge order work: green on this branch before
ThinkFlowLab#25 lands, and running by itself once it does.
Verified both ways: with ThinkFlowLab#25's files absent all three steps skip, and with them
present 66 checker tests, 66 kernel tests and the contract check all pass.
|
Fixed in
Related, in #25 ( |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Re-reviewed at head f39757975 (merge base 7f39ac409; current main 4a79980d). One new commit landed since my 2026-10-05 pass: f397579 bumps the manifest to abi_version: 5, following #64's CS1_ABI_VERSION bump on main. My earlier test-layout request is addressed: b1a886a replaces the src/backends/cuda/*/ scan with unittest discover -s tests/cuda, and the observed CI run on this head executes those 8 backend tests green.
Verified at this head: the manifest is valid JSON and schema-consistent with laya.backend.json; its 9 declared sources exactly match the directory; build.output/default_arch/architectures match what build.sh writes and passes to -gencode; min_capability: 80 matches the README and docs/supported-models.md sm_80 floor; and status: "experimental" correctly defers the tolerance/reference fields that only "validated" may claim. Nothing overclaims native support or batching — the manifest declares build targets only, and the qwen executor remains single-prompt with CPU heads. I reproduced the checker's verdicts myself: current main errors with "qwen3_5 has kernel sources but no manifest" (which this PR clears), and the simulated merge checks clean at 2 backends / 0 errors / 1 pre-existing laya warning — matching the observed CI exactly.
P3 findings:
src/backends/cuda/qwen3_5/qwen3_5.backend.json:3— on the branch's own tree,abi_version: 5disagrees withops.h:16(CS1_ABI_VERSION 4), so a direct checkout of this branch fails the contract checker even though CI passes (pull_request CI checks out the merge preview, which has main's ops.h=5). Reproduced. Please rebase onto current main4a79980dso the manifest describes the tree it is committed in..github/workflows/ci.yml:46-83— the new job never compiles the declared backend (no nvcc/build.shinvocation); thebuild.*claims are only reconciled textually. This is honestly scoped in the job comment and contract.md assigns compilation to the still-unwired Tier 2, but it leaves the compile tier as an open follow-up.- Optional: the
modelskey (consumerssrc/models/cua_s1/native/,src/models/open_jev/native/) is omitted; contract.md's example lists it and both directories exist — adding it would produce zero warnings. (Laya's manifest omits it too, so this matches in-tree precedent.)
No CUDA compilation or GPU execution was run by this review; kernel correctness/numerics remain governed by the existing evidence (#19/#52, recipes) and are unaffected by this PR. Rust suite observed green on this head (CI); not rerun locally (no Rust toolchain on this host).
Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108) and its repository map read at trusted base 4a79980d8a75. Remote head rechecked before posting.
ThinkFlowLab#19 added the kernels under src/backends/cuda/qwen3_5/ but left them undeclared, so the directory has kernel sources and no manifest. ThinkFlowLab#25 defines the manifest and its checker, and running that checker against main reports exactly that: [error] qwen3_5: has kernel sources (attention.cu, common.cuh, elementwise.cu) but no qwen3_5.backend.json manifest This adds the declaration, with the values the merged files already state: - sources: the nine kernel and header files, from the directory listing. - build.script and build.output: what build.sh actually writes, libqwen3_5_cuda.so. - build.default_arch 89 and build.architectures [89]: build.sh defaults `arch` to CUDA_COMPUTE_CAP-or-89 and passes it to a single -gencode flag. - build.min_capability 80: the README says "Tensor-core kernels need sm_80 or newer". status is `experimental`, not `validated`. ThinkFlowLab#19 validated the kernels, but this manifest is written by someone other than their author and a `validated` manifest asserts a tolerance and a reference entrypoint, which are twu3202's to state. A declaration that the backend exists and how it builds is useful on its own and does not put words in anyone's mouth; promoting it is a one-line change once the tolerance is recorded. Checked with the checker from ThinkFlowLab#25: `1 backend(s), 0 error(s), 0 warning(s)`, and with discover_buildable.py from #2, which reports it as compile-checkable.
The manifest said abi_version 1. ThinkFlowLab#19's ops.h defines CS1_ABI_VERSION 4, so the declaration disagreed with the library it describes. Nothing caught it when this was written, because the checker did not read headers. ThinkFlowLab#25 now does: it reads the `#define <PREFIX>_ABI_VERSION N` out of the declared sources and requires the manifest to match, which is what surfaced this. abi_version is 1 but ops.h defines CS1_ABI_VERSION 4 also strengthens the description in the same PR.
Nothing ran `src/backends/cuda/tests` or `check_contract.py`: the only Python discovery in this workflow is `tests/benchmarks`. Two things make this land here rather than with the checker. The step needs `check_contract.py`, which arrives in ThinkFlowLab#25. And the checker exits 1 on a tree that has kernels but no declaration for them, which is main's state until the manifest in this branch lands — so wiring it up first would redden main. Both are stated in the job's comment and in its failure message, so a wrong merge order is self-explanatory rather than a bare "file not found". No CUDA toolkit and no GPU: the checker and its tests read files as text.
The first version ran `discover -s src/backends/cuda/tests`, which finds the contract checker's tests and nothing else. Kernel tests sit beside their kernels -- `src/backends/cuda/scoring/test_scoring.py` is 17 of them -- so a backend that merges later would have had no test coverage at all while appearing to be covered. The step now loops over `src/backends/cuda/*/` and discovers in any directory holding `test_*.py`. `-t` has to point at the directory itself rather than the repository root: the kernel directories are not packages, so `-t .` fails with "Start directory is not importable". Simulated on a tree carrying both: 17 from `scoring/`, 66 from `tests/`.
The job failed on its own branch, in five seconds:
ImportError: Start directory is not importable: 'src/backends/cuda/tests'
The checker and its tests live in ThinkFlowLab#25, so on ThinkFlowLab#77's tree there is nothing to run.
The guard I wrote checked for `check_contract.py` alone and ran *after* the test
step, so the first step failed before the guard was reached and the message
explaining the dependency never printed.
Now a first step decides whether the prerequisites are present and the three
steps run only if they are; otherwise the job reports a notice naming ThinkFlowLab#25 and
passes. That is what makes either merge order work: green on this branch before
ThinkFlowLab#25 lands, and running by itself once it does.
Verified both ways: with ThinkFlowLab#25's files absent all three steps skip, and with them
present 66 checker tests, 66 kernel tests and the contract check all pass.
The loop scanned src/backends/cuda/*/ for test files, which matched the checker's own tests/ directory -- running those 66 tests a second time -- and missed tests/cuda. CONTRIBUTING.md keeps test bodies under the root tests/ tree, so use the same invocation as the benchmarks job.
`ops.h` was bumped to 5 by the Cua-S1 vision work merged after this branch
was opened, so the manifest no longer matched the library it describes.
The contract checker caught it the first time the CI step ran against
current main, which is what the check exists for:
[error] qwen3_5: abi_version is 4 but ops.h defines CS1_ABI_VERSION 5
`models` names the model executors that consume a backend, which is what makes reuse visible instead of a private arrangement between crates. `src/models/cua_s1/native/` and `src/models/open_jev/native/` both depend on `omni-qwen3-5-native`, the executor that loads `libqwen3_5_cuda.so`, and both exist on main. contract.md's example manifest is this backend and lists them.
f397579 to
d7599cd
Compare
|
Rebased onto Added On the compile tier: it is wired up, in #101 (stacked on #100). Every declared backend builds on a hosted runner in an |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Rebase verified at head d7599cd5de (now based on current main 4a79980d): the branch tree is self-consistent — in-tree ops.h:16 is CS1_ABI_VERSION 5 and the manifest says abi_version: 5 — and I re-ran the checker locally on the head tree: 2 backend(s), 0 error(s), 1 pre-existing laya warning, exit 0, plus the mirrored job steps (68 checker tests, 8 backend tests, all OK). The models consumers key landed. Prior finding status: rebase request fixed; compile-tier gap open by design (honestly scoped, Tier 2 unwired); optional models key fixed.
One new P3 introduced by the rebase:
src/backends/cuda/qwen3_5/qwen3_5.backend.json:5-15— the declaredsourcesno longer match the directory: the rebase brought insrc/backends/cuda/qwen3_5/vision.cu(the #47/#64 vision work, which is also what bumped ops.h to ABI 5 and declares thecs1_vision_*family).build.shcompiles"$here"/*.cuby glob, so vision.cu is built intolibqwen3_5_cuda.so, but the manifest lists only the 9 pre-vision sources. Nothing fails today — the checker only reads the ABI macro from declared sources — but this PR's deliverable is the manifest that describes the library and its ABI surface, and it now under-declares both. One-line fix: add"vision.cu"tosources.
Observed CI at this head: rust/build/benchmarks/cuda-contract all SUCCESS. No CUDA compilation or GPU execution was run by this review (none exists in CI either); Rust suite covered by observed CI (no toolchain on this host).
Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108) and its repository map at trusted base 4a79980d8a75; remote head rechecked before posting.
`build.sh` compiles `"$here"/*.cu`, so `vision.cu` has been built into `libqwen3_5_cuda.so` since the vision work (ThinkFlowLab#47/ThinkFlowLab#64) landed -- the same work that took `CS1_ABI_VERSION` to 5. The manifest described the pre-vision library: nine sources out of the ten in the directory, and none of the `cs1_vision_*` family that file adds to the ABI surface. The checker reads the ABI macro out of the declared sources and does not compare a directory against `sources`, so nothing failed. This PR compared them by hand, against a tree from before the rebase; the rebase is what made that check stale.
|
Fixed in Why nothing caught it: the checker reads the ABI macro out of the declared sources and never compares a directory against Verified on the head tree locally: the manifest declares 10 files for the 10 in the directory, |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Incremental re-check at new head c75379177d after the branch rebase (prior review: COMMENTED at d7599cd5de, 2026-10-08). Series identity: git range-diff of the previously reviewed head vs the new head shows every commit patch-identical and none added or dropped — a rebase, not new work. Provenance: review skill at main@4a79980d (SHA-256 58be3bc6…dde108); remote head re-pinned immediately before posting.
Prior finding status — still open (no content change): src/backends/cuda/qwen3_5/qwen3_5.backend.json still under-declares the library (declared sources omit vision.cu, which build.sh's glob compiles in; one-line fix). Also unchanged by design: the compile-tier gap (Tier 2 unwired). No CI has reported on the rebased head yet (identical series passed rust/build/benchmarks/cuda-contract at the prior head). Unverified (unchanged): no CUDA compilation or GPU execution performed by this review — none exists in CI either.
Verdict: COMMENTED (one open P3).
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Incremental re-check at new head 633c953300 — rebased after my review at c75379177d (range-diff: all commits patch-identical, none added or dropped; no content change). Prior finding unchanged and still open: qwen3_5.backend.json still under-declares the library (sources omits vision.cu, which build.sh's glob compiles in; one-line fix), and the Tier-2 compile gap remains open by design. No CI on this head yet. Verdict: COMMENTED (one open P3).
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Correction at head 633c953300 (unchanged since my 2026-10-10 re-check; found in the 2026-10-11 audit). That re-check claimed "none added or dropped; no content change" and reported the manifest P3 open — both statements were wrong. One commit was added — 633c953 (aligns the manifest's abi_version to 6 and adds the jev_vl model path) — and the P3 itself was already fixed at the very head where I first raised it: c753791 ("cuda: declare vision.cu in the qwen3_5 manifest") adds vision.cu to sources, and the manifest at this head lists it. The sweep that repeatedly reported the finding open was reading trees older than the heads it pinned (same root cause as the #29 correction; tooling since fixed). Verdict: APPROVED — zero actionable findings at this head (the broader compile-glob gap remains the acknowledged Tier-2 item, by design). Unverified: CUDA compile behavior observed via fork CI only; no Rust toolchain on this host.
Provenance: review skill at ca5f69b (SHA-256 58be3bc6…dde108); fork CI green at this exact head.
4 participants
Purpose
Declare the shared Qwen CUDA backend and run its maintained metadata checks in CI. The manifest records the actual library/build settings, all ten
.cu,.cuhand.hsource files, and the three verified consumers: Cua-S1, Open-Jev and experimental JEV-VL. Each consumer uses the sharedomni-qwen3-5-nativemodel APIs.After merging current main,
ops.hdefinesCS1_ABI_VERSION 6; the manifest now also declares ABI 6. The real checker first reproduced the stale ABI 5 mismatch on the merged tree, then passed after this correction.statusremains experimental. No numerical tolerance or reference-parity claim is added.The retained
cuda-contractCI job runs the checker's tests, roottests/cudatests andcheck_contract.py --repo-root .. It reads source/build metadata without a CUDA toolkit or GPU.Current scope and integration
Current head:
633c9533007c77dd596e2a0b0a6bf4b3f77cd00e. Actual PR target:mainat7cbce19f117f23214c8acd8d874c2ab5f10c683e; diff merge base:7cbce19f117f23214c8acd8d874c2ab5f10c683e. Main-target diff: 3 files, 70 authored changed lines / 77 total text changed lines (additions plus deletions). Authored counts include source, tests, build/validation scripts, Cargo/backend manifests and workflow code; documentation, licenses/notices, static fixtures, lockfiles and generated output are excluded.Normal merge
984cf53ea96f437e38a7926395b478ce77d2ce07preserves the published author branch and includes current main. The resulting main-target delta consists of the manifest, the metadata CI job and backend documentation; CUDA kernel/runtime changes already in main are preserved.The manifest declares exactly the merged directory's ten source/header files, including
vision.cu. JEV-VL ownership was verified through its dependency onomni-qwen3-5-nativeand directModel::load, multimodal forward and prefix-continuation calls. This declares reuse without asserting device parity for those consumers.Test plan and results
Current-head host checks pass. The contract checker reports 2 backends, 0 errors, 1 warning, exit 0. The existing warning is Laya's missing source ABI macro and remains unchanged. 68 checker tests and 8 root CUDA tests pass. Locked workspace tests report 139 passed, 0 failed, 25 ignored; formatting, strict all-target workspace Clippy, release build and strict MkDocs pass. The source inventory was also compared directly against all ten
.cu/.cuh/.hfiles and matches exactly. No checker behavior or test bodies were changed for the metadata correction.The ignored tests require GPU/checkpoint evidence and are not counted as passes. These host checks did not invoke NVCC or execute GPU work. Maintained CI still lacks Tier 2 CUDA compilation; current-head CUDA compilation and numerical regressions remain separate, unverified release checks.
Exact-head hosted verification
GitHub Actions ran in
Levius-Fubuki/system1-omnion the identical PR head633c9533007c77dd596e2a0b0a6bf4b3f77cd00e. These are fork workflow-dispatch runs, separate from upstream pull-request merge checks.CI rust covers formatting, all-target Clippy, frontend API tests, workspace tests and release build; benchmark validation/tests and the
cuda-contractmetadata job pass. Docs build is strict, with deployment skipped on the fork. PublishedFork CI / rust,Fork CI / benchmarksandFork Docs / buildstatuses all report success for this SHA. These runs do not add Tier 2 CUDA compilation or GPU parity evidence.Review feedback resolution
633c9533alignsqwen3_5.backend.jsonwith the actual ABI 6 header, preserves all ten declared sources and adds the verified JEV-VL consumer.README.mdrecords the consumer ownership and distinguishes maintained metadata/CPU CI from CUDA compilation and GPU parity.ci.yml. The metadata checker, experimental status and validation policy are retained.