FIX Task-type add_adapter: forward autocast_adapter_dtype, stop mutating modules_to_save - #3668
Conversation
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for bringing this to our attention, I could indeed reproduce the bugs.
First, please split the PR into two, as the changes are unrelated. Please keep the autocast issue in this PR, as this is the significant error, the fix is rather low priority.
Regarding the autocast issue, the fix itself is good but the test needs work. Instead of putting the test into test_other.py, it should move to testing_common.py. Then the corresponding test files, test_seq_classifier.py and test_token_classification_qa.py. should be updated to call this common test. Let's also update test_decoder_models.py and test_encoder_decoder_models.py, even if they're not affected.
The parametrization of the test should be over the base model dtype with float16 and bfloat16 being tested. There is no need to explicitly test float32 or autocast_adapter_dtype=True. The modules_to_save part should be kept out of this PR.
3b19498 to
0e44a11
Compare
|
Thanks for the review. Split: this PR now only contains the Test restructure: the test moved to Parametrization is over the base model dtype, Tests (CPU, macOS, transformers main). The new test alone:
Reverting the source fix makes all SEQ_CLS/TOKEN_CLS/QUESTION_ANS cases fail, so it is a real This PR was written with AI assistance (Claude Code). I reviewed every changed line and ran |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for splitting off the other changes and for reworking the testing. I still have a comment about the test though, please check.
There is no
tests/test_token_classification_qa.pyon main
Sorry about the confusion, I had that file locally and forgot that it's not checked in.
| # The adapter created by get_peft_model is the reference: adapters added afterwards must end up with the | ||
| # same dtypes. A few PEFT methods deliberately keep some weights in float32 regardless of the base model | ||
| # dtype, hence the comparison against the reference instead of against `dtype` directly. | ||
| expected_dtypes = _get_adapter_float_dtypes(model, "default") |
There was a problem hiding this comment.
Hmm, I'm not a fan of getting the dtypes of the first adapter as reference. If a new PEFT method is added where this is already incorrect, then we wouldn't even notice. I think we should compare against the expected dtype from the base model, i.e. against dtype. To check this, I updated the test as follows:
def _test_add_adapter_no_autocast_adapter_dtype(self, model_id, config_cls, config_kwargs, dtype):
# With autocast_adapter_dtype=False, adapters that are added after the PeftModel was created must keep the
# dtype of the base model instead of being upcast to float32. This covers add_adapter, which some task types
# override, as well as load_adapter, which routes through add_adapter.
if issubclass(config_cls, PromptLearningConfig):
pytest.skip("Prompt learning does not create tuner layers whose dtype could be autocast.")
if config_cls == AdaLoraConfig:
pytest.skip("AdaLoRA does not support multiple adapters")
if issubclass(config_cls, ShadowConfig) and config_kwargs.get("task_type") == "SEQ_CLS":
pytest.skip("ShadowPEFT does not support multiple adapters for sequence classification")
def get_adapter_dtype(model, adapter_name):
dtypes = set()
for name, param in model.named_parameters():
if (model.prefix in name) and (adapter_name in name) and param.is_floating_point:
dtypes.add(param.dtype)
if not dtypes:
raise ValueError("Could not determine the dtype of this adapter")
return dtypes
expected_dtype = {dtype}
with hub_online_once(model_id):
model = self.transformers_class.from_pretrained(model_id, dtype=dtype)
config = config_cls(
base_model_name_or_path=model_id,
**config_kwargs,
)
model = get_peft_model(model, config, autocast_adapter_dtype=False)
assert get_adapter_dtype(model, "default") == expected_dtype
with tempfile.TemporaryDirectory() as tmp_dirname:
model.save_pretrained(tmp_dirname)
model.add_adapter("added", config, autocast_adapter_dtype=False)
assert get_adapter_dtype(model, "added") == expected_dtype
# load_adapter goes through the same add_adapter code path
model.load_adapter(tmp_dirname, adapter_name="loaded", autocast_adapter_dtype=False)
assert get_adapter_dtype(model, "loaded") == expected_dtypeThe test generally passes, with the exception of ShadowPEFT and the following three:
FAILED tests/test_encoder_decoder_models.py::TestEncoderDecoderModels::test_add_adapter_no_autocast_adapter_dtype[dtype0-IA3Config-config_kwargs12-peft-internal-testing/tiny-random-T5ForConditionalGeneration-calibrated] - AssertionError
FAILED tests/test_encoder_decoder_models.py::TestEncoderDecoderModels::test_add_adapter_no_autocast_adapter_dtype[dtype0-VBLoRAConfig-config_kwargs25-peft-internal-testing/tiny-random-T5ForConditionalGeneration-calibrated] - AssertionError
FAILED tests/test_encoder_decoder_models.py::TestEncoderDecoderModels::test_add_adapter_no_autocast_adapter_dtype[dtype0-OSFConfig-config_kwargs33-peft-internal-testing/tiny-random-T5ForConditionalGeneration-calibrated] - AssertionError
The reason why those fail is because they target some modules that are intentionally kept in float32 in T5:
So we would either have to extend the expected dtypes for those cases to include float32, not target wo, or use a different model than T5.
As for ShadowPEFT, we also see some float32 weights. I haven't checked the details, but I think that's intentional, so we'd have to add an exception there too.
PeftModelForSequenceClassification, PeftModelForTokenClassification and PeftModelForQuestionAnswering override add_adapter to inject their head into modules_to_save. All three accept and document autocast_adapter_dtype but called super().add_adapter() without passing it on, so PeftModel's default of True always won: adapters were upcast to float32 even when the user explicitly passed autocast_adapter_dtype=False. This also affected PeftModel.load_adapter for these task types, since it routes through the same override; its trailing cast_adapter_dtype call is a no-op when the flag is False, so the adapter stayed float32 after loading. Adds a common test, _test_add_adapter_no_autocast_adapter_dtype, parametrized over the base model dtype (float16 and bfloat16). It is called from test_seq_classifier.py and the new test_token_classification_qa.py, which cover the affected task types, and from test_decoder_models.py and test_encoder_decoder_models.py, which are not affected but guard against a regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback: taking the dtypes of the first adapter as the reference means a PEFT method that is already wrong would never be noticed. Compare against the base model dtype instead, as suggested. Two architectures need an exception: - T5 pins `wo` to float32 via transformers' `_keep_in_fp32_modules`, so an adapter on `wo` correctly inherits float32 from its own base layer. Rather than listing the affected configs (IA3, VBLoRA and OSF all resolve to targets that include `wo`), the expectation is widened to include float32 whenever the loaded base model actually holds float32 weights. Of the 13 models and 2 dtypes used by these four test files, only T5 in float16 hits that branch; T5 in bfloat16 and every other model keep the strict single-dtype check. - ShadowPEFT wraps whole decoder layers, so get_base_layer() returns a module without a `weight` attribute, _move_adapter_to_device_of_base_layer() cannot determine a dtype and returns early, and the per-layer shadow weights keep nn.Linear's float32 default regardless of autocast_adapter_dtype. Skipped, with the reason spelled out; it is a pre-existing gap unrelated to this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0e44a11 to
45d03c3
Compare
|
Thanks, that's a fair point — I've switched to your version of the test, including On the two exceptions: T5. The three failures come from I went with your first option (extend the expected dtypes) but computed it instead of listing the configs: The assertions use ShadowPEFT. I looked into it and I don't think it's intentional. One other change to your version: Results (CPU, transformers 5.16.1),
Reverting the fix in This PR was written with AI assistance (Claude Code). I reviewed every changed line and ran the tests listed above. |
Two related bugs in the task-specific
PeftModelsubclasses (PeftModelForSequenceClassification,PeftModelForTokenClassification,PeftModelForQuestionAnswering), both in the same__init__/add_adaptermethods. They are independent defects but touch adjacent lines, so they are here as two commits in one PR.1.
autocast_adapter_dtypeis accepted, documented, and then droppedAll three
add_adapteroverrides declareautocast_adapter_dtype: bool = Trueand document it, but callwithout forwarding it, so
PeftModel.add_adapter's defaultTruealways wins. A user who explicitly opts out still gets fp32 adapters on an fp16/bf16 base model, silently.This also reaches
PeftModel.load_adapter, which forwardsautocast_adapter_dtypeintoadd_adapter; its trailingcast_adapter_dtypecall cannot repair it, because that function returns early when the flag isFalse.Reproducing on
mainwith an fp16 base model —__init__is the control and behaves correctly, which isolates the fault toadd_adapter:(identical for TokenClassification and QuestionAnswering)
Fix: forward the argument. After it, all three rows are
{torch.float16}.2.
modules_to_saveis mutated in place and grows without boundSix sites do:
This mutates the list the caller passed in, and has no membership check, so it is not idempotent. Since PEFT stores the config by reference, reusing one config object — across CV folds or a sweep — appends the classifier names again each time, and retroactively corrupts the config of models already built from it. That corrupted list is what
save_pretrainedwrites toadapter_config.json:Fix: rebind rather than mutate, and dedupe while preserving order. The rebind stops the caller's list being touched; the dedupe makes repeat calls idempotent.
Scope note: this stops the growth and the aliasing, but the config attribute is still rebound to include the head names after first use, so reusing that same object for a later non-classification model still carries
["classifier", "score"]. Removing that too would mean deep-copying the config, which is a larger behaviour change and out of scope here.Tests
grep -rn "classifier_module_names\|qa_module_names" tests/currently returns nothing — this injection logic has no coverage at all, which is why both bugs survived. AddedTestTaskTypeAddAdapterAutocastDtypeandTestTaskTypeModulesToSaveintests/test_other.py, parametrized over all three task types: 24 tests covering the autocast path via bothadd_adapterandload_adapter(including theTruecase, so the tests also guard against over-correcting), config growth across repeatedget_peft_modelandadd_adaptercalls, the writtenadapter_config.json, and that the caller's list object is left alone.Verified non-vacuous: reverting fix 1 alone fails exactly the 6
autocast=Falsecases while the 6autocast=Truecases still pass; reverting fix 2 alone fails all 12 of its tests.pytest tests/test_other.py74 passed,tests/test_auto.py16 passed, ruff check/format and doc-builder style clean.(AI-assisted: implemented with Claude Code.)