feat: Vulkan compute-engine + shared-runtime foundation (upstream PR1) - #87
Conversation
ericcurtin
left a comment
There was a problem hiding this comment.
Code Review — PR #87 (foundation-compute-runtime)
Reviewed by checking out kdunn926/upstream/foundation-compute-runtime at bb8d243, building with cargo check/cargo build/cargo test/cargo clippy under the documented foundation slice (--no-default-features --features multiple-pymethods), and cross-referencing build.rs/scripts/compile_shaders.sh against shaders/.
What's good
cargo check/cargo buildsucceed cleanly for the documented foundation slice; the built extension loads andVulkanContextconstruction (device init + all 77 shader pipelines) verified working on real Vulkan hardware.- The buffer-pool/size-class design in
compute.rs, the pipeline-cache + shader-registry codegen inpipeline.rs/build.rs, and the device-feature-detection fix (device.rs:shader_int64/shader_int16now queried viaPhysicalDeviceFeatures2instead of unconditionally requested) are solid, well-commented improvements. - No
todo!()/unimplemented!(), and only one strayTODOcomment — scaffolding is otherwise complete for what it claims to cover. clippyis clean of correctness lints (only expecteddead_codefor not-yet-landed model scaffolding + minor style nits).
Blocking issues
-
cargo test --libdoes not compile under this PR's own documented default/foundation feature slice — 35 errors. Several#[cfg(test)]modules unconditionally referenceqwen35/gemma-gated items that don't exist on this branch:src/push_constants.rs:1884-1885(use crate::qwen35;,crate::tp::{q35_tp_shard,...}) — blocks all 37 tests indispatch_tests.src/kvstore.rs:390-391,558,564,585(crate::qwen35::kv_prefix_tests::{...},crate::qwen35::LayerType).src/tp.rs:5,8,1466,1595,1613,1630,1650,1669(gemma_shard_testscallinggemma_fp8_tp_shard/gemma_nvfp4_tp_shard, gated behindfeature = "gemma"inlib.rs:83).src/lib.rs(multiple modules):9264,9723-9724(use qwen35::{...}), plus calls into never-built model code (forward_batched_impl,forward_qwen35_prefill_impl,forward_qwen35_verify_impl,spec_snapshot_impl,dn_gpu_sync_to_host/_from_host,kv_export_prefix/kv_import_prefix), and 3 type mismatches frominner: placeholder(9093,9423,9780) expecting the foundation()stub but constructingGemma4Modelin test code.- This is invisible to CI:
.github/workflows/ci.yml'stestjob only runsscripts/test.sh(pytest), andcargo testis never invoked anywhere in CI. Please gate these test modules on the relevant model features (or move them out of the foundation branch entirely) and confirmcargo test --no-default-features --features multiple-pymethodspasses green before merge — ideally also add acargo teststep to CI so this class of bug can't recur.
-
Stale test assertion:
src/lib.rs:8961cutover_guard_registry_len()assertsinclude_all_shaders().len() == 131, copied from a much larger upstream tree. This branch's actual registry has ~79 entries. Once issue #1 is fixed and this test module actually compiles/runs, it will fail immediately. Please update the assertion (ideally derive expected count from the shader source list rather than hardcoding) and replace the stale narrative comment history above it (lib.rs:8866-8961, "MERGE (gemma-31b-spec into working main)... 108 registered shaders" etc.) with something accurate to this branch. -
Self-contradictory
Cargo.tomldocumentation (Cargo.toml:15-47): The first comment block (lines 15-40) states`default` re-enables EVERY model + debug-api, so a plain `cargo build`... [is] byte-for-behavior identical to before this refactor— this is false for this commit (default = ["multiple-pymethods"]at line 49 enables no model). The very next block (lines 41-47, "FOUNDATION BRANCH") correctly states the opposite. Please delete/rewrite lines 15-40 — this will actively mislead the author of the next stacked PR. -
~51 orphaned shader source files:
scripts/compile_shaders.shonly compiles 77 targets; comparing against the new/changed.compfiles in this diff, ~51 are never referenced by the compile script, never land inshader_registry.rs, and are never dispatched from Rust (their correspondingpush_constants.rshelpers likeq35_gdn_scan_pc,dsv4_swiglu_clamp_pc,laguna_moe_accum_pc,ling_kda_decay_pc,nemotron_gated_rmsnorm_pcare flagged "never used" by the compiler). Examples: alldsv4_*.comp(7),laguna_*/ling_*(9),nemotron_*(4),q35_*(6),kda_*(2), most newmul_mat_vec_mlx4*/nvfp4*/fp8*variants (~20), plusrelu2.comp,mul_mat_vec_q8_0_cols.comp,paged_attn_decode_f16_sg.comp/_f32_sg.comp. For a PR whose premise is "no models, foundation only," pre-staging that much inert shader source is scope creep that's hard to review meaningfully and easy to silently leave dead. Either drop these from this PR (let each follow-up model PR add its own shaders) or add an explicit note (e.g.shaders/PENDING.md) marking them as intentionally-inert placeholders so nobody deletes or forgets to wire them up.
Non-blocking / follow-up worth tracking
- Heavy reliance on
.unwrap()onOptionfields inside#[pymethods](e.g.self.qwen35.as_mut().unwrap()throughoutlib.rs/tp.rs) will hard-panic the whole process on a wrong-dispatch call from Python rather than raising a catchableRuntimeError, which is at odds with the stated goal ofgpu_error.rs("replace unwrap() with a Python-catchable RuntimeError"). Pre-existing pattern, not introduced by this PR, but worth a follow-up pass givengpu_error.rsnow exists to fix exactly this. direct_engine.rs:21blanket#![allow(clippy::missing_safety_doc)]over 35unsafeblocks/functions — consider narrowing this and adding// SAFETYcomments per-callsite (only 8/230unsafeblocks across the crate currently have one).
Verdict
Solid Vulkan-engineering foundation and the documented feature slice builds/runs correctly, but the test suite for that same slice doesn't compile (uncaught by CI), the Cargo.toml docs contradict themselves, and ~51 shader files are dead weight in a PR scoped to "no models." Requesting changes on items 1-4 before merge; happy to re-review quickly once addressed.
SCORE: 5/10 | VERDICT: Needs work — solid architecture but broken test suite under its own documented feature slice, contradictory Cargo.toml docs, and scope creep from 51 orphaned shader files | ISSUES: cargo test fails to compile (35 errors) under default features, CI never runs cargo test so this is invisible, stale shader-registry-length test assertion (131 vs ~79), self-contradictory Cargo.toml default-feature documentation, 51 orphaned/unreferenced shader files, unwrap()-heavy Option access in pymethods risks process panics across FFI boundary
|
@kdunn926 I got opencode + claude sonnet to do an initial review, this PR is huge, it's helpful. Could you pick up the issues flagged? |
|
There's some big ones, this being one: |
First PR of the vllm-vulkan upstreaming stack: the model-free Vulkan compute
engine + shared runtime that every model PR builds on.
- compute.rs (buffer pool, size-classes, staging), pipeline.rs (pipeline
cache + ShaderClass), device.rs / vulkan_ctx.rs, push_constants.rs,
flags.rs, gpu_error.rs, host_state.rs
- shared runtime: model.rs, moe.rs, direct_engine.rs, direct_kernels.rs,
tp.rs, spec_pipe.rs, vccl_ffi.rs, kvstore.rs, kv_prefix.rs
- build.rs shader-registry codegen + the base Vulkan shader library
- the #[pymodule] skeleton
`default` builds with NO model (identical to
`--no-default-features --features multiple-pymethods`). Per-model feature flags
are declared as stack scaffolding; each model's modules + shaders land in a
follow-up PR that enables its feature.
Addresses review feedback:
- #[cfg(test)] modules that referenced model-gated symbols are now gated on
the relevant model feature, so `cargo test --no-default-features --features
multiple-pymethods` compiles and passes GREEN (117 pass / 0 fail).
- Fixed the stale registry-length assertion (hardcoded 131 -> derived from the
actual registry) and its stale narrative comment.
- Rewrote the self-contradictory Cargo.toml feature docs (the old block
claimed `default` enables every model; this branch's default is model-free).
- Dropped 52 model-specific/quant shader sources that this branch never
compiles (base compile_shaders.sh doesn't reference them, so they were inert
and never registered/dispatched); each lands with its model PR.
NOTE: a CI job to compile the foundation-slice test binary (so this gating
regression can't recur) is prepared but omitted here because the push token
lacks `workflow` scope; apply it separately.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DLi9pfh26WWKSLzCaEoH1m
bb8d243 to
e3b705e
Compare
scripts/test.sh only runs pytest, so `cargo test` never ran in CI — which let #[cfg(test)] modules that reference model-gated symbols break `cargo test --no-default-features --features multiple-pymethods` invisibly. Add a rust-test-foundation job that runs the model-free foundation slice's unit tests. The slice is deterministic on CI (no GPU — those tests are #[ignore]; no env-sensitive tests — they are gated to model features). Deps: glslang (build.rs shader compile, via the project's ensure_vulkan), libvulkan-dev (linker needs libvulkan.so), libpython3.12-dev (pyo3 links -lpython3.12); PYTHONHOME points the embedded interpreter at the system Python. Also bump actions/checkout@v4 -> @v6 (Node 20 deprecation) across all jobs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DLi9pfh26WWKSLzCaEoH1m
d84602c to
3a01b2f
Compare
@ericcurtin - my mistake to not see the unwired CI issue before putting this up. The latest commit should address everything and I confirmed a green CI run in my fork as well. There are 10 more PRs (model/feature specific) to follow this "foundation". |
CodeRabbit on PR ericcurtin#89 (r3900946581). `matvec_cols_variant_core` picked the batched `_r{rows}_c{t}` matvec base from a match that ended in `_ => matvec_shader_core(true, false)` — "anything that is not q8_0/q4_0/q4_K is f16". `QuantFormat::Bf16` fell into that arm, so a bf16-resident batched verify (`VLLM_VULKAN_QUANT=bf16`, t > 1) was dispatched on `mul_mat_vec_f16_f32_f32_r*_c*`: the bf16 weight bytes read under the f16 exponent bias, wrong logits, no error anywhere. The finding is correct and the asymmetry is real: the SINGLE-column selector `matvec_variant_core` has always had a dedicated `QuantFormat::Bf16 => "mul_mat_vec_bf16_f32_f32"` arm, so the two selectors disagreed about the same weight. bf16 is a live residency format, not a theoretical one — `geom_pick` carries measured geometry entries for `mul_mat_vec_bf16_f32_f32` at the Nemotron/qwen35 `dn in_proj_qkv`, `in_proj_z` and `out_proj` shapes — and nothing upstream of the batched path rejects it: `batched_forward::bres_proj` refuses a weight only when `w.aux.is_some()`, and bf16 carries no aux. PRE-EXISTING, not a regression from this PR. Both arms are byte-identical at the parent commit (25e83ba) and both were introduced in the already-merged foundation PR ericcurtin#87 (5c85dd3). PR ericcurtin#89 only made the path easier to reach. Which of the reviewer's two options: ADD REAL SUPPORT, not a rejection. The `_r{rows}_c{t}` names are not separate shaders — `PipelineCache:: compile_matvec_cols` recompiles the SAME SPIR-V with (BLOCK_SIZE, NUM_ROWS, NUM_COLS) as specialization constants, and `mul_mat_vec.comp`'s NUM_COLS loop is dtype-agnostic (bf16 takes the same K_PER_ITER==2 branch f16 does). So bf16 support is one match arm plus one entry in the cols-compile list, no new shader. Rejecting a working format when support costs that little would be silent lever disengagement. Changes: - push_constants.rs: the `t > 1` match is now EXHAUSTIVE — Bf16, F16 and F32 each name their own base. No catch-all, so the next codec added to `QuantFormat` is a compile error here rather than a silent f16 mis-dispatch. Packed formats (Mlx4/Nvfp4/Fp8) have no `_c{t}` sibling at all; they are already refused by `bres_proj`, and now map to a deliberately-unregistered name so a future caller that slips past that guard gets a named `Shader '...' not found` at dispatch instead of a matvec reading 4-bit nibbles as f16. This follows the precedent `bres_proj` set. - push_constants.rs: the `t <= 1` branch now delegates to `matvec_variant_by_format` instead of re-deriving the single-column call with `f16_weight` hardcoded to `true`. That hardcode was a SECOND instance of the same defect class, found while verifying this one: it made an `F32`-formatted weight select an f16 shader. Not currently exploitable — `QuantFormat::F32` is never assigned as a weight format anywhere (grep: `from_env_str` cannot return it, and the enum variant appears in exactly one place, a filter in qwen35_forward.rs) — so this is a latent hazard removed, not a live bug fixed. Fixed here because it is the same one-line class of error and costs nothing. - pipeline.rs: compile the `mul_mat_vec_bf16_f32_f32` `_r*_c*` siblings. Best-effort like the quant bases (skipped if a trimmed shader set omits the base), in which case the dispatch now fails by name rather than silently running the wrong kernel. - pipeline.rs: `cols_block_size` decided bf16's BLOCK_SIZE by the substring test `name.contains("f16")`, which matches "bf16" by coincidence. The answer it gives is the RIGHT one — bf16 is the same 2-byte, K_PER_ITER==2, no-per-block-scale kernel shape as f16, so it wants the f16 column and not the dequant-in-loop bs=64 — but it was accidental. Made explicit, with a test pinning bf16 to the f16 block sizes across all cols so a rename cannot quietly move it, and the 16KB-LDS bound test extended to the bf16 base. Tests: `matvec_cols_variant_never_reinterprets_the_weight_dtype` asserts the general invariant (the multi-column selector must name the same weight-dtype base the single-column selector names, for every format and every T), not just "bf16 has a name". It lives in a NEW ungated `cols_dispatch_tests` module, not in `dispatch_tests` — that module is `feature = "qwen35"`-gated and therefore invisible to the gemma CI job, while this selector is shared by every model. Negative control run: with only the selector reverted and everything else intact, the test fails with batched Bf16 t=2 must stay on the mul_mat_vec_bf16_f32_f32 family left: ("mul_mat_vec_f16_f32_f32_r4_c2", 4) right: ("mul_mat_vec_bf16_f32_f32_r4_c2", 4) and with the fix restored it passes. Verified: debug, single-threaded, gemma feature set — 146 passed / 0 failed / 15 ignored (baseline 144/0/15, +2 new). scripts/lint.sh clean.
First PR of the vllm-vulkan upstreaming stack. Introduces the model-free foundation that every subsequent model PR builds on:
defaultbuilds with NO model (same slice as--no-default-features --features multiple-pymethods). Per-model feature flags are declared as stack scaffolding; each model's module(s) land in a follow-up PR that enables the corresponding feature.