Repository navigation
Conversation
githubnemo
left a comment
githubnemo
left a comment
There was a problem hiding this comment.
Thanks for the PR. At first glance the changes look sensible. I'm unsure about the injection order regarding tied modules, left a comment about that.
| # Only applicable if `ensure_weight_tying = True` for LoraConfig | ||
| if self._check_tied_module_exists(peft_config, key): | ||
| targets_to_tie.append(key) | ||
| targeted_module_names.append(key) |
There was a problem hiding this comment.
Can you explain to me why you moved the append here? Before the tied modules were added last to the targeted_module_names list, now they are getting added earlier if I'm not mistaken.
There was a problem hiding this comment.
Oh, true, true.
I moved the append earlier so tied targets would be included in the pre-injection validation, but I didn't notice that it would change the ordering of targeted_module_names.
I鈥檒l keep tied targets separate during discovery, validate them before any mutation, and append them at the original point so normal targets remain first and tied targets remain last. You think is worth adding a regression test asserting that ordering?
| if getattr(peft_config, "target_parameters", []): | ||
| target_parameter_names = set(peft_config.target_parameters) | ||
| for module_name in module_names_to_inject + targets_to_tie: | ||
| module = model.get_submodule(module_name) | ||
| for parameter_name, _ in module.named_parameters(recurse=False): | ||
| key = f"{module_name}.{parameter_name}" | ||
| if (key in target_parameter_names) or any( | ||
| key.endswith(f".{target_name}") for target_name in target_parameter_names | ||
| ): | ||
| raise ValueError( | ||
| f"Trying to wrap an `nn.Parameter` of layer '{module_name}' of type " | ||
| f"{type(module).__name__}, which is not a valid target. Make sure that this layer is not " | ||
| "also targeted with `target_modules`. For some models, PEFT will do this automatically, " | ||
| "try setting `target_modules=[]` to prevent it." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Let's also add a short comment about the specific intent of this block, i.e. # Validate potential collisions between targeted parameter names targeted module names and let's refactor the raising of the exception into its own helper function which we then use in create_and_replace_param as well.
|
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. |
This branch has not been deployed
3 participants
What does this PR do?
This makes adapter injection validate all targets before mutating the model.
Previously, if an earlier target was valid but a later target failed validation, PEFT could leave the model partially modified. This was especially noticeable when dynamically adding adapters: the configuration was rolled back, but adapter parameters from the failed injection could remain in the model.
Changes
add_adapter().Tests
tests/test_tuners_utils.py: 267 passed, 16 skippedtests/test_initialization.py: 449 passed, 2 xfailedtests/test_custom_models.py: 18,977 passed, 1,046 skippedtests/test_mixed.py tests/test_other.py: 98 passed, 1 skippedAlong the way, the broader suites caught a couple of ordering issues around target parameters and tied modules, so I kept the full-suite results here instead of relying only on some subsets.
Closes #3707
This PR was authored by a human. And written too.