serve: load canonical Fruit QSRT atoms - #269
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe change adds authenticated, schema-aware QSRT loading; Fruit W4A8 and legacy trellis execution; runtime evidence; hardened image and launcher workflows; B12X namespace integration; native-library path validation; prompt-logprob validation; and speculative-decoding phase identities. Fruit QSRT support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Launcher
participant QSRTPublication
participant vLLM
participant KQuantHybridMoEMethod
participant B12X
Launcher->>QSRTPublication: authenticate publication and source snapshots
Launcher->>vLLM: start isolated Fruit QSRT server
vLLM->>KQuantHybridMoEMethod: configure schema and runtime
KQuantHybridMoEMethod->>QSRTPublication: load authenticated atom metadata
KQuantHybridMoEMethod->>B12X: dispatch prepared W4A8 trellis parts
B12X-->>KQuantHybridMoEMethod: return accumulated part outputs
KQuantHybridMoEMethod-->>vLLM: return model output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
tests/quantization/test_kquant_hybrid.py (3)
343-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not assert on the private
Tensor._baseattribute.
_baseis a PyTorch internal. Assert the storage identity instead, which states the same fact about the view relationship and does not depend on an undocumented attribute.♻️ Proposed change
- assert view_calls[1].gate._base is backing.gate - assert view_calls[1].up._base is backing.up + assert view_calls[1].gate.data_ptr() == backing.gate.data_ptr() + assert view_calls[1].up.data_ptr() == backing.up.data_ptr()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_hybrid.py` around lines 343 - 344, Update the assertions in the view relationship test around view_calls[1].gate and view_calls[1].up to avoid the private Tensor._base attribute. Assert that each view shares storage identity with backing.gate and backing.up instead, preserving validation of the same underlying storage relationship.
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 89 discards the
trellis_w4a8stub module created at line 52.
modulesis built with aModuleTypefor"b12x.moe._shared.kernels.trellis_w4a8", and line 89 replaces that entry with the caller's object before themonkeypatch.setitemloop. TheModuleTypeis never used. Remove the name from the tuple at lines 54-62 and assign the entry directly, so the intent is explicit.♻️ Proposed cleanup
"b12x.moe._shared.kernels.w4a16", "b12x.moe._shared.kernels.w4a16.host", - "b12x.moe._shared.kernels.trellis_w4a8", ) }Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_hybrid.py` at line 61, Update the test module setup around the modules tuple and the assignment near the trellis_w4a8 entry: remove "b12x.moe._shared.kernels.trellis_w4a8" from the tuple used to create ModuleType stubs, then assign that module entry directly from the caller’s object before the monkeypatch.setitem loop.
458-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new rotation-row validations.
_ensure_runtimeadds two fail-closed checks that no test exercises: theRuntimeError("Fruit W4A8 prepared parts disagree on gate/up rotation rows")when parts report differentgate_suh/up_suhrow counts, and theRuntimeErrorwhengate_rows != up_rowsorgate_rowsis neither1norstate.num_secondary._prepared_partalready builds the rotation tensors, so both cases need only a variant helper that takes the row count.Also add a case for
shared_suh=True, sincegate_rows == 1changes bothmake_trellis_w4a8_moe_scratchand everyview_trellis_w4a8_moe_scratchcall, and the current tests only covershared_suh=False.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_hybrid.py` around lines 458 - 465, Extend the W4A8 hybrid tests around _ensure_runtime with a _prepared_part variant that accepts a rotation-row count, covering mismatched gate_suh/up_suh rows and invalid matching rows where the count is neither 1 nor state.num_secondary, both expecting RuntimeError. Add shared_suh=True coverage that verifies make_trellis_w4a8_moe_scratch and every view_trellis_w4a8_moe_scratch call use the single-row rotation layout.vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpand the docstring for the new parameters.
read_qsrt_atom_layer_metadatanow takesexpected_experts,expected_hidden_size, andexpected_intermediate_size, and it raisesValueErrorfor many distinct conditions. AddArgs:,Returns:, andRaises:sections.As per coding guidelines: "Use Google-style docstrings in Python code, with
Args:/Returns:/Raises:sections".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py` at line 94, Expand the docstring for read_qsrt_atom_layer_metadata with Google-style Args, Returns, and Raises sections, documenting expected_experts, expected_hidden_size, expected_intermediate_size, the returned value, and the ValueError conditions raised by the validation.Source: Coding guidelines
vllm/model_executor/layers/quantization/kquant_hybrid.py (3)
251-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
uses_qsrtcurrently duplicatesuses_qsrt_atoms.
create_weightssets both flags from the same expression at lines 555-556, and no other assignment differentiates them. The second flag adds a state field with no distinct meaning yet. Either dropuses_qsrtand keepuses_qsrt_atoms, or document the future non-atom QSRT storage that will separate them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 251 - 254, Remove the redundant uses_qsrt state from the class and update create_weights to rely solely on uses_qsrt_atoms, since both flags currently receive identical values and no distinct behavior uses uses_qsrt.
1004-1062: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the atom-pair geometry from the shared constants.
The literals
256(line 1004 and the error text at line 1041),8(lines 1036, 1037, 1045, 1050), and32(line 1038) all encode the same fact: one atom pair isATOMS_PER_PAIR * ATOM_CHANNELSchannels.kquant_qsrt_atomsalready exportsATOMS_PER_PAIRandATOM_CHANNELS. Import them and compute the pair width once, so a future change to the atom geometry cannot leave these five sites inconsistent.♻️ Sketch
+ pair_channels = ATOMS_PER_PAIR * ATOM_CHANNELS plan = fused_moe.plan_weights( ... - intermediate_size=256, + intermediate_size=pair_channels,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 1004 - 1062, Update the QSRT preparation flow around local atom partitioning to import and reuse ATOMS_PER_PAIR and ATOM_CHANNELS from kquant_qsrt_atoms, derive the pair channel width once, and replace the duplicated geometry literals in the constructor, validation, error text, loop stride, narrow length, and slot alignment checks. Preserve the existing complete atom-pair validation and weight-preparation behavior.
410-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
kept_storageafter the descriptor validation completes.Line 413 assigns
config.kept_storage = "x4t"between the schema check and theexpected_qsrtloop. Move the assignment below the loop so the descriptor is fully validated before any config field is mutated.♻️ Proposed reordering
schema = qsrt.get("schema") if schema not in QSRT_ATOM_SCHEMAS: raise ValueError(f"unsupported QSRT atom schema {schema!r}") - config.kept_storage = "x4t" for name, expected in expected_qsrt.items(): if qsrt.get(name) != expected: raise ValueError( f"QSRT {name} must be {expected!r}, got {qsrt.get(name)!r}" ) runtime = str(qsrt.get("runtime", "w4a16")).lower() if runtime not in {"w4a16", "w4a8"}: raise ValueError(f"unsupported QSRT runtime {runtime!r}") + config.kept_storage = "x4t" config.qsrt_runtime = runtime🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 410 - 422, Move the config.kept_storage assignment in the QSRT validation flow to after the expected_qsrt loop completes successfully, while preserving the existing schema, descriptor, and runtime validations.tests/quantization/test_kquant_qsrt_atoms.py (1)
17-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the Kimi-K3 schema and the rejection paths.
The file tests only
FRUIT_SCHEMAon the success path. The reader now branches on the schema in three places:rotation_multiplierdefaulting,pair_countvalidation, andshared_scale_rows(1 for Kimi,expertsfor Fruit). The Kimi branch, including thevectors[:, 0]squeeze, has no test here. Add one Kimi-schema case and a few rejection cases, for example an unsupportedschema, apair_countthat disagrees withatom_slots // 8, and an unalignedatom_slot_stride_bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_qsrt_atoms.py` around lines 17 - 102, Extend test_reads_and_partitions_fruit_qsrt_atoms or add focused tests covering the Kimi-K3 schema, including shared-scale row handling and the vectors[:, 0] squeeze, while preserving the existing Fruit success case. Add rejection tests for an unsupported schema, a pair_count inconsistent with atom_slots // 8, and an unaligned atom_slot_stride_bytes, asserting each is rejected by the relevant metadata/extent reader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@serve-glm52-fruit-qsrt.sh`:
- Line 7: Resolve the MODEL value to an absolute path before validating or
changing directories. Update the validation checks and vLLM invocation in the
script to reuse this resolved path, ensuring relative MODEL values are
interpreted from the caller’s working directory consistently.
- Around line 13-22: Strengthen the preflight checks in
serve-glm52-fruit-qsrt.sh before exec: verify the checked-out b12x revision
under B12X_ROOT and the expected KQuant revision, rather than only checking
directory existence. Validate MANIFEST.sha256 against the model payload and
verify QSRT_COMPLETE.json completion metadata, rejecting mismatches or modified
artifacts before startup.
- Around line 40-47: Validate TENSOR_PARALLEL_SIZE immediately after its default
assignment and before changing directories or launching vLLM, exiting with an
error unless its value is exactly "1". Keep the existing serve command unchanged
for the supported configuration.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Around line 1861-1866: Update the KQuant capture condition around
collect_kquant_exl3_mid so multi-part layers still emit a warning_once when
VLLM_KQUANT_CAPTURE_DIR is set, explicitly indicating capture is skipped because
trellis_parts has more than one part. Preserve the existing single-part capture
behavior and avoid invoking collect_kquant_exl3_mid for multi-part layers.
- Around line 208-209: Key w4a8_scratch by the layer geometry in
_HybridSharedRuntime, matching runtime.launches, so layers with different
hidden_size or intermediate_size cannot reuse incompatible scratch buffers.
Update _ensure_runtime and all w4a8_scratch lookups/initialization to include
both geometry dimensions, or record and validate the geometry before reuse while
preserving existing per-M scratch behavior.
- Around line 559-576: In create_weights, use the already-known kept state and
self.quant_config.qsrt_runtime to reject W4A8 configurations that include a kept
tier before registering or loading kept parameters. Preserve the existing
_ensure_runtime check as a fail-closed backstop, and do not alter the valid
W4A16 kept-tier path.
- Around line 1827-1828: Update the single-part trellis return paths in the
relevant forward logic to detach or clone the B12X output before returning it,
rather than relying on output.to(x.dtype). Apply this at both single-part return
sites, including the path around output_accum/part_output and the corresponding
path near the repeated-check result, while preserving the existing dtype
conversion and multi-part behavior.
---
Nitpick comments:
In `@tests/quantization/test_kquant_hybrid.py`:
- Around line 343-344: Update the assertions in the view relationship test
around view_calls[1].gate and view_calls[1].up to avoid the private Tensor._base
attribute. Assert that each view shares storage identity with backing.gate and
backing.up instead, preserving validation of the same underlying storage
relationship.
- Line 61: Update the test module setup around the modules tuple and the
assignment near the trellis_w4a8 entry: remove
"b12x.moe._shared.kernels.trellis_w4a8" from the tuple used to create ModuleType
stubs, then assign that module entry directly from the caller’s object before
the monkeypatch.setitem loop.
- Around line 458-465: Extend the W4A8 hybrid tests around _ensure_runtime with
a _prepared_part variant that accepts a rotation-row count, covering mismatched
gate_suh/up_suh rows and invalid matching rows where the count is neither 1 nor
state.num_secondary, both expecting RuntimeError. Add shared_suh=True coverage
that verifies make_trellis_w4a8_moe_scratch and every
view_trellis_w4a8_moe_scratch call use the single-row rotation layout.
In `@tests/quantization/test_kquant_qsrt_atoms.py`:
- Around line 17-102: Extend test_reads_and_partitions_fruit_qsrt_atoms or add
focused tests covering the Kimi-K3 schema, including shared-scale row handling
and the vectors[:, 0] squeeze, while preserving the existing Fruit success case.
Add rejection tests for an unsupported schema, a pair_count inconsistent with
atom_slots // 8, and an unaligned atom_slot_stride_bytes, asserting each is
rejected by the relevant metadata/extent reader.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Around line 251-254: Remove the redundant uses_qsrt state from the class and
update create_weights to rely solely on uses_qsrt_atoms, since both flags
currently receive identical values and no distinct behavior uses uses_qsrt.
- Around line 1004-1062: Update the QSRT preparation flow around local atom
partitioning to import and reuse ATOMS_PER_PAIR and ATOM_CHANNELS from
kquant_qsrt_atoms, derive the pair channel width once, and replace the
duplicated geometry literals in the constructor, validation, error text, loop
stride, narrow length, and slot alignment checks. Preserve the existing complete
atom-pair validation and weight-preparation behavior.
- Around line 410-422: Move the config.kept_storage assignment in the QSRT
validation flow to after the expected_qsrt loop completes successfully, while
preserving the existing schema, descriptor, and runtime validations.
In `@vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py`:
- Line 94: Expand the docstring for read_qsrt_atom_layer_metadata with
Google-style Args, Returns, and Raises sections, documenting expected_experts,
expected_hidden_size, expected_intermediate_size, the returned value, and the
ValueError conditions raised by the validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c979d98-c1d7-4ea2-acdb-110ce9ec7579
📒 Files selected for processing (5)
serve-glm52-fruit-qsrt.shtests/quantization/test_kquant_hybrid.pytests/quantization/test_kquant_qsrt_atoms.pyvllm/model_executor/layers/quantization/kquant_hybrid.pyvllm/model_executor/layers/quantization/kquant_qsrt_atoms.py
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
serve-glm52-fruit-qsrt.sh (2)
147-147: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDisable CUDA-graph serving in the Fruit QSRT launcher.
serve-glm52-fruit-qsrt.shcurrently passes--compilation-configwithcudagraph_modeset toFULL_AND_PIECEWISE. Keep Fruit QSRT on the non-CUDA-graph execution mode until CUDA-graph serving is explicitly qualified for this launcher.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve-glm52-fruit-qsrt.sh` at line 147, Update the compilation configuration in the Fruit QSRT launcher to disable CUDA-graph serving by replacing the current cudagraph_mode value with the non-CUDA-graph execution mode, while preserving the remaining compilation settings.
131-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unsupported
MAX_NUM_SEQSvalues and forwarded limits.
MAX_NUM_SEQSdefaults to1, but a caller can set it to2while the script still passes--max-num-seqsto vLLM. If a forwarded--max-num-seqsis also supplied, the expanded command can contain duplicate options unless vLLM rejects them. SetMAX_NUM_SEQSonly to1, and reject forwarded arguments that can change the request limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve-glm52-fruit-qsrt.sh` at line 131, Update the MAX_NUM_SEQS configuration to accept only the value 1, rejecting any caller-provided alternative instead of forwarding it to vLLM. Add argument validation before command construction to reject forwarded --max-num-seqs options, including their supported value forms, so duplicate or conflicting request limits cannot reach vLLM.vllm/model_executor/layers/quantization/kquant_hybrid.py (2)
1531-1544: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKey
trellis_output_accumby hidden size, asw4a8_scratchnow is.
runtime.trellis_output_accumis process-wide but is validated only against the calling layer's geometry. The reuse test at line 1536 requires an exacthidden_sizematch and reallocates otherwise. If two MoE layers with different hidden sizes both use multi-part trellis weights, the second layer's_ensure_runtimereallocates the buffer, and the first layer's nextcopy_at line 1938 fails on a shape mismatch. The failure is loud, not silent, and a single model normally has one hidden size. Apply the same geometry-keyed storage you introduced forw4a8_scratchso the shared runtime cannot be invalidated by a second geometry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 1531 - 1544, Update the runtime storage used by the trellis accumulation path in _ensure_runtime to key trellis_output_accum by hidden size, matching the geometry-keyed approach used for w4a8_scratch. Ensure each hidden-size entry is independently validated or allocated with the required capacity, dtype, and device, and update the later accumulation access to retrieve the entry for state.hidden_size so layers with different geometries cannot invalidate one another.
1904-1944: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winW4A8 disables the KQuant capture with no diagnostic.
The W4A8 branch returns at line 1944, before the capture handling at lines 1977-2005. A user who sets
VLLM_KQUANT_CAPTURE_DIRon a W4A8 deployment therefore gets an empty capture, for both single-part and multi-part layers, and sees no message. This is the same diagnostic gap raised previously for multi-part W4A16 layers, which you addressed withwarning_onceat lines 1979-1984. Emit the equivalentwarning_onceon the W4A8 path before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 1904 - 1944, Add the same warning_once capture-disabled diagnostic used by the later W4A16 handling to the W4A8 branch, immediately before its return after selecting output. Ensure it triggers when capture is configured and clearly explains that W4A8 bypasses KQuant capture, while preserving both single-part and multi-part output behavior.
🧹 Nitpick comments (4)
vllm/model_executor/layers/quantization/kquant_hybrid.py (2)
1025-1034: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueApply the seal cross-check to the
x4tsidecar as well.
manifest_filecompareslayer_entry["sha256"]against the sealed digest only whenfield == "qsrt_atoms". Thex4tsidecar is checked for seal membership but its manifest-declared digest is never compared. Seal membership already proves the file was hashed during publication, so this is a consistency gap rather than a hole. Generalize the check if the layer entry declares a digest per file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 1025 - 1034, The seal cross-check in manifest_file currently validates layer_entry["sha256"] only for qsrt_atoms; generalize that condition to validate any declared per-file digest, including the x4t sidecar, against publication.checksums[name]. Preserve the existing missing-checksum validation and mismatch error behavior.
1141-1162: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-pair preparation scales the launch count with the local intermediate size.
The loop creates one prepared part per 256 output channels, so a layer issues
intermediate_size / 256sequential MoE launches in_apply_once. The validated Fruit geometry produces one part per layer, which hides this cost. A deployment with a larger TP-local intermediate size, or TP1 on a wider model, produces many launches plus one FP32 accumulate per part. Record the expected part count in the existing load log at line 1247 so the launch fan-out is visible in production logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 1141 - 1162, The per-pair loop in the weight preparation path creates multiple launch parts as local intermediate size grows, but the existing load log does not expose this fan-out. Compute the resulting parts count from the same geometry used by the loop and include it in the existing load log near the weight-loading logic, using the relevant symbols such as local_atom_slots and QSRT_ATOMS_PER_PAIR so production logs show the expected number of prepared parts.tests/quantization/test_kquant_qsrt_atoms.py (1)
236-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for descriptor and marker disagreement.
The current tests exercise the file-inventory and digest branches. Two identity branches of
verify_qsrt_publicationremain untested: the descriptor comparison atkquant_qsrt_atoms.pylines 222-225, and the marker field-set check at lines 107-111. A test that mutates one descriptor field inconfig.jsonand re-seals, plus one that deletes a marker key, would pin both branches. The descriptor test also pins the presence gap I raised onkquant_qsrt_atoms.pylines 210-225.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_qsrt_atoms.py` around lines 236 - 247, Extend test_verifies_complete_qsrt_publication_and_rejects_mutation with coverage for identity mismatches: mutate a descriptor field in config.json, re-seal the publication, and assert verify_qsrt_publication rejects it; separately remove a required marker key and assert the marker field-set validation rejects it. Preserve the existing inventory and payload-mutation assertions.vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py (1)
131-174: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffFull-package hashing runs once per process, per rank.
verify_qsrt_publicationhashes every file in the package. For a multi-hundred-GiB QSRT package, each TP rank repeats this read. The seal is cached inshared_runtime, so the cost is paid once per process, but N ranks on one host multiply the disk read by N. Consider documenting the expected startup cost, or gating the payload hashing behind an explicit env flag while always verifying the manifests and the per-layer atom digest thatmanifest_filealready checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py` around lines 131 - 174, Update verify_qsrt_publication to avoid repeated full-package payload hashing across TP ranks by gating the per-file hash verification behind an explicit environment/configuration flag. Keep checksum manifest, publication marker, package identity, and per-layer atom digest validation enabled unconditionally, and preserve the existing full verification path when the flag is enabled; document the resulting startup-cost behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@serve-glm52-fruit-qsrt.sh`:
- Around line 5-6: Harden the launch initialization before the first Python
command: resolve and validate PYTHON_BIN and B12X_ROOT overrides, change to
SCRIPT_DIR, and clear PYTHONPATH before all preflight imports. Update the
sparkinfer preflight to require an approved source root or fingerprint rather
than only successful import, preventing caller-local b12x, vllm, or sparkinfer
modules from executing.
In `@tests/quantization/test_kquant_hybrid.py`:
- Around line 535-537: In test_w4a8_scratch_cache_separates_layer_geometries,
initialize runtime.trellis_scratch with the same CPU buffer used by adjacent
W4A8 tests before invoking method._ensure_runtime, so _ensure_runtime does not
allocate through torch.accelerator.current_device_index().
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Around line 471-475: Update the runtime validation in the configuration path
around `_apply_once` and the `qsrt_runtime` assignment so `w4a8` is accepted
only when the selected schema is Fruit; reject `w4a8` for all other registered
schemas during loading, while preserving `w4a16` support across schemas and the
existing runtime dispatch behavior.
- Around line 82-94: Update the fallback error construction after the namespace
import loop to preserve diagnostics from both attempted imports, rather than
chaining only failures[-1]. Include the b12x and sparkinfer ModuleNotFoundError
details in the final message while retaining the existing import filtering and
fallback behavior.
In `@vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py`:
- Around line 251-253: Update the `profile_id` field annotation in the relevant
dataclass to `int | None`, matching the `None` value produced by
`read_qsrt_atom_layer_metadata` and the existing Kimi metadata test; leave the
neighboring field annotations unchanged.
- Around line 210-225: Update the descriptor validation around
expected_descriptor to explicitly require that the manifest contains both
“codebook” and “profile_id” before comparing values. Reject missing keys at the
authentication boundary, while preserving the existing descriptor mismatch
validation for present values.
---
Outside diff comments:
In `@serve-glm52-fruit-qsrt.sh`:
- Line 147: Update the compilation configuration in the Fruit QSRT launcher to
disable CUDA-graph serving by replacing the current cudagraph_mode value with
the non-CUDA-graph execution mode, while preserving the remaining compilation
settings.
- Line 131: Update the MAX_NUM_SEQS configuration to accept only the value 1,
rejecting any caller-provided alternative instead of forwarding it to vLLM. Add
argument validation before command construction to reject forwarded
--max-num-seqs options, including their supported value forms, so duplicate or
conflicting request limits cannot reach vLLM.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Around line 1531-1544: Update the runtime storage used by the trellis
accumulation path in _ensure_runtime to key trellis_output_accum by hidden size,
matching the geometry-keyed approach used for w4a8_scratch. Ensure each
hidden-size entry is independently validated or allocated with the required
capacity, dtype, and device, and update the later accumulation access to
retrieve the entry for state.hidden_size so layers with different geometries
cannot invalidate one another.
- Around line 1904-1944: Add the same warning_once capture-disabled diagnostic
used by the later W4A16 handling to the W4A8 branch, immediately before its
return after selecting output. Ensure it triggers when capture is configured and
clearly explains that W4A8 bypasses KQuant capture, while preserving both
single-part and multi-part output behavior.
---
Nitpick comments:
In `@tests/quantization/test_kquant_qsrt_atoms.py`:
- Around line 236-247: Extend
test_verifies_complete_qsrt_publication_and_rejects_mutation with coverage for
identity mismatches: mutate a descriptor field in config.json, re-seal the
publication, and assert verify_qsrt_publication rejects it; separately remove a
required marker key and assert the marker field-set validation rejects it.
Preserve the existing inventory and payload-mutation assertions.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Around line 1025-1034: The seal cross-check in manifest_file currently
validates layer_entry["sha256"] only for qsrt_atoms; generalize that condition
to validate any declared per-file digest, including the x4t sidecar, against
publication.checksums[name]. Preserve the existing missing-checksum validation
and mismatch error behavior.
- Around line 1141-1162: The per-pair loop in the weight preparation path
creates multiple launch parts as local intermediate size grows, but the existing
load log does not expose this fan-out. Compute the resulting parts count from
the same geometry used by the loop and include it in the existing load log near
the weight-loading logic, using the relevant symbols such as local_atom_slots
and QSRT_ATOMS_PER_PAIR so production logs show the expected number of prepared
parts.
In `@vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py`:
- Around line 131-174: Update verify_qsrt_publication to avoid repeated
full-package payload hashing across TP ranks by gating the per-file hash
verification behind an explicit environment/configuration flag. Keep checksum
manifest, publication marker, package identity, and per-layer atom digest
validation enabled unconditionally, and preserve the existing full verification
path when the flag is enabled; document the resulting startup-cost behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d47e278-87cc-4d53-9dea-70b78b8c3d5d
📒 Files selected for processing (5)
serve-glm52-fruit-qsrt.shtests/quantization/test_kquant_hybrid.pytests/quantization/test_kquant_qsrt_atoms.pyvllm/model_executor/layers/quantization/kquant_hybrid.pyvllm/model_executor/layers/quantization/kquant_qsrt_atoms.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
serve-glm52-fruit-qsrt.sh (2)
91-99: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not run imported modules before root-level startup hooks are excluded.
-Pdoes not disablePythonPathprocessing orsitecustomizeimports. The fingerprint checks run afterimport b12xandimport vllm, so root-levelsitecustomize.py,b12x.py, orvllm.pycan execute before source validation. Move source-origin checks beforePYTHONPATHstarts in Python, or build a controlled import path and skip unsupported root-level modules/hooks before importingb12xandvllm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve-glm52-fruit-qsrt.sh` around lines 91 - 99, Move the source-origin fingerprint checks in the embedded Python startup code before importing b12x or vllm, and prevent root-level sitecustomize.py, b12x.py, or vllm.py from executing first. Update the PYTHONPATH/import setup around the embedded command so validation uses a controlled path or otherwise disables unsupported startup hooks before those imports occur.
187-187: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a documented CUDA-graph-free mode for this launcher.
cudagraph_mode: FULL_AND_PIECEWISEenables full/piecewise CUDA graph capture in the vLLM config, and this QA launcher does not establish that the QSRT path is qualified for it. Setcudagraph_modetoNONEwhen CUDA graphs should be disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve-glm52-fruit-qsrt.sh` at line 187, Update the --compilation-config JSON in the launcher to set cudagraph_mode to NONE instead of FULL_AND_PIECEWISE, while preserving the existing custom_ops and cudagraph_capture_sizes settings.
🧹 Nitpick comments (2)
tests/quantization/test_kquant_qsrt_atoms.py (2)
280-282: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover descriptor-only omissions separately.
manifest.pop(field)makesexpected_descriptor[field]becomeNone.verify_qsrt_publicationthen raisesQSRT package manifest omits a required descriptor fieldbefore it comparesdescriptor[field]. This test does not cover a descriptor-only omission.Keep the manifest field for this case and expect
QSRT model descriptor disagrees with the sealed manifest. Add a separate case for the manifest omission.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_qsrt_atoms.py` around lines 280 - 282, Update the test around _write_test_publication so the descriptor-only omission case removes the field only from descriptor, retains it in manifest, and expects “QSRT model descriptor disagrees with the sealed manifest.” Add a separate test case that removes the field from manifest to cover the manifest-omission error.
257-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style docstrings to the new functions.
Document
_reseal_publication_identityandtest_publication_rejects_missing_descriptor_identity. Include applicableArgs:,Returns:, andRaises:sections.As per coding guidelines, Python code must use Google-style docstrings with
Args:/Returns:/Raises:sections instead of reStructuredText/Sphinx fields.Also applies to: 275-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/quantization/test_kquant_qsrt_atoms.py` at line 257, Add Google-style docstrings to _reseal_publication_identity and test_publication_rejects_missing_descriptor_identity. Document each function’s purpose and include applicable Args:, Returns:, and Raises: sections, omitting sections that do not apply; use Google-style headings rather than reStructuredText or Sphinx fields.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@serve-glm52-fruit-qsrt.sh`:
- Line 172: Update the MAX_NUM_SEQS initialization in serve-glm52-fruit-qsrt.sh
to reject any configured value other than exactly 1 before startup, including
environment overrides. Preserve the existing default of 1 and terminate startup
with a clear error when the value is invalid.
---
Outside diff comments:
In `@serve-glm52-fruit-qsrt.sh`:
- Around line 91-99: Move the source-origin fingerprint checks in the embedded
Python startup code before importing b12x or vllm, and prevent root-level
sitecustomize.py, b12x.py, or vllm.py from executing first. Update the
PYTHONPATH/import setup around the embedded command so validation uses a
controlled path or otherwise disables unsupported startup hooks before those
imports occur.
- Line 187: Update the --compilation-config JSON in the launcher to set
cudagraph_mode to NONE instead of FULL_AND_PIECEWISE, while preserving the
existing custom_ops and cudagraph_capture_sizes settings.
---
Nitpick comments:
In `@tests/quantization/test_kquant_qsrt_atoms.py`:
- Around line 280-282: Update the test around _write_test_publication so the
descriptor-only omission case removes the field only from descriptor, retains it
in manifest, and expects “QSRT model descriptor disagrees with the sealed
manifest.” Add a separate test case that removes the field from manifest to
cover the manifest-omission error.
- Line 257: Add Google-style docstrings to _reseal_publication_identity and
test_publication_rejects_missing_descriptor_identity. Document each function’s
purpose and include applicable Args:, Returns:, and Raises: sections, omitting
sections that do not apply; use Google-style headings rather than
reStructuredText or Sphinx fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12c3667d-8562-48d9-a8b9-0cd0457bde32
📒 Files selected for processing (13)
serve-glm52-fruit-qsrt.shtests/model_executor/layers/test_sparse_attn_indexer_b12x.pytests/quantization/test_kquant_hybrid.pytests/quantization/test_kquant_qsrt_atoms.pytests/v1/attention/test_b12x_mla_fp8_rope_writer.pytests/v1/attention/test_sparse_mla_backends.pyvllm/compilation/b12x_capture.pyvllm/model_executor/layers/quantization/kquant_hybrid.pyvllm/model_executor/layers/quantization/kquant_qsrt_atoms.pyvllm/model_executor/layers/sparse_attn_indexer.pyvllm/model_executor/warmup/b12x_sparse_indexer_warmup.pyvllm/v1/attention/backends/mla/b12x_mla_sparse.pyvllm/v1/attention/backends/mla/indexer.py
🚧 Files skipped from review as they are similar to previous changes (3)
- vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py
- tests/quantization/test_kquant_hybrid.py
- vllm/model_executor/layers/quantization/kquant_hybrid.py
54891de to
9c12428
Compare
Signed-off-by: Michel Belleau <michel.belleau@malaiwah.com>
Signed-off-by: Michel Belleau <michel.belleau@malaiwah.com>
Signed-off-by: Michel Belleau <michel.belleau@malaiwah.com>
|
@coderabbitai review |
|
Final review request for immutable qualified runtime head The target Fresh checks on the current head:
This head is fingerprinted into the sealed runtime image and package. Please preserve |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vllm/v1/spec_decode/llm_base_proposer.py (1)
628-678: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPreserve the MTP phase on later draft forwards.
Line 639 adds the phase-specific descriptor only for the first forward. If
num_speculative_tokens > 1, later proposal forwards create a default descriptor and omitspeculative_draft_decode_only. Decode rows then lose the W4A8 selection signal and phase-specific CUDA graph identity after draft step 0.Pass the computed phase to every later
set_forward_contextcall. Add a multi-step MTP test that checks both forward contexts.Proposed propagation for later draft steps
with set_forward_context( per_layer_attn_metadata, self.vllm_config, num_tokens=input_batch_size, num_tokens_across_dp=batch_size_across_dp, cudagraph_runtime_mode=cudagraph_runtime_mode, + batch_descriptor=( + BatchDescriptor( + num_tokens=input_batch_size, + speculative_draft_decode_only=mtp_draft_decode_only, + ) + if cudagraph_runtime_mode != CUDAGraphMode.NONE + and mtp_draft_decode_only is not None + else None + ), slot_mapping=self._get_slot_mapping(input_batch_size), ): + if mtp_draft_decode_only is not None: + get_forward_context().additional_kwargs[ + "speculative_draft_decode_only" + ] = mtp_draft_decode_only ret_hidden_states = self._model_forward(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/v1/spec_decode/llm_base_proposer.py` around lines 628 - 678, Propagate the computed mtp_draft_decode_only phase through every later draft-forward set_forward_context call, not only the first forward block shown here. Ensure each later BatchDescriptor and forward context preserves speculative_draft_decode_only so phase-specific kernel selection and CUDA graph identity remain consistent across all MTP steps. Add a multi-step MTP test that verifies the value in both the initial and subsequent forward contexts.
🧹 Nitpick comments (4)
vllm/v1/spec_decode/llm_base_proposer.py (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style argument and return sections.
The new function accepts
common_attn_metadataand returnsbool, but its docstring only has a summary. AddArgs:andReturns:sections.As per coding guidelines, use Google-style docstrings with
Args:andReturns:sections.Proposed docstring update
def _mtp_draft_decode_only( common_attn_metadata: CommonAttentionMetadata, ) -> bool: - """Return whether every active MTP request has completed its prompt.""" + """Return whether every active MTP request has completed its prompt. + + Args: + common_attn_metadata: Attention metadata for active MTP requests. + + Returns: + True if every active request has completed its prompt. + """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/v1/spec_decode/llm_base_proposer.py` around lines 78 - 81, Update the _mtp_draft_decode_only docstring to include Google-style Args and Returns sections, documenting common_attn_metadata and the returned bool while preserving the existing summary.Source: Coding guidelines
vllm/model_executor/layers/quantization/kquant_qsrt_publication.py (1)
1043-1069: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive routed-layer validation from
_FRUIT_LAYERS.
runtime_layersaccepts every key in_FRUIT_LAYERS, but validation uses a hardcoded range and"13". If_FRUIT_LAYERSgains another layer, that layer is accepted without observation validation. Define_FRUIT_MTP_LAYER = 13, iterate over_FRUIT_LAYERSexcluding it, and usestr(_FRUIT_MTP_LAYER)for the MTP lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/model_executor/layers/quantization/kquant_qsrt_publication.py` around lines 1043 - 1069, Update the runtime-layer validation loop to iterate over `_FRUIT_LAYERS` rather than the hardcoded range, excluding a new `_FRUIT_MTP_LAYER = 13` constant; retain regular prefill/decode validation for routed layers and use `str(_FRUIT_MTP_LAYER)` for the MTP lookup and observation name.vllm/compilation/b12x_capture.py (1)
30-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
_kernel_resolution_api's import contract.This helper defines namespace precedence and deliberately preserves unrelated import failures. Add a Google-style docstring with
Returns:andRaises:sections.As per coding guidelines: “Use Google-style docstrings in Python code, with
Args:/Returns:/Raises:sections instead of reStructuredText/Sphinx fields such as:param:,:return:, and:rtype:.”Proposed docstring
def _kernel_resolution_api() -> ( tuple[ Callable[[str], None], Callable[[], bool], Callable[[], None], ] | None ): + """Resolve the complete kernel-resolution API. + + Returns: + A complete `(freeze, frozen, unfreeze)` callable tuple, or `None` + when no supported namespace exposes all three callables. + Raises: + ImportError: If an available namespace fails to import a dependency. + """ for namespace in ("b12x", "sparkinfer"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/compilation/b12x_capture.py` around lines 30 - 50, Document _kernel_resolution_api with a Google-style docstring describing the b12x-then-sparkinfer namespace precedence, the callable API tuple returned when available, and None when no compatible namespace is found. Include explicit Returns: and Raises: sections, noting that unrelated ModuleNotFoundError exceptions are propagated.Source: Coding guidelines
tests/compile/test_b12x_capture.py (1)
26-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover cleanup when the guarded body raises.
guard_b12x_kernel_resolutionunfreezes in afinallyblock, but this test covers only normal exit. Add an exception-path case and assert thatunfreezestill occurs after a capture failure.Proposed exception-path test diff
+import pytest + @@ assert events == ["freeze:test capture", "body", "unfreeze"] + + events.clear() + with pytest.raises(RuntimeError, match="capture failed"): + with b12x_capture.guard_b12x_kernel_resolution("test capture"): + events.append("body") + raise RuntimeError("capture failed") + assert events == ["freeze:test capture", "body", "unfreeze"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/compile/test_b12x_capture.py` around lines 26 - 29, Add an exception-path test alongside the existing normal-exit test for guard_b12x_kernel_resolution, make the guarded body raise a capture failure after recording its event, assert the exception propagates, and verify events include unfreeze after the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/compile/test_b12x_capture.py`:
- Around line 17-21: Update import_backend to record every requested namespace
before handling it, then assert after the fallback guard that the recorded
requests are exactly ["b12x", "sparkinfer"], preserving the existing b12x
failure and sparkinfer return behavior.
In `@tests/quantization/test_kquant_qsrt_atoms.py`:
- Line 905: Update the pytest.raises assertion’s match pattern in the affected
test to use a raw regex literal, preserving the existing “fingerprint” or
“invalid keys” matching behavior while resolving Ruff RUF043.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py`:
- Line 2264: Wrap the return expression in the fused MoE execution path around
fused_moe.run so no line exceeds the 88-character limit, while preserving the
existing slicing, dtype conversion, copy behavior, and return value.
- Around line 89-94: Update the docstrings at
vllm/model_executor/layers/quantization/kquant_hybrid.py:89-94, 173-176,
179-187, and 2234-2241 to use Google-style Args: and Returns: sections. Document
m and is_mtp_layer with the bool | None result for _decode_only_forward_phase;
the dispatch inputs and boolean result for the function at 173-176; layer_number
and its boolean result for the function at 179-187; and the layer and routing
inputs plus returned tensor for the function at 2234-2241.
---
Outside diff comments:
In `@vllm/v1/spec_decode/llm_base_proposer.py`:
- Around line 628-678: Propagate the computed mtp_draft_decode_only phase
through every later draft-forward set_forward_context call, not only the first
forward block shown here. Ensure each later BatchDescriptor and forward context
preserves speculative_draft_decode_only so phase-specific kernel selection and
CUDA graph identity remain consistent across all MTP steps. Add a multi-step MTP
test that verifies the value in both the initial and subsequent forward
contexts.
---
Nitpick comments:
In `@tests/compile/test_b12x_capture.py`:
- Around line 26-29: Add an exception-path test alongside the existing
normal-exit test for guard_b12x_kernel_resolution, make the guarded body raise a
capture failure after recording its event, assert the exception propagates, and
verify events include unfreeze after the failure.
In `@vllm/compilation/b12x_capture.py`:
- Around line 30-50: Document _kernel_resolution_api with a Google-style
docstring describing the b12x-then-sparkinfer namespace precedence, the callable
API tuple returned when available, and None when no compatible namespace is
found. Include explicit Returns: and Raises: sections, noting that unrelated
ModuleNotFoundError exceptions are propagated.
In `@vllm/model_executor/layers/quantization/kquant_qsrt_publication.py`:
- Around line 1043-1069: Update the runtime-layer validation loop to iterate
over `_FRUIT_LAYERS` rather than the hardcoded range, excluding a new
`_FRUIT_MTP_LAYER = 13` constant; retain regular prefill/decode validation for
routed layers and use `str(_FRUIT_MTP_LAYER)` for the MTP lookup and observation
name.
In `@vllm/v1/spec_decode/llm_base_proposer.py`:
- Around line 78-81: Update the _mtp_draft_decode_only docstring to include
Google-style Args and Returns sections, documenting common_attn_metadata and the
returned bool while preserving the existing summary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83a863fb-5a0d-46ab-8b79-60098ec4a3aa
📒 Files selected for processing (28)
serve-glm52-fruit-qsrt.shtests/compile/test_b12x_capture.pytests/distributed/test_nccl_path_security.pytests/entrypoints/openai/chat_completion/test_chat_echo.pytests/quantization/test_kquant_hybrid.pytests/quantization/test_kquant_qsrt_atoms.pytests/quantization/test_kquant_qsrt_launcher.pytests/quantization/test_kquant_runtime_evidence.pytests/test_sampling_params.pytests/tools/test_prepare_fruit_qsrt_image.pytests/v1/sample/test_logprobs.pytests/v1/spec_decode/test_eagle.pytests/v1/spec_decode/test_llm_base_proposer_sampling.pytools/prepare_fruit_qsrt_image.pyvllm/compilation/b12x_capture.pyvllm/compilation/kquant_runtime_evidence.pyvllm/distributed/device_communicators/pynccl_wrapper.pyvllm/entrypoints/openai/chat_completion/protocol.pyvllm/entrypoints/openai/completion/protocol.pyvllm/forward_context.pyvllm/model_executor/layers/quantization/kquant_hybrid.pyvllm/model_executor/layers/quantization/kquant_qsrt_atoms.pyvllm/model_executor/layers/quantization/kquant_qsrt_publication.pyvllm/model_executor/layers/quantization/kquant_x4t.pyvllm/sampling_params.pyvllm/utils/nccl.pyvllm/utils/path_validation.pyvllm/v1/spec_decode/llm_base_proposer.py
🚧 Files skipped from review as they are similar to previous changes (16)
- vllm/distributed/device_communicators/pynccl_wrapper.py
- vllm/utils/nccl.py
- vllm/entrypoints/openai/completion/protocol.py
- tests/v1/sample/test_logprobs.py
- tests/entrypoints/openai/chat_completion/test_chat_echo.py
- vllm/sampling_params.py
- vllm/utils/path_validation.py
- tests/quantization/test_kquant_hybrid.py
- tests/tools/test_prepare_fruit_qsrt_image.py
- vllm/model_executor/layers/quantization/kquant_qsrt_atoms.py
- vllm/compilation/kquant_runtime_evidence.py
- tests/quantization/test_kquant_qsrt_launcher.py
- serve-glm52-fruit-qsrt.sh
- tests/test_sampling_params.py
- vllm/entrypoints/openai/chat_completion/protocol.py
- tests/quantization/test_kquant_runtime_evidence.py
| def import_backend(namespace: str): | ||
| if namespace == "b12x": | ||
| raise ModuleNotFoundError(name=namespace) | ||
| assert namespace == "sparkinfer" | ||
| return legacy |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that fallback attempts b12x first.
import_backend does not record requested namespaces. An implementation that imports only sparkinfer still passes this test. Record each request and assert ["b12x", "sparkinfer"] after the guard.
Proposed test-strengthening diff
def test_kernel_resolution_guard_falls_back_to_sparkinfer(monkeypatch) -> None:
events: list[str] = []
+ imported_namespaces: list[str] = []
legacy = SimpleNamespace(
@@
def import_backend(namespace: str):
+ imported_namespaces.append(namespace)
if namespace == "b12x":
raise ModuleNotFoundError(name=namespace)
@@
assert events == ["freeze:test capture", "body", "unfreeze"]
+ assert imported_namespaces == ["b12x", "sparkinfer"]Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/compile/test_b12x_capture.py` around lines 17 - 21, Update
import_backend to record every requested namespace before handling it, then
assert after the fallback guard that the recorded requests are exactly ["b12x",
"sparkinfer"], preserving the existing b12x failure and sparkinfer return
behavior.
| manifest_path.write_text(json.dumps(manifest), encoding="utf-8") | ||
| _reseal_publication_identity(tmp_path) | ||
|
|
||
| with pytest.raises(ValueError, match="fingerprint|invalid keys"): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a raw regex literal.
Line 905 triggers Ruff RUF043 because match= contains regex metacharacters. Prefix the pattern with r to retain the same matching behavior and satisfy lint.
Proposed fix
- with pytest.raises(ValueError, match="fingerprint|invalid keys"):
+ with pytest.raises(ValueError, match=r"fingerprint|invalid keys"):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(ValueError, match="fingerprint|invalid keys"): | |
| with pytest.raises(ValueError, match=r"fingerprint|invalid keys"): |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 905-905: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/quantization/test_kquant_qsrt_atoms.py` at line 905, Update the
pytest.raises assertion’s match pattern in the affected test to use a raw regex
literal, preserving the existing “fingerprint” or “invalid keys” matching
behavior while resolving Ruff RUF043.
Source: Linters/SAST tools
| def _decode_only_forward_phase( | ||
| m: int, | ||
| *, | ||
| is_mtp_layer: bool = False, | ||
| ) -> bool | None: | ||
| """Resolve an authoritative decode phase, or ``None`` when unavailable.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add Google-style sections to the new function docstrings.
The new parameterized function docstrings omit Args: and Returns: sections.
vllm/model_executor/layers/quantization/kquant_hybrid.py#L89-L94: Documentm,is_mtp_layer, and thebool | Noneresult.vllm/model_executor/layers/quantization/kquant_hybrid.py#L173-L176: Document the dispatch inputs and boolean result.vllm/model_executor/layers/quantization/kquant_hybrid.py#L179-L187: Documentlayer_numberand the boolean result.vllm/model_executor/layers/quantization/kquant_hybrid.py#L2234-L2241: Document the layer and routing inputs and the returned tensor.
As per coding guidelines, Python docstrings must use Google-style Args:/Returns:/Raises: sections.
📍 Affects 1 file
vllm/model_executor/layers/quantization/kquant_hybrid.py#L89-L94(this comment)vllm/model_executor/layers/quantization/kquant_hybrid.py#L173-L176vllm/model_executor/layers/quantization/kquant_hybrid.py#L179-L187vllm/model_executor/layers/quantization/kquant_hybrid.py#L2234-L2241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` around lines 89 -
94, Update the docstrings at
vllm/model_executor/layers/quantization/kquant_hybrid.py:89-94, 173-176,
179-187, and 2234-2241 to use Google-style Args: and Returns: sections. Document
m and is_mtp_layer with the bool | None result for _decode_only_forward_phase;
the dispatch inputs and boolean result for the function at 173-176; layer_number
and its boolean result for the function at 179-187; and the layer and routing
inputs plus returned tensor for the function at 2234-2241.
Source: Coding guidelines
| topk_ids=tids, | ||
| route_expert_map=state.emap_kept, | ||
| ) | ||
| return fused_moe.run(binding=binding)[: x.shape[0]].to(dtype=x.dtype, copy=True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap this return statement.
Line 2264 is 90 characters including indentation. It exceeds the required 88-character limit. Split the expression across lines.
As per coding guidelines, Python code must follow an 88-character line length limit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vllm/model_executor/layers/quantization/kquant_hybrid.py` at line 2264, Wrap
the return expression in the fused MoE execution path around fused_moe.run so no
line exceeds the 88-character limit, while preserving the existing slicing,
dtype conversion, copy behavior, and return value.
Source: Coding guidelines
Test Results (automated)Host: macOS M4 Max, CPU-only (no CUDA) Tests require dependencies not available on this host (macOS M4 Max, CPU-only torch, no CUDA). Cannot run. The collection step failed with an Because Automated test run by @malaiwah's agent. Results are from a CPU-only environment; GPU-dependent tests may behave differently on CUDA hardware. |
…neath (C8) Address adversarial review findings on PR vllm-project#269: C7: active_runtime_environment_from_env() authenticated nothing. Both the JSON payload (FRUIT_QSRT_RUNTIME_ENVIRONMENT_JSON) and its SHA-256 digest (FRUIT_QSRT_RUNTIME_ENVIRONMENT_SHA256) came from os.environ, so the hash comparison was self-satisfying -- anyone who could set one could set the other. The only real check compared an env-supplied copy of the constant against the constant itself. Rewrite it to inspect the REAL os.environ: anchor the private root on FRUIT_QSRT_AUTHENTICATED_MODEL_ROOT (<PRIVATE_ROOT>/model) and the root id on LOCAL_INFERENCE_CACHE_FINGERPRINT, substitute the placeholders, and require every contract variable to be present in the live environment with the canonically-substituted value. The canonical placeholder map is still returned for downstream comparison against the sealed qualification receipt. Docstring now accurately describes a coherence check, not authentication. C8: _open_beneath() did not itself reject an absolute `relative` (whose parts[0] == os.sep makes os.open(..., dir_fd=...) ignore dir_fd and escape the root) nor `..` components. Move both rejections inside the helper so it is safe to call standalone, not only when callers pre-validate. Co-authored-by: GLM-5.2 <noreply@z.ai>
Adversarial review fixes applied (B6, C7, C8)B6 — base branch reversed back to
|
1 participant
Summary
qsrt_atoms_v1packages without reconstructing dense expert weights.Current canonical head:
0429cb4c11ad7a67ec8b40b621eae061a363b6cb, based ondev/k3-qsrt-sqgatad1d3d1cf7123864bdd5e2bf1ed52c3437035828.Runtime trust boundary
The launcher:
max_num_seqs=1,max_model_len=4096, andmax_num_batched_tokens=4096;Final runtime pins are QSRT
2113af303f37cedf4b538dcf68eb699d5e31f7df, B12Xf4064d06f029240040a128388f32c861a535ad68, and vLLM0429cb4c11ad7a67ec8b40b621eae061a363b6cb.Verification
The immutable image
sha256:e1b411c3c1ef02e0e8be966de31d727f04683f93d0c61711cb6ae703c9ee8a19loaded every routed and MTP layer in the 2,816-expert package under vLLMFULL_AND_PIECEWISECUDA graphs. Runtime-path evidence records W4A16 prompt/prefill and W4A8 decode graph capture and replay, including the packaged MTP layer. Final completion-marker authentication and an OpenAI-compatible completion smoke both passed.The sealed matched protocol used that one image on one RTX 5090, launch order QSRT → BF16 → SIQ, TP1,
max_num_seqs=1, identical prompt tokens/settings, and three warmed repetitions per arm:These rates include serving/request overhead and are not decode-only or general-throughput claims. QSRT used 68.14% less loader weight memory than BF16 and 4.32% less than SIQ; its median rate was 4.35% below BF16 and 0.69% below SIQ under this exact protocol.
Across 5,870 full-vocabulary BF16-reference positions:
All three arms passed 0/8 focused absolute behavior contracts. The result qualifies storage, loading, bounded runtime execution, and relative fidelity—not assistant quality.
Publication
malaiwah/GLM-5.2-QSRT-Fruit-Instruct@cba27c73.41f32656b7cc68e4c1bc6cec8c91964672ff2ff0ff989448a6d5f37e3b7fb9c3.c56f33b5da5e813d83152c5775288c5364c74205a3d70aacf528eafdc327ea78(B12X distribution1.2.3).Scope
Only TP1 physical serving is qualified. TP2 atom ownership is unit-tested but not physically served here. The launcher enforces
max_num_seqs=1; this PR does not claim broader concurrency, TP>1, long-context qualification, standardized benchmark quality, or general assistant capability.Depends on local-inference-lab/qsrt#4 and local-inference-lab/b12x#129.
Summary by CodeRabbit
New Features
Bug Fixes
prompt_logprobsnow rejects negative and vocabulary-sized values with clearer validation.Compatibility