Skip to content

SLERP merge silently returns only the second adapter: state_a aliases base_model storage (CR-A13-001) #407

Closed

Description

@cemililik

Wave 1 · theme D1-5: Training, merging and export correctness · highest severity High · 1 finding(s) · effort small

Summary

Operator merges a code adapter and a support adapter with slerp 0.5/0.5, ships the 'merged' model; it is just the second adapter, the first specialist's behaviour is silently lost, and README/docs advertise SLERP as a working method.

Primary finding

CR-A13-001 — SLERP merge silently returns only the second adapter: state_a aliases base_model storage

severity High · reachability default · intent contract-violation · confidence confirmed · independently confirmed · reproduced end to end

What happens. _slerp_merge stores state_a = model_a.merge_and_unload().state_dict() without cloning (forgelm/merging.py:198-199). The state_dict tensors share storage with base_model's live parameters, so base_model.load_state_dict(base_state) (merging.py:203) copies the base back into state_a in place, and merging adapter B (merging.py:205-206) writes base+dB into the same storage. state_a and state_b are therefore identical, and slerp(v, v) takes the near-parallel branch and returns base+B whatever the weights. MergeResult still reports success with method 'slerp'. The SLERP tests mock PEFT with independent dicts, so they cannot see the aliasing.

Where.

Evidence.

  • Two real LoRA adapters (sft, dpo) merged with slerp weights 0.7/0.3: for all q_proj/v_proj tensors max|out-base-dpo_delta| < 1e-8 while |out-base-sft_delta| ~1e-4 (sl2.py). Entry order swapped (dpo first, sft second): output equals sft delta, not dpo (sl3.py). Linear and TIES recomputed independently and match (err 7.5e-9 and 0). Existing slerp tests mock the PEFT load (tests/test_merging_algos.py:592-640) and cannot see aliasing.
  • slerp_mech.py (tiny-qwen + env/artifacts/sft as A, env/artifacts/dpo as B; torch 2.12.1, transformers 5.13.0, peft 0.19.1): state_a[k].data_ptr()==live param ptr -> True; after load_state_dict(base_state) state_a[k]==base -> True; after B merge torch.equal(state_a[k], state_b[k]) -> True, same storage True. slerp_repro.py: merge_peft_adapters(method='slerp') success=True; for weights (1,1) and (0.9,0.1) the output's max |diff| on the LoRA-touched q_proj is 7.45e-09 to B and 1.018e-04 to A, whil…
  • m3.py with tiny-qwen and the real distinct fixture adapters artifacts/sft (A) and artifacts/dpo (B), q_proj weight of layer 0: A and B differ by 1.0e-4 max. For weights 1:1, 3:1 and 1:3 the SLERP result differs from the B-merged model by at most 7.5e-9 (float noise) and from the A-merged model by 1.0e-4; the expected lerp-equivalent differs from the result by 5.1e-5, 7.6e-5, 2.5e-5. The result is identical for all three weightings.

Recommended fix. Clone state_a (and state_b) immediately after merge_and_unload, e.g. {k: v.clone() for k, v in ...state_dict().items()}; add a non-mocked tiny-adapter test asserting the result differs from each input and respects weights.

Acceptance criteria

  • A regression test reproduces the failure scenario and asserts the corrected behaviour.
  • Documentation listed above is corrected in English and in its Turkish mirror in the same PR.
  • Every finding in this issue is resolved or explicitly closed with a reason.

Found by the October 2026 ForgeLM code and documentation review against commit cab2947. Code links point at that commit; line numbers may have moved since. Finding IDs (CR-…, DOC-…, TECH-…) are stable references for this backlog.

Activity

  1. added
    bugSomething isn't working
    severity: highWrong behaviour or a false claim users rely on
    source: review-2026-10Found by the October 2026 code and documentation review
    wave: 1Roadmap wave 1: high severity, small effort
    on Oct 9, 2026
  2. cemililik commented on Oct 15, 2026

    @cemililik
    CollaboratorAuthor

    Where to start

    Thanks for taking a look. Please confirm the issue is unassigned and comment before starting so work does not overlap.

    Start with these files on development:

    In _slerp_merge, state_a = model_a.merge_and_unload().state_dict() is not cloned, so its tensors share storage with base_model. Loading the base state back and then merging adapter B writes into the same storage, which leaves state_a and state_b identical; SLERP of two identical tensors returns the second adapter whatever the weights, and the merge reports success. The other merge algorithms in the file already clone the state tensors (v.clone()), which is the pattern to follow.

    Clone state_a and state_b right after merge_and_unload, and add a regression test with a tiny, non-mocked adapter pair that asserts the merged result differs from each input and responds to the weights. The red-before / green-after test is the main deliverable here.

    Run the focused tests:

    pytest tests/test_merging_algos.py --no-cov

    Run these guards/checks as applicable to the changed files:

    ruff format --check forgelm/merging.py tests/test_merging_algos.py
    ruff check forgelm/merging.py tests/test_merging_algos.py

    Tests must not need a GPU, network access or model downloads. Add a ### Fixed entry under ## [Unreleased] in CHANGELOG.md naming the finding ID.

    If user-facing documentation changes, update the existing Turkish mirror in the same PR; keep commands, code and terminal output in English and structurally aligned. English-only contributor standards need no new mirror.

    Open the PR against development with Fixes #407 once every finding is resolved or explicitly closed with a reason; use Refs #407 for partial work and list the resolved finding IDs (Resolves: CR-A13-001).

  3. pinned this issue on Oct 17, 2026
  4. cemililik commented on Oct 19, 2026

    @cemililik
    CollaboratorAuthor

    Hi @krisnaparahita and @hiyouwhatsapp, thank you both for your interest in ForgeLM!

    A quick note so that nobody spends effort twice: @hiyouwhatsapp has already been working on this issue and opened #1025 with a fix and a regression test, which is close to ready. @krisnaparahita, if you were considering this one, there is no need to duplicate that work, but your help would be very welcome elsewhere. The open good first issue and help wanted issues without an assignee are a good place to start, and "Working on an issue" in CONTRIBUTING explains how to claim one with /assign.

    To both of you: contributions of any size are very welcome here, and if you find ForgeLM useful, a star on the repository would mean a lot to us.

    Thanks again for your time and care!

  5. added a commit that references this issue on Oct 20, 2026
    84b2cbd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: post-trainingCode area: post-trainingbugSomething isn't workinggood first issueGood for newcomersseverity: highWrong behaviour or a false claim users rely onsource: review-2026-10Found by the October 2026 code and documentation reviewwave: 1Roadmap wave 1: high severity, small effort

    Type

    No type

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions