Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AdaLora: Trigger warning when user uses 'r' inplace of 'init_r' #1981

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

bhargavyagnik
Copy link
Contributor

Description:
This PR addresses Issue #1971 by adding a warning in __post_init__ of AdaLoraConfig when r is set to any non-default value. The changes aim to clarify the usage of init_r instead of r in AdaLoraConfig, resolving confusion around parameter naming.
Updated the docstring example in AdaLoraModel to reflect the same.

Testing:

  • ✅ Verified that the warning is correctly triggered when 'r' is set in AdaLoraConfig
  • ✅ Ensured that existing functionality is not affected by these changes
  • ✅ Checked that the updated docstring example works as expected

Related Issues
Closes #1971

@bhargavyagnik
Copy link
Contributor Author

@BenjaminBossan is this what you were suggesting in the comment on #1971

@BenjaminBossan
Copy link
Member

BenjaminBossan commented Jul 31, 2024

Thanks for working on this suggestion. The PR looks good (nice catch adjusting the docstring). Would you be so kind as to also add unit tests for this warning? So in tests/test_config.py, create a test where an AdaLoraConfig with r is being initialized and check that the correct warning was given? You should be able to use the with pytest.warns context manager.

Also, please ensure that the character limit of 120 chars per line is kept.

@bhargavyagnik
Copy link
Contributor Author

Thank you for reviewing. I have added the test for AdaLoraConfig warning.

@BenjaminBossan
Copy link
Member

Thanks, could you please run make style?

@bhargavyagnik
Copy link
Contributor Author

Sorry, previously i could not run make properly and found out my ruff (0.5.5) version was different and was getting numerous errors. I did it with ruff==0.4.10 as you suggested in one of the PRs. Thanks.

@HuggingFaceDocBuilderDev

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.

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this so quickly!

@BenjaminBossan BenjaminBossan merged commit 269aba5 into huggingface:main Aug 1, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hyper-paramter r in adalora_confg and lora_config
3 participants