MNT: Move tuners to subpackages

[BenjaminBossan]

As discussed internally.

Description

All tuners now live in a sub-directory consisting of multiple modules. The
directory structure, e.g. for LoRA, looks like this:

src/peft/tuners
└── lora
    ├── __init__.py
    ├── bnb.py
    ├── config.py
    ├── gptq.py
    ├── layer.py
    └── model.py

Note that I moved the quantized layers, bnb and gptq, to their own respective
modules.

Regarding the changes, no code was actually changed, just moved around. The only
minor change I made was to use absolute imports instead of relative imports,
except for the local files in the same directory:

# instead of this
from ...import_utils import is_bnb_4bit_available
from ..tuners_utils import BaseTuner
from .config import LoraConfig

# do this
from peft.import_utils import is_bnb_4bit_available
from peft.tuners.tuners_utils import BaseTuner
from .config import LoraConfig

To repeat some of the pros and cos we discussed:

• more structure
• smaller files
• if we have to do it eventually, the earlier the better

• is a bit of work
• messes with git history (git blame)

AFAICT, it does not actually resolve any circular dependency issues.

For some existing modules, the license comment of the top of the file
was missing, I always added it.

Comments

Note that GitHub shows some diffs as movements and some as completely new files.
Not much that we can do about it. This makes review a bit harder but all tuners
were treated the same, only the diff is shown differently.

Also note that only files inside of peft/tuners/ were changed, all the rest is
still the same. In particular, no imports were changed anywhere else. This
should (hopefully) mean that the changes are completely backwards compatible.

We have a few open PRs that add new methods. I wouldn't ask of those
contributors to refactor the code, as it is a bit of work. However, I think we
should make them aware of that new structure so that they can change their code
if they want to. Maybe that's in their interest so that their name appears when
looking at the git blame.

Additional tests I ran locally:

• single GPU
• unit tests without bnb installed

[pacman100]

Thank you @BenjaminBossan for refactoring. I am trying to think about how the sub-package would look like for prompting-based methods. Will it be 2 files config.py and model.py?

[BenjaminBossan]

I don't think that all methods need to have the exact same structure, only when it makes sense. So your suggestion would be an option.

Also, given some recent issues, we could investigate if moving the bnb layers (and imports) to a submodule could help.

[younesbelkada]

Thanks a lot for working on this @BenjaminBossan
I really like the refactor as it makes the code more readable ! I also believe this will also make things easier in the future in the case we want to add new methods.
Regarding git history I think it's fine (I think that we want to do such a refactor at some point so better to do it sooner rather than later). What do you think?
Also regarding non-lora methods, I agree with what sourab said, it will be probably 2 files

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[BenjaminBossan]

@pacman100 @younesbelkada I finished the PR and it's now ready for a proper review. The original description has been updated to reflect that. Ideally, we can proceed fast with this PR, as it can easily cause merge conflicts.

[younesbelkada]

Thanks a lot, this looks super nice! I am keen to add docstrings in missing places in a follow up PR
Let's also wait for @pacman100 's approval before merging

[BenjaminBossan]

@pacman100 @younesbelkada

Okay, so I merged the changes from #851 and at the same time fixed a bug and wrote a test for it. As shortly discussed internally, the bug that was introduced is for the case that the model is merged AND adapters are disabled. For that scenario, we need to unmerge first before generating the output, same as we do for the vanilla Linear layer. This step was missing from the code previously.

To catch this bug, I extended the tests to include a test case with merging + disabling. This test fails with the current code but passes with the bugfix.

Moreover, I had to make the tests more precise. So far, they were checking the generated token IDs, but that's very imprecise. On the other hand, checking the logits is also not nice, since they can deviate quite a lot because of the rounding errors. Instead, I'm now checking in probability space, where we can see differences, but the rounding error does not affect them too much.

Unfortunately, this still requires a bit of fiddling with tolerances, and I'm not sure if the ones I found on my machine will generalize. Therefore, it would be really good if you could check the changes. Basically, running pytest tests/ -k "test_4bit_merge and lora" should succeed. On the other hand, we can just go ahead, merge, and see if the nightly GPU tests pass with the given tolerances.

[younesbelkada]

Thanks again! As discussed offline I can confirm the new tests pass and the softmax test is the best compromise for testing whether merging 4-bit layers works as expected or not !

[pacman100]

Thank you @BenjaminBossan for creating subpackges and refactoring code to be modular, understandable and simpler. Kudos to all the effort you put into this, super helpful! 🚀

LGTM, just a couple of comments.