feat(convrot): select validated backend paths - #44
Conversation
|
Updated the ggml pin to leejet#94 head 0c180703, carrying the leejet#93 dynamic-backend test fix and HIP mask fix. Also limited WINDOWS_EXPORT_ALL_SYMBOLS to Windows shared-library test builds to address the inherited in-tree test-link failures. CI is running on signed head 3b94b06; no new full-video run was performed for these build/test-only changes. |
|
Update on signed head 57fa391: Windows CPU shared-library tests, Windows Vulkan, macOS, Ubuntu CPU, Ubuntu Vulkan, and static ABot now pass. Dynamic CPU/CUDA ConvRot tests also pass on NV5090. Windows CUDA plus Windows/Ubuntu ROCm jobs are still running in CI run 36120006239, so I am not calling the PR fully green yet. The temporary Windows diagnostic tracing was removed; the net follow-up changes are the test-only DLL exports and non-throwing rejection of malformed ConvRot metadata. |
67c29a6 to
9116752
Compare
9116752 to
99e547f
Compare
99e547f to
f40fff3
Compare
| if (ctx->weight_adapter) { | ||
| WeightAdapter::ForwardParams forward_params; | ||
| forward_params.op_type = WeightAdapter::ForwardParams::op_type_t::OP_LINEAR; | ||
| forward_params.linear.force_prec_f32 = force_prec_f32; | ||
| forward_params.linear.scale = scale; | ||
| if (ggml_tensor* delta = ctx->weight_adapter->lora_output_delta( | ||
| ctx->ggml_ctx, ctx->backend, x, w, prefix, forward_params)) { | ||
| out = ggml_add_inplace(ctx->ggml_ctx, out, delta); | ||
| } | ||
| } |
There was a problem hiding this comment.
Under the default LORA_APPLY_AUTO, a ConvRot model with a LoRA takes the immediate-merge path, which adds the LoRA delta to the stored ConvRot weight in the wrong basis — the runtime lora_output_delta branch added here is never reached.
AUTO switches to runtime LoRA only when ggml_is_quantized holds for some weight type (src/stable-diffusion.cpp:1018-1033). ConvRot weights are stored as GGML_TYPE_I8, which ggml marks non-quantized, so a single-device run with no offload or streaming picks the immediate merge. apply_loras_to_params (src/model_manager.cpp:570-600) does not filter by type, and build_lora_graph (src/model/adapter/lora.hpp:1134-1175) casts each matched weight to F32, adds the delta and copies it back:
- Q8_0 (the auto path on CUDA, Vulkan and Metal): the stored rows are the rotated
q·s, so the merged layer computesW x + ΔW·H xinstead ofW x + ΔW x. - I8 (native path): the cast ignores the per-row scale sidecar, so the delta is added to raw integer codes.
Impact: any LoRA on a ConvRot model under default settings silently corrupts the affected layers and the generation; the runtime lora_output_delta path is never exercised.
Suggested fix: count ConvRot storage as quantized in the AUTO decision, so these models take the runtime path lora_output_delta already implements. Inside the LORA_APPLY_AUTO branch, after the wtype_stat loop and before if (have_quantized_weight || …):
for (const auto& [name, ts] : model_loader.get_tensor_storage_map()) {
if (ts.is_comfy_int8_tensorwise) {
have_quantized_weight = true;
break;
}
}For an explicit --lora-apply-mode immediately, skip ConvRot parameters whose destination is I8 or Q8_0 in apply_loras_to_params, with the warn-once-and-mark-applied handling the row-split case uses at src/model_manager.cpp:582-592. Keep merging compat (F16) ConvRot weights — they are reconstructed in the true basis, so the merge is correct there.
On the runtime path, LoHa and raw .diff patches are still dropped; see the comment on lora_output_delta.
There was a problem hiding this comment.
Addressed in f8fa0ee. AUTO now selects runtime LoRA for ConvRot storage at src/stable-diffusion.cpp:1027. Explicit immediate mode skips I8/Q8_0 ConvRot weights with a warning at src/model_manager.cpp:595; compat F16 weights can still merge. The focused tests passed on NV5090, Strix, and Mac Studio.
| ggml_tensor* lora_output_delta(ggml_context* ctx, | ||
| ggml_backend_t backend, | ||
| ggml_tensor* x, | ||
| ggml_tensor* w, | ||
| const std::string& prefix, | ||
| WeightAdapter::ForwardParams forward_params) override { | ||
| ggml_tensor* delta = nullptr; | ||
| for (auto& lora_model : lora_models) { | ||
| ggml_tensor* current = lora_model->get_out_diff(ctx, backend, x, w, forward_params, prefix + "weight"); | ||
| if (current != nullptr) { | ||
| delta = delta == nullptr ? current : ggml_add_inplace(ctx, delta, current); | ||
| } | ||
| } | ||
| return delta; |
There was a problem hiding this comment.
lora_output_delta applies only the get_out_diff contribution (LoRA and LoKr), so the LoHa, full-weight .diff and bias-diff patches that forward_with_lora applies through patch_weight(..., false) are ignored on every ConvRot Linear.
forward_with_lora (lora.hpp:1269-1306) splits adapters in two: patch_weight(..., false) folds get_weight_diff patches — raw .diff and LoHa (hada_*), lora.hpp:760-777 — into w and b, and get_out_diff adds LoRA/LoKr in output space. lora_output_delta implements only the second. Runtime mode calls stat(at_runntime=true), which suppresses the unused-tensor warning, so nothing is logged.
Impact: LoHa adapters, full-diff patches and bias diffs are silent no-ops on every ConvRot layer while the run reports success. Once AUTO routes ConvRot models to runtime LoRA, this is the only LoRA path they have.
Suggested fix: apply the weight-diff kinds in output space, inside the existing per-model loop, before the get_out_diff call:
if (ggml_tensor* diff = lora_model->get_weight_diff(prefix + "weight", backend, ctx, w, false)) {
ggml_tensor* current = ggml_mul_mat(ctx, diff, x);
delta = delta == nullptr ? current : ggml_add_inplace(ctx, delta, current);
}with_lora_and_lokr = false excludes exactly what get_out_diff already handles, so nothing is counted twice. For the bias, patch it in the ConvRot branch of Linear::forward before ggml_add_inplace(…, out, b) (ggml_extend.hpp:4256-4258), as the conv blocks do at :4634-4635:
if (ctx->weight_adapter) {
b = ctx->weight_adapter->patch_weight(ctx->ggml_ctx, ctx->backend, b, prefix + "bias");
}There was a problem hiding this comment.
Addressed in f8fa0ee. Runtime ConvRot LoRA now includes raw weight diffs and LoHa in output space at src/model/adapter/lora.hpp:1316, and patches the bias before adding it at src/core/ggml_extend.hpp:4265. Regression coverage for raw diffs and LoHa starts at tests/test-lora-validation.cpp:124. The focused tests passed on all three servers.
| if (tensor_storage.is_comfy_int8_convrot_weight() && state->tensor->type == GGML_TYPE_Q8_0) { | ||
| if (!model_loader_.load_comfy_int8_tensorwise(tensor_storage, state->tensor, nullptr)) { | ||
| return false; | ||
| } | ||
| std::lock_guard<std::mutex> lock(loaded_names_mutex); | ||
| loaded_names.insert(name); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The loader chooses the Q8_0 repack from the destination tensor's type alone, so a compat-mode Linear that received a Q8_0 destination from --type q8_0 gets the raw rotated int8 codes while its forward() never rotates the activations.
set_wtype_override (src/model_loader.cpp:949-966) sets expected_type = Q8_0 on ConvRot *.weight tensors, because tensor_should_be_converted does not exclude them. Under SD_CONVROT_MODE=compat, Linear::init_params takes the ordinary get_type path and allocates a Q8_0 weight. This check then routes it to load_comfy_int8_tensorwise, which repacks the rotated codes, while forward() runs a plain ggml_ext_linear with no activation rotation. Other quantized overrides fail instead, since the compat reconstruction accepts only F16/F32.
Impact: compat — the fallback the loader's own error message recommends (ggml_extend.hpp:179-182) — combined with --type q8_0 produces silently wrong output.
Suggested fix: keep quantized weight-type overrides off ConvRot storage. In set_wtype_override, after the tensor_should_be_converted check and before expected_type is assigned:
if (tensor_storage.is_comfy_int8_tensorwise && ggml_is_quantized(dst_type)) {
continue;
}Compat Linears then allocate F16 and take the existing reconstruction, and F16/F32 overrides keep working. The native and Q8_0 Linears never read expected_type, so they are unaffected. A quantized --type is then ignored for these tensors rather than failing, which is worth one log line.
Keying the repack on the policy flags instead does not work here: select_convrot_tensor_storage sets them on a copy, so the loader's TensorStorage never carries them, and the standalone-op path has no convrot_h256 sibling to detect.
There was a problem hiding this comment.
Addressed in f8fa0ee. Quantized type overrides are ignored for ConvRot weights at src/model_loader.cpp:966, so compat mode reconstructs an F16 weight. The override and compat allocation checks are at tests/test-safetensors-convrot.cpp:199 and tests/test-safetensors-convrot.cpp:426. The focused tests passed on all three servers.
| if (has_convrot_weight) { | ||
| if (use_convrot_q8_decomp) { | ||
| if (use_convrot_rotation_op) { | ||
| ggml_tensor* rotated = ggml_convrot(ctx->ggml_ctx, x, 256); | ||
| ggml_set_name(rotated, (prefix + "trace.convrot.post_h256").c_str()); | ||
| out = ggml_mul_mat(ctx->ggml_ctx, w, rotated); | ||
| } else { | ||
| // H256 is symmetric: (H*w_row).x == w_row.(H*x). Rotate | ||
| // each 256-wide block before the stock Q8_0 matmul. | ||
| ggml_tensor* contiguous = ggml_is_contiguous(x) ? x : ggml_cont(ctx->ggml_ctx, x); | ||
| ggml_tensor* blocks = ggml_reshape_2d(ctx->ggml_ctx, contiguous, 256, | ||
| ggml_nelements(contiguous) / 256); | ||
| ggml_set_name(blocks, (prefix + "trace.convrot.pre_h256").c_str()); | ||
| ggml_tensor* rotated = ggml_mul_mat(ctx->ggml_ctx, params["weight.convrot_h256"], blocks); | ||
| ggml_set_name(rotated, (prefix + "trace.convrot.post_h256").c_str()); | ||
| if (use_convrot_fast_h256) { | ||
| ggml_mul_mat_set_hint(rotated, GGML_HINT_SRC0_IS_CONVROT_H256); | ||
| } | ||
| rotated = ggml_reshape_4d(ctx->ggml_ctx, rotated, x->ne[0], x->ne[1], x->ne[2], x->ne[3]); | ||
| out = ggml_mul_mat(ctx->ggml_ctx, w, rotated); | ||
| } | ||
| ggml_set_name(out, (prefix + "trace.convrot.post_q8_gemm").c_str()); |
There was a problem hiding this comment.
The Q8_0 ConvRot matmuls bypass ggml_ext_linear, so MiniMax-H3's fc1/fc2 lose their force_prec_f32=true / scale=1/128 F16-overflow guard on the new default paths.
minimax_h3.hpp:158-159 declares fc1/fc2 with force_prec_f32 = true and scale = 1/128. ggml_ext_linear (ggml_extend.hpp:1158-1190) honours both: it prescales x, sets GGML_PREC_F32 on the matmul, and undoes the scale. The Q8_0 ConvRot branch calls ggml_mul_mat(w, rotated) directly (:4230, :4244) and does neither. On Vulkan, a Q8_0 matmul at GGML_PREC_DEFAULT selects the f16acc pipeline on fp16 devices (ggml-vulkan.cpp:8926-8935), so the H3 FFN now accumulates in F16 without the guard the model code added for it.
Impact: latent inf/NaN or precision loss in the H3 FFN on the Vulkan default path. The matched Strix run was correct, so this depends on the workload rather than failing today.
Suggested fix: in the use_convrot_q8_decomp branch, scale a copy of x — keep x itself unscaled, since lora_output_delta below consumes it — and restore after the final matmul, before the bias add:
ggml_tensor* xs = scale != 1.f ? ggml_ext_scale(ctx->ggml_ctx, x, scale) : x;
// ... rotate xs instead of x on both the ROTATION_OP and dense-H256 paths ...
if (force_prec_f32) {
ggml_mul_mat_set_prec(out, GGML_PREC_F32);
}
if (scale != 1.f) {
out = ggml_ext_scale(ctx->ggml_ctx, out, 1.f / scale);
}The rotation is linear, so prescaling before it is exact. ggml_mul_mat_set_prec asserts GGML_OP_MUL_MAT, so this applies to the two Q8_0 paths only; leave the native ggml_mul_mat_convrot path as is. On Vulkan this selects f32acc, so re-run the Vulkan benchmark.
Separately, force_f32 Linears (proj_in/proj_out, *_patch_proj, video_out/audio_out) skip force_f32 when the file marks them ConvRot. Adding && !force_f32 to the ConvRot condition in init_params routes them to the existing F32 reconstruction.
There was a problem hiding this comment.
Addressed in f8fa0ee. Both Q8_0 paths now prescale activations at src/core/ggml_extend.hpp:4227, request F32 matmul accumulation at src/core/ggml_extend.hpp:4247, and restore scale at src/core/ggml_extend.hpp:4251. ConvRot Linears marked force_f32 use F32 reconstruction at src/core/ggml_extend.hpp:4153. Graph checks cover the rotation-op and dense paths; they passed on NV5090, Strix, and Mac Studio. The full Vulkan performance benchmark has not been rerun.
| if (!tensor_should_be_converted(tensor_storage, dst_type)) { | ||
| continue; | ||
| } | ||
| if (tensor_storage.is_comfy_int8_convrot_weight() && ggml_is_quantized(dst_type)) { |
There was a problem hiding this comment.
[level:major] Quantized --type overrides still apply to marked non-ConvRot int8_tensorwise weights when their width is divisible by the quantization block size. The destination becomes Q8_0, but dequantize_comfy_int8_tensorwise accepts only F16/F32, so loading fails. This guard excludes only ConvRot weights.
Suggested fix: exclude all is_comfy_int8_tensorwise weights from quantized overrides (or convert them to the requested type), and add a non-ConvRot 256-wide fixture with --type q8_0.
There was a problem hiding this comment.
Addressed in 6dee045. The override guard now covers every marked tensorwise I8 weight at src/model_loader.cpp:966. A 256-wide non-ConvRot fixture applies Q8_0, checks that the destination remains F16, and loads it successfully at tests/test-safetensors-convrot.cpp:513. The focused tests passed on NV5090, Strix, and Mac Studio.
| GGMLRunnerContext op_runner_ctx; | ||
| op_runner_ctx.ggml_ctx = op_graph_ctx; | ||
| ggml_tensor* op_output = op_linear.forward(&op_runner_ctx, op_input); | ||
| GGML_ASSERT(op_output != nullptr && op_output->op == GGML_OP_MUL_MAT); |
There was a problem hiding this comment.
[level:minor] These graph assertions confirm that the Q8 ConvRot paths are assembled, but never execute a Linear or compare its values against a reference. A scale, rotation, or bias regression in this integration can pass the test.
Recommended: run both standalone and dense Q8 Linears on a deterministic input and compare their outputs with the F32 compatibility reconstruction, including a non-unit scale and bias.
There was a problem hiding this comment.
Addressed in 6dee045 and refined in a69aade. The test executes the standalone and dense Q8 Linears with deterministic input, non-unit scale, and bias at tests/test-safetensors-convrot.cpp:436, then compares their computed values against F32 compatibility reconstruction at tests/test-safetensors-convrot.cpp:484. The CPU graph test passed on NV5090, Strix, and Mac Studio.
Summary
Select the fastest validated, quality-safe ConvRot execution path automatically for each backend:
GGML_OP_CONVROTplus stock Q8_0 matmulGGML_OP_CONVROTplus stock Q8_0 matmulThis PR also integrates the ggml Q8_0 repacking API, adds policy/graph tests, and documents the diagnostic overrides.
Dependencies
24c4fe54Performance and quality
Matched 864x480, 120-frame, 20-step boat/cliff workload:
The original PR leejet#93 fast CUDA hint produced the wrong scene because it lacked a CUDA PDL producer synchronization. Signed commit
06c91cfdfixes that race; the patched PDL-enabled full H3 run now produces the correct scene and exactly matches the PDL-disabled fast-hint video. The shipped CUDA default remains the separately validated dense H256 path pending a distinct policy decision. The separate standalone CUDA transform remains unsupported due to its unvalidated numerical behavior.Verification
test-safetensors-convrotpassed on all three platformstest-convrotpassed on Vulkan and Metal; CUDA standalone correctly reported unsupported and CPU reference coverage passed5760e41fc195ca525ca957afbd842985ba70e34875795486abc410fde4dd1cd2--diffusion-fa67c29a6(pins ggml add hipBlas support leejet/stable-diffusion.cpp#94 at24c4fe54)Diagnostic overrides remain available through
SD_CONVROT_MODE=native|dense|op|auto|compat. The now PDL-safe fast H256 hint remains opt-in through the separateSD_CONVROT_H256_MODE=fastdiagnostic override.2026-09-25 inherited CUDA validation
With the patched leejet#93 hint, PDL enabled, the RTX 5090 completed the matched 864x480 / 124-frame / 20-step H3 run in 194.49 s. All 124 decoded frames exactly matched the PDL-disabled fast-hint run (SSIM 1.000); versus the corrected dense reference, overall SSIM was 0.966451 and PSNR 35.095725 dB. The video visibly showed the requested paper boat and cliff. This pin update does not change the automatic CUDA/Vulkan/Metal policy above.
The final ggml pin also includes the signed independent dense-oracle test follow-up
23954978; the CPU and CUDA tests pass.2026-09-25 scale-fidelity review follow-up
Signed commit
67c29a6pins ggml leejet#94 review fix24c4fe54and audits F32-to-F16 scales when loading Q8_0-packed ConvRot. It reports subnormal rows and maximum relative scale error by tensor, and rejects scales that would become zero or non-finite (the native mode remains available). The loader regression passes with a usable 1e-6 subnormal fixture and a rejected 1e-9 underflow fixture. Real H3 scan: text encoder 59,650/3,584,000 rows subnormal, maximum relative error 0.227%, no underflow/overflow; diffusion 0/3,046,400 subnormal. Automatic backend selection is unchanged.