-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[WIP] TST: Clean up testing #2846
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
base: main
Are you sure you want to change the base?
[WIP] TST: Clean up testing #2846
Conversation
There is still some chaos in our test suite, despite recent efforts to refactor it. This PR tries to improve the situation a bit. Currently, some tests are expected to fail. This can be due to errors in this PR but some errors may be genuine bugs. I'll investigate further. Some of the changes: - don't add return to pytest.skip, it's not necessary - avoid self.skipTest, always use pytest.skip - unused CONFIG_TESTING_KWARGS is removed - factor out common skipping logic into dedicated functions - many tests had code like: skip unless it's LoRA or IA³ or ... often, new PEFT methdos would actually work but were not added to this list, resulting in unnecessary skips
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. |
@Phoveran I also noticed that we missed a TODO: peft/src/peft/tuners/c3a/model.py Line 33 in 2813b9c
|
Hi, Thanks for the notice! I have added the paper link in my fork. |
Nice, could you please create a PR?
You would have to use my branch from this PR. It removes a lot of skipping logic from the tests that I found unnecessary. But there is no need for you to get active, I already fixed the issue in C3A, my ping is just to let you know and for you to double check my change. |
Got you. The PR is ready. Thanks! |
* more BOFT dtype fixes * fix faulty test
Currently, adapter deletion raises an error with DeLoRA. The reason is that the dropout module is called module_dropout, i.e. the prefix "delora" is not part of the name, which is required for proper working. This PR renames the attribute to delora_dropout. The reason why this was not caught is because the corresponding test was not updated to include DeLoRA. The test was thus also changed. Note: I came across this issue in huggingface#2846 but I wanted to fix it in a separate PR, as huggingface#2846 is probably not going to make it into the next PEFT release.
Note on some of the failing tests:
|
There is still some chaos in our test suite, despite recent efforts to refactor it. This PR tries to improve the situation a bit.
Some of the changes:
pytest.skip
, it's not necessaryself.skipTest
, always usepytest.skip
CONFIG_TESTING_KWARGS
is removedcayley_batch
always returning float32 dtype (ping @zqiu24)delta_weight
for WaveFT merging if base layer is Conv1D (ping @Bilican)UnboundLocalError
in C3A (ping @Phoveran)For those that I pinged, this is just to let you know I made some fixes on PEFT methods that may concern you and ask you, if you have time, to double check them.
Errors not fixed in this PR (yet):