Closed jsilter closed 1 month ago
Thanks a lot for this PR to add
Conv3d
support to IA³ and LoRA. Overall, this already looks pretty good. I made some small suggestions regarding the implementations, please check.Thanks also for adding the tests. I saw that you already made some adjustments with regard to tolerances. However, for me, they don't appear to be sufficient. When I run the tests locally on GPU, I still get these failing tests:
FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_019_Conv3d_1_LoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_020_Conv3d_2_LoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_021_Conv3d2_1_LoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_023_Conv3d_1_LoRA_with_DoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_024_Conv3d_2_LoRA_with_DoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_025_Conv3d2_1_LoRA_with_DoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_disable_adapters_with_merging_026_Conv3d2_2_LoRA_with_DoRA - AssertionError: assert False FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_020_Conv3d_2_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_021_Conv3d2_1_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_022_Conv3d2_2_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_024_Conv3d_2_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_025_Conv3d2_1_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_merge_layers_026_Conv3d2_2_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_019_Conv3d_1_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_020_Conv3d_2_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_021_Conv3d2_1_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_022_Conv3d2_2_LoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_023_Conv3d_1_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_024_Conv3d_2_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_025_Conv3d2_1_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_026_Conv3d2_2_LoRA_with_DoRA - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_049_Conv3d2_1_IA3 - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_050_Conv3d2_2_IA3 - AssertionError FAILED tests/test_custom_models.py::PeftCustomModelTester::test_safe_merge_051_Conv3d2_3_IA3 - AssertionError
I didn't check all in details, but did a spot check and it was always a matter of tolerances being too strict. Do you observe the same behavior?
I had seen this at first on my local computer (CPU) but having the Conv3d tolerances be the same as Conv2d fixed it. Testing on a few different GPUs:
RTX 2080 CUDA 12.6 - all tests pass V100 CUDA 12.2 - all tests pass A600 CUDA 12.2 - 7 failed, all Conv3d2 with LoRA
I just pushed an update with increased tolerances (57e7db9bc34079cdaeacea9d13b90a19ef5961c3), in which all tests pass.
Thanks a lot for the updates. I have a few more comments: ...
I implemented all of these changes. Although in the Dora case I wonder if having those 2 sub-classes just to avoid a single trinary statement is a bit over-engineered, if we're going to do that for LoRA (and IA^3 and maybe more) it probably makes sense to be consistent.
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.
@BenjaminBossan I don't have a specific realistic example at the moment. I'm working on analyzing CT images (in the vein of https://github.com/reginabarzilaygroup/Sybil) but nothing ready to be published. If you think a toy example would be useful I can add that to my todo list; video classification would be the obvious choice.
Thanks for the info, I agree something like video classification would be a good example. Would that be something you could take a look at soon or rather do it separately from this PR?
I'm not sure when I'll be able to get to it so I'd prefer to keep it separate if that's alright.
Support Conv3d layer in LoRA and IA3
Issue GH-2079