Fix Trainable Tokens random-init unmerge - #3656
Conversation
BenjaminBossan
left a comment
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for fixing this bug, but there are some issues with the tests, please check.
| model.eval() | ||
| outputs_before = model(**X) | ||
| if issubclass(config_cls, TrainableTokensConfig): | ||
| with model.disable_adapter(): |
There was a problem hiding this comment.
We shouldn't need this extra call for trainable tokens. The issue is that for trainable tokens, we have init_weights=False. Just ensure that it's True before calling get_peft_model and we should be good.
There was a problem hiding this comment.
Updated the test to set config.init_weights = True before get_peft_model. The shared disable-adapter assertion now covers Trainable Tokens without the extra baseline call.
| config_kwargs = set_init_weights_false(config_cls, config_kwargs) | ||
| self._test_safe_merge(model_id, config_cls, config_kwargs) | ||
|
|
||
| @pytest.mark.parametrize(("target_module", "token_indices"), [("emb", [0, 1, 3]), ("lin0", [0, 1])]) |
There was a problem hiding this comment.
Let's make the first argument a string to stay consistent with the other tests.
There was a problem hiding this comment.
Updated the parametrization argument to the string form for consistency.
2 participants
Summary
Closes #3650.
Tests
pytest tests/test_custom_models.py -k trainable_tokens -q --no-cov --regression— 117 passed, 6 skippedruff check tests/test_custom_models.py src/peft/tuners/trainable_tokens/layer.pyruff format --check tests/test_custom_models.py src/peft/tuners/trainable_tokens/layer.pygit diff --checkAI assistance was used to investigate the failure and prepare the patch. I reviewed every changed line, understand the change, and ran the tests above.