fix(lora): make adapter weight fusion Conv1D-layout aware - #955
Conversation
|
One acceptance criterion is intentionally left unticked on the issue: the end-to-end run that loads a GPT-2 checkpoint with an adapter through What that run should show, given the guard: a raw HuggingFace |
CI blocker, pre-existing on
|
`fuse_lora_weights_into` added a LoRA delta to a base tensor with no shape check. Every delta is in `[out_features, in_features]` layout, and that held for every family until GPT-2 landed: raw HuggingFace GPT-2 exports store `c_attn`, `c_proj` and `c_fc` in the `Conv1D` layout `[in_features, out_features]`. `Gpt2Layout` transposes them, but at model construction, and `load_model_with_adapter` fuses into the raw safetensors map first, so at fusion time the base is still transposed. Three of the four projections could not broadcast, and `mlxcel_core::add` is not declared fallible in the cxx bridge, so the MLX C++ exception was an uncatchable `std::terminate` that killed the process with no diagnostic. The square attention `c_proj` was worse: `[768, 768]` against a `[768, 768]` delta broadcasts cleanly, so the model silently ran `W + Dᵀ` instead of `W + D`, a different rank-r update of the same shape with no error, no warning and no shape anomaly. Two guards now sit in the shared fusion primitive, so both entry points are covered: the single-process path through `apply_lora_adapters`, and the pipeline-parallel `apply_stage_lora_adapter`, which never goes through `load_model_with_adapter` and would have missed a fix applied at the loading layer. `reject_conv1d_layout_fusion` takes a whole-map verdict before the first add. The layout is read from the fused QKV projection, which is `[n_embd, 3 * n_embd]` on disk and `[3 * n_embd, n_embd]` once transposed and can never be confused; that sibling is the only evidence available, because the square `c_proj` carries no layout signal of its own. Only adapter targets that resolve to a projection actually stored transposed are rejected, so an adapter touching nothing transposed still fuses. Quantized projections are skipped on the same `.scales` probe `Gpt2Layout::detect` uses. Nothing is repaired: transposing the delta would fix the non-square projections and leave the square one just as wrong, so the error names the evidence key, its shape, and every affected target instead. The second guard compares base and delta shapes before every add, and reports a mismatch as an `anyhow::Error` naming the adapter layer, the base weight key and both shapes, with an extra note when the two are exact transposes. That covers any future family that lands with a transposed on-disk layout, and it converts the process abort into a diagnosable load error. Both diagnostics are returned errors rather than `tracing::warn!`, which is a no-op in the CLI binary. Tests fuse an mlxcel-convention adapter against the raw HuggingFace GPT-2 checkpoint fixtures from `src/models/gpt2_tests.rs`, reused here rather than re-derived so the map is the one `Gpt2Layout::detect` is separately proven to classify as a Conv1D export. The square `attn.c_proj` case asserts that base and delta shapes agree before asserting the rejection, which is what makes the point that a shape check alone cannot catch it, and that the base tensor is left untouched. The three non-square projections are covered too, and reaching their assertions at all is the evidence, since on an unguarded build each aborts the test binary with SIGABRT. Positive controls confirm an already-transposed MLX conversion still fuses to the exact expected values, and `apply_stage_lora_adapter` has its own rejection test and its own `[out, in]` control. Closes #925
Comment text only, no behavior change. The repository style forbids em dashes; both occurrences become a colon, which is what the sentences meant.
7041bf2 to
5e258fd
Compare
Summary
LoRA weight fusion added a delta to a base tensor with no shape check and no notion of how the base was stored on disk. Deltas are always
[out_features, in_features]; raw HuggingFace GPT-2 exports store their projections in theConv1Dlayout[in_features, out_features], andGpt2Layoutonly transposes them at model construction, which runs after fusion on theload_model_with_adapterpath. Fusion is now layout-aware and shape-checked, and it reports rather than guesses.The two failure modes this closes
The square attention
c_proj([768, 768]against a[768, 768]delta) broadcast cleanly, so the model silently ranW + Dᵀinstead ofW + D. That is a different rank-r update of the same shape: no error, no warning, no shape anomaly, just wrong numbers. This is the case the issue was filed for, and it is the reason a shape check alone is not a fix.The three non-square projections (
c_attn,c_fc, MLPc_proj) could not broadcast, andmlxcel_core::addis not declared fallible in the cxx bridge, so the MLX C++ exception was an uncatchablestd::terminatethat killed the process instead of returning an error.What changed
src/lora/loader.rs:reject_conv1d_layout_fusiontakes a whole-map layout verdict before the firstadd. The verdict comes from the fused QKV projection, which is[n_embd, 3 * n_embd]on disk and[3 * n_embd, n_embd]once transposed and can never be confused; that sibling is the only evidence available, since the squarec_projcarries no layout signal of its own. Quantized projections are skipped on the same.scalesprobeGpt2Layout::detectuses, and the reported evidence key is the lexicographically smallest match so the diagnostic does not depend onHashMaporder.src/lora/loader.rs: a shape guard compares base and delta before everyaddand returns ananyhow::Errornaming the adapter layer, the base weight key and both shapes, with an added note when the two are exact transposes. This covers any future family that lands with a transposed on-disk layout.apply_lora_adaptersand the pipeline-parallelapply_stage_lora_adapter, which never goes throughload_model_with_adapterand would have missed a fix applied at the loading layer.tracing::warn!, which is a no-op in themlxcelCLI binary because onlysrc/server/startup.rsinstalls a subscriber.src/lora/loader_tests.rs(new): the inlinemod testsmoved out to a sibling_tests.rsper the project convention, keepingloader.rsunder the file-size limit, plus the new coverage.src/models/gpt2.rs,src/models/gpt2_tests.rs: the checkpoint-layout fixtures are nowpub(crate)so the LoRA tests assert against the same maps instead of re-deriving them.Test plan
The layout tests fuse an mlxcel-convention (
.lora_a/.lora_b) adapter against the raw HuggingFace GPT-2 fixtures fromsrc/models/gpt2_tests.rs, reused rather than reinvented: a locally invented fixture would assume the very thing under test, whereasraw_hf_weightsis the mapGpt2Layout::detectis separately proven to classify as a Conv1D export. Every layout test re-asserts that classification before touching fusion.conv1d_checkpoint_rejects_fusion_into_the_square_attention_c_projasserts base and delta shapes are equal before asserting the rejection, which is what demonstrates that a shape check alone cannot catch this case, and asserts the base tensor is left byte-unchanged.conv1d_checkpoint_rejects_every_non_square_projection_without_abortingcoversc_attn,c_fcand MLPc_proj. Reaching the assertions at all is the evidence: on an unguarded build each of these aborts the whole test binary with SIGABRT.an_already_transposed_gpt2_checkpoint_still_fuses_correctlyandconv1d_checkpoint_still_fuses_an_adapter_that_targets_nothing_transposedare positive controls asserting exact fused sums.stage_local_fusion_rejects_a_conv1d_layout_stage_weight_mapandstage_local_fusion_still_applies_to_an_out_in_stage_weight_mapcoverapply_stage_lora_adapterend to end through a real on-disk adapter directory.a_transposed_base_weight_is_reported_instead_of_reaching_mlx_addanda_mismatch_that_is_not_a_transpose_omits_the_transpose_hintcover the generic guard on a family with no Conv1D signal.DEVELOPER_DIR=... cargo test --release --lib --features metal,accelerate lora(20 passed)DEVELOPER_DIR=... cargo test --release --lib --features metal,accelerate partial_loading_adapter(14 passed, including the pre-existingstage_filtered_fusion_matches_full_fusion_on_stage_subsetparity test, so[out, in]behavior is unchanged)DEVELOPER_DIR=... cargo test --release --lib --features metal,accelerate models::gpt2(25 passed)DEVELOPER_DIR=... cargo clippy --release --lib --tests --features metal,accelerate(clean; the twohost_preprocessorwarnings are pre-existing onmain)cargo fmt --all -- --checkpython3 scripts/ci/check_cross_repo_refs.pyCloses #925