huggingface/peft

`bitsandbytes` is imported eagerly in `peft/utils/loftq_utils.py`

function2-llx opened this issue · 5 comments

#1230 makes the import of bitsandbytes lazy, however, the fix seems to be imcomplete.

if is_bnb_available():
import bitsandbytes as bnb

Perhaps there can be a test case ensuring that the CUDA context is not initialized after importing peft, e.g.:

import torch
import peft
assert not torch.cuda.is_initialized()

@BenjaminBossan

Thanks for pointing this out, we can surely work on this, as it's a valid concern. Let us know if you're interested in creating a PR to fix this.

Hello, I can make a PR, but perhaps one month later. Is that okay?

Thanks! I'll hopefully have time to fix this earlier, as it's a bit annoying and should only require a few extra lines for the fix and test. If I don't find the time by then, I'll be happy to accept your PR.

I added a quick PR in #1683.

Unfortunately, the test that you proposed won't work so easily. The issue is that if we initialize CUDA in any other test, that test would fail. Therefore, it can only run when it's the very first test. This is not so easily achieved and we don't want the order of the tests to influence whether they succeed or fail.

I also thought about testing if bitsandbytes is in sys.modules after importing peft. But again, this won't because of the order dependency. We could "unload" bitsandbytes by deleting it from sys.modules, but even then, when importing peft, it is cached and bitsandbytes will never be imported (I checked without the patch in the PR and that test still passed).

Right now, I don't have a good proposal how to test this and we just have to be careful. If you have a better idea, let me know.

@BenjaminBossan Thank you so much for your PR and explanation! Your explanation makes sense to me. The side effect of importing seems to be a common issue, and I'll definitely pay attention to related issues and solutions, and share my ideas when available.