Fix: Add warning for adapter_name conflict with tuner #2254
Conversation
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for this PR. It is certainly not a bad idea to warn users if they come across this issue. I'd go even further and also add a warning when users first create a model with an overlapping adapter name. For that, we'd have to adjust:
Would you be interested in working on those too? If not, I can later tackle this in a separate PR.
Regardless of that question, it would also be great to add a unit test for the warning (i.e. that there is no warning on the happy path but a warning when an overlapping name is chosen). Here is an example of a test that could be used as a starting point.
|
Yes I'd happy to take that! |
30082ca to
c9846c1
Compare
|
Thanks for the guidance @BenjaminBossan ! I have added the warnings to both creation paths and the loading path plus an unit test. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks a lot for adding the other warnings and for providing the unit tests. In general, this all looks very good. I just have a small request, which is to break the one long test into multiple smaller tests. This improves readability and makes the tests useful (right now, if one assert fails, the checks further below are not executed).
One way is just to split this function into multiple smaller function. This results in a bit of code duplication but that's okay. Another suggestion is to create a class for all these tests. Inside the class, you can have define a pytest fixture for the duplicate code (like the model and config, check this test as an example). I'll leave this decision up to you. LMK if you have any questions.
c9846c1 to
91c9cf2
Compare
|
Hi thanks for the tips @BenjaminBossan ! Hope this does the job now 😄 |
|
@pzdkn Thanks for refactoring the test, it is now well structured. Could you please run |
|
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. |
…eft_model, add_adapter, and from_pretrained - Warn when adapter_name contains the tuner_prefix, which can cause weight reinitialization during model loading. - Create TestNamingConflictWarning test class
91c9cf2 to
ca95bd5
Compare
|
Done 👍 Happy for another round of review @BenjaminBossan |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for adding the warnings for this edge case in naming, and for adding the nice unit tests. The PR LGTM.
Please ignore the failing CI, they're unrelated to the PR. We currently have trouble with the CI runners.
Warn when adapter_name contains the tuner_prefix, which can cause weight reinitialization during model loading.
- The warning message was missing spaces between sentences. - Added ' around strings for clarity - For one warning, which extended another warning, put it at the start instead of the end, because the other warning can be quite long, leading to users missing the addition For more context on this warning, see huggingface#2254
- The warning message was missing spaces between sentences. - Added ' around strings for clarity - For one warning, which extended another warning, put it at the start instead of the end, because the other warning can be quite long, leading to users missing the addition For more context on this warning, see #2254
This pull request addresses an issue where adapter weights are incorrectly initialized when the adapter_name conflicts with the tuner_prefix during model loading. Specifically, it introduces a warning message to notify users when this conflict occurs, as this could lead to confusion or unintentional weight reinitialization.
Changes:
Impact: