Dequantize the bitsandbytes base before the vLLM weight push - #6922
Open
behroozazarkhalili wants to merge 6 commits into
Open
Dequantize the bitsandbytes base before the vLLM weight push#6922behroozazarkhalili wants to merge 6 commits into
behroozazarkhalili wants to merge 6 commits into
Conversation
A 4-bit base is stored as a flat packed uint8 buffer whose scales live in `quant_state`. The weight sync read `param.data`, which drops `quant_state`, and pushed that buffer into vLLM unchanged. On Qwen2.5-3B, 14 of 27 tensors failed the shape assert in vLLM's weight loader, and no scales crossed at all. Route every push site through `_dense_param_data`, which dequantizes a `Params4bit` back to the model dtype and returns any other parameter's data unchanged. The dequantization has to act on the parameter object, because `.data` has already discarded the quantization state. Build the colocate engine dense to match. It previously received `quantization="bitsandbytes"` whenever a `Linear4bit` was present, which allocates packed `[out_features, in_features // 2]` weights and rejects a dense push with the same assert. The two halves only work together. The cost is that the vLLM engine holds a full-precision copy of the base, so the QLoRA memory saving covers training but not the rollout engine. online_dpo_trainer.py duplicates this block and is updated in lockstep.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
FSDP2 syncs weights from `state_dict()`, which returns plain tensors. The bitsandbytes `quant_state` holding the scales is dropped there, so `_dense_param_data` has nothing to dequantize from and the packed storage reaches vLLM unchanged. Before the engine was made dense this was silent: packed weights met a packed engine and the scales simply never crossed. Now the same push hits the shape assert instead. Neither is correct, so detect the combination when the engine is built and say what is wrong. The check covers FSDP2 only. The FSDP1 path reads parameters through `summon_full_params` and has not been measured, so it is deliberately left alone rather than guarded on an assumption. online_dpo_trainer.py carries the same weight-sync block and is updated in lockstep, using the `fsdp_plugin` lookup that file already uses.
The branch was 21 commits behind and GitHub reported it unmergeable. Main had meanwhile restructured the vLLM weight-sync path into _iter_named_params, which is the function this branch changes, so the conflict landed squarely on trl/generation/vllm_generation.py. Resolved by taking main's version of the generator and re-applying the three changes this branch makes on top of it: the _dense_param_data helper, the _check_quantization_supported guard extracted from __init__, and the three yield sites that now route through the helper instead of param.data. Verified after resolution rather than assumed: the helper is defined once and called at all three sites, the guard is defined and called once, no stale bare `quantization` name survives, and the file compiles. Running tests/test_vllm_client_server.py on the merged tree and on origin/main gives 0 failures on both, with the merged tree collecting exactly the two test functions this branch adds and dropping none.
The two guards this PR adds had no committed test. Both are pure functions over a module, so they run without an accelerator or a live vLLM engine. _check_quantization_supported gets seven cases: the two that must raise, an 8-bit base at any FSDP version and a 4-bit base under FSDP2, and five that must not, including a 4-bit base under FSDP1 and with no FSDP at all, since FSDP1 gathers through summon_full_params and keeps the quant_state that FSDP2's state_dict has already dropped. _dense_param_data gets a passthrough case asserting an unquantized parameter comes back as the same storage. The negative cases are the point. A guard that raises on everything would pass a suite that only tested the failures. Checked by mutation rather than by reading: flipping the FSDP2 version comparison, deleting the 8-bit raise, and breaking the passthrough each turn the suite red, and all three revert to green.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ea30fad. Configure here.
`_check_quantization_supported` declared `fsdp_version: int` and documented
`0` for no FSDP. Neither matches the caller. `DistributedBackend` sets
self.fsdp_version = getattr(fsdp_plugin, "fsdp_version", None) if fsdp_plugin else None
so the guard is handed `None`, not `0`, whenever FSDP is off, which is the
common case.
The guard still behaves correctly today, because `None == 2` is `False`. The
problem is that the annotation, the docstring and the tests all described a
sentinel that never reaches the function, so the contract was documented and
verified against itself rather than against the call site. Any later rewrite
to a numeric form such as `fsdp_version >= 2` would raise `TypeError` in
production while the suite stayed green.
Annotate `int | None`, document `None` as the no-FSDP value and name
`DistributedBackend` as its source, and add the three cases the caller can
actually produce: 8-bit with `None` must raise, 4-bit and dense with `None`
must not.
Checked by mutation, not by reading: substituting `>= 2` for `== 2` fails
exactly one test, `test_check_quantization_supported[<lambda>-None-None0]`,
one of the cases added here. The seven pre-existing cases all pass against
that mutant. Baseline and restored runs are 11 passed.
Reported by Cursor Bugbot on this PR.
…ase-before-vllm-push
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What does this PR do?
Fixes #4973. This is direction A from the discussion on that issue; direction B is still open, see below.
The report attributed the failure to
merge_adapter()dequantizing the base. That is not what happens: PEFT'sLinear4bit.mergeleaves the weight asParams4bit, so nothing is upcast.The measured cause is different. bitsandbytes stores a 4-bit weight as a flat packed
uint8buffer whose scales live inquant_state. The sync path readparam.data, which dropsquant_state, and handed that buffer to vLLM. On Qwen2.5-3B, 14 of 27 tensors failed the shape assert in vLLM's weight loader, and 0quant_statetensors were transferred, so the scales never crossed even for the tensors whose shapes did fit.The change
Every push site now goes through
_dense_param_data, which dequantizes aParams4bitback to the model dtype and returns any other parameter's data unchanged. The dequantization acts on the parameter object rather than on.data, because.datahas already discarded the quantization state by the time the push sees it.The colocate engine is built dense to match. It previously received
quantization="bitsandbytes"whenever aLinear4bitwas found, which allocates packed[out_features, in_features // 2]weights and would reject the dense push with the same assert. The two halves only work together.trl/experimental/online_dpo/online_dpo_trainer.pyduplicates this block and is updated in lockstep.Cost
The vLLM engine now holds a full-precision copy of the base, so the QLoRA memory saving covers training but not the rollout engine.
The alternative
Direction B, sending the packed weights together with
quant_stateand adding a--quantizationflag, keeps the engine quantized and avoids that cost. It is a larger change and I have not built it. If you prefer B, say so and I will close this.Verification
ruff check,ruff format --checkand the pinneddoc-builder stylepass on both files, each run alongside a control so a silent no-op would show up.Params4bitroutes todequantize_4bitwithquant_stateforwarded and the packed shape intact.Note
Medium Risk
Touches every vLLM weight-sync path and changes colocate engine quantization defaults; behavior is well-guarded for unsupported combos but increases GPU memory for QLoRA rollouts.
Overview
Fixes QLoRA + vLLM weight sync by dequantizing bitsandbytes 4-bit parameters before they are pushed to vLLM, instead of sending packed
param.data(which dropsquant_stateand breaks shape checks).Adds
_dense_param_data(4-bit → dense viadequantize_4bit) and routes all server/colocate sync sites through it inVLLMGenerationandOnlineDPOTrainer. ColocateLLMinit no longer setsquantization="bitsandbytes"so the engine expects dense weights matching the push.Adds
_check_quantization_supported: still rejects 8-bit bases; fails fast on 4-bit + FSDP2 (weights fromstate_dict()cannot be dequantized). Unit tests cover both helpers under@require_bitsandbytes.Trade-off: the vLLM rollout engine holds a full-precision copy of the base; QLoRA memory savings apply to training, not the colocated engine.
Reviewed by Cursor Bugbot for commit 8da2109. Bugbot is set up for automated code reviews on this repo. Configure here.