add bnb test marker

[Titus-von-Koeller]

Small utility to easily all tests that require bnb..

Unfortunately I'm on my Intel box right now and also don't have a fully functioning dev setup for accelerate. I think this should work as is, but be sure to give it a quick spin, as I can't execute the tests in my current setup.

[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.

[muellerzr]

This looks very nice! Small suggestion for cleaning things up some

(I can follow up where then we expand this to our other suites, e.g. FSDP, DeepSpeed, big model inference, core)

[muellerzr]

We can simplify this quite a lot by having the following at the top of the file:

from .imports import is_pytest_available
if is_pytest_available():
    from pytest import mark

And then in the code:

    if is_bnb_available():
        return mark.bnb(test_case)
    return unittest.skip("test requires the bitsandbytes library")(test_case)

[Titus-von-Koeller]

Wouldn't that throw an exception if someone would run the test suite without pytest present? I mean this whole discussion is a bit weird, I think we should just commit to pytest fully and that's it, but if we want to support people running without pytest as a runner, then we gotta leave it as is, imo...

Actually in Transformers I also bumped into a bunch of pytest things that would throw an exception if the test suite wouldn't be run with pytest, but seems noone does that anyways..

[muellerzr]

Our test suite is pytest + unittest. It's required in our test requirements in the .py:

extras["test_prod"] = ["pytest>=7.2.0,<=8.0.0", "pytest-xdist", "pytest-subtests", "parameterized"]

And correct, we only run our tests inside unittest.TestCases :) (Explicit pytest tests are reserved in accelerate only in /src/accelerate/test_utils/scripts

[Titus-von-Koeller]

Ok, so if we assume pytest is a hard requirement, then we don't need is_pytest_available(), right? We could just go ahead and import it. In case we assume someone might need to run these tests without pytest, then I think we need the code unmodified as I provided it (because in your suggestion mark would then be undefined, right?), unless I misunderstand sth?

[muellerzr]

It's a hard requirement, but this code exists in the base project. So it's still a test requirement and we need the check at the top :)

[BenjaminBossan]

Maybe I misunderstand your suggestion, Zach, but in case pytest is not installed, mark would be undefined, right? This would not lead to an error as long as require_bnb is only called inside of tests, which require pytest as a runner. But would this not possibly be detected as a linting error, as the linter cannot know this?

[muellerzr]

Yes. Agreed (was chatting offline with Titus about it).

I think it'd be fine if we did:

if bnb_available() and is_pytest_available():
   ...
return unittest....

Honestly it could be even simpler and we abstract out the pytest + unittest logic

[Titus-von-Koeller]

we did some brainstorming and after some back and forth came up with a very nice generic solution to apply markers for all requires_sth

[BenjaminBossan]

Thanks, LGTM, good discussion.