-
Notifications
You must be signed in to change notification settings - Fork 649
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
Ruff fixes #984
Ruff fixes #984
Conversation
6616ebc
to
a1bb547
Compare
Thanks a lot for this contribution. Yes, I had contributed the Ruff configuration (which might still need some tweaking, just put this as a starting point). I agree that having proper formatting + linting in place is a definite must for a project like ours. Anyways, the issue why I haven't still followed through with this is that, unfortunately, our test suite is still flaky. The bounds on non-deterministic operations also are different based on GPU model, which makes the whole situation tricky as so far we depend on contributors running their tests locally or using our integration test pipeline on the HuggingFace side which has T4 GPU runners, but which we don't run commit for commit and which can't integrate directly with the PRs as Right now, this leaves us without GPU runners to run our test suite on every PR (or even commit) and we've shied away from implementing this as we're still rushing to get on top of the big maintenance load that had gone relatively unattended for quite a while. Our biggest topics right now are the FSDP + QLoRA and cross-platform, for the latter there's a couple of outstanding PRs. I'm mentioning PRs, because if we merge these fixes here first, then all 20ish open PRs might get merge collisions because of this. And for each fix, we also have to validate that it's not breaking anything as some of the Ruff fixes are not guaranteed to not introduce issues based on my current knowledge (correct me if I'm wrong). I'm afraid to do this because we have 2+ million downloads a month and given a certain degree of uncertainty so far my judgement has been to hold off with this, until other more high impact topics have been solved. However, I'm open to discuss this and agree that linting + formatting should be solved (preferably asap). The CI topic is also close to my heart, but currently the team has decided to hold off on that for the time being based on our priorities. Did you run the full test suite after applying the fixes? Maybe you could post the results? Yes, if you'd contribute a GitHub Action, I would definitely merge it as soon as we decide it's safe. What are your thoughts? Thanks for your initiative, I really appreciate having so many active contributors on this project! 🙏🏻 |
That's the case for any PR, though, isn't it? 😅 Merge conflicts will just need to be fixed one way or the other. This PR does not run a full
As mentioned in the pull request description, these fixes are mostly manual, i.e. suggested or flagged by Ruff, and applied (or marked As for whether issues can be introduced – any software can have bugs, Ruff included, but the Ruff team (disclaimer: I've contributed to Ruff but am not otherwise affiliated) has taken quite some care to make possibly-unsafe autofixes opt-in (
I'm the only current maintainer of Babel which has 28+ million downloads a month, so I'm aware of how that feels... 😄 Either way, even if changes are merged to
I wish I could – my primary dev machine is Apple Silicon, so it's not exactly CUDA conducive, nor even Running in a
|
Really cool, great work on Babel! That kind of stuff keeps the community thriving 🙌🏻 Ok, thanks for your input. All valid points and thanks in your interest in Also thanks for all the good tips + pointers, really appreciated 🤗 I've been wanting to improve our Hugging Face side integration pipeline to also run bitsandbytes tests and not just the HF integration tests anyways. Let me take this as an occasion and take a look these days and then see what our tests say and review your PR. Getting the tests to be non-flaky on that pipeline / GPU model will be another big win. You're right, better to get this merged :) Thanks again for your work and kind words. If you feel like contributing the GH action, that would also be very appreciated. We've so much going on to get on top maintenance since we took over that having that solved would be a big help! |
Good luck 😅 I hope the pipeline won't remain just an "integration pipeline on our side" that contributors at large won't have any visibility to, or that tests devolve into "oh yeah, it works for our in-house developers but is red for external contributions 🤷" (looking at Gradio's CI right now...). On that note, I think it would be super beneficial to get #949 merged. Would be nice if HF could donate CPU + CUDA GPU time in the form of Github Actions runners!
Rebased + added a commit to add it (as a pre-commit configuration).
Ah, is HF (if I understood who "we" is correctly) maintaining |
* do not autofix always * be less strict around tests and benchmarks * adjust ignores for now
@Titus-von-Koeller Could this get merged? I rebased it to fix conflicts (and added a commit to fix a bug introduced in #873 that would have been spotted by Ruff... 😄) |
Yes, with HF I refer to Hugging Face. I was with bitsandbytes already before, but after using my savings to work on BNB full-time, they allowed me to continue my work by paying me and they now support bitsandbytes otherwise as well. Yes, they already said that they'd sponsor compute for the runners, but there's no managed solution available and with the cross-platform effort we need a whole matrix of accelerated runners with Linux, Windows, Mac, Nvidia, AMD, Intel, Apple Silicon.. We're still shying away from the pipeline logic of spinning up runners on demand, but that's definitely gonna happen within the coming 3 months or so. The integration pipeline on the HF side was just because we have easy access to accelerated runners there (HF Github organization) and needed a quick fix to have visibility of the integrations. It was not to exclude anyone. Anyways, I totally agree that visibility on PRs is super important and we'll try to get that working soon enough. It's just that we have a ginormous amount of work to get on top of and the GPU runners was decided to not be the most urgent. By us, I mean BNB maintainers btw. Right now that is pretty much mainly me, with @younesbelkada chipping in when he finds some time and Tim Dettmers not available for the next 3 months due to his applications in accademia, but consulting every now and then. I should also be working on high impact stuff as the FSDP + Qlora integration, etc, so right now it's a lot on my plate. But I'm trying to get to a point where I'm not blocking community contributions anymore and we have a plan that allows the community to chip in as much as possible, also in regard to which technical solutions we go for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very helpful contribution! Thanks for being so considerate and the initiative of tackling this heads-on. Reviewed it thoroughly and happy to merge it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for addressing this @akx - very nice contribution ! Thanks @Titus-von-Koeller for the detailed review!
The CI is failing due to some files not correctly formatted, could you have a look before merging? 🙏
@younesbelkada Yeah, just fixed it. Had to go to do sports first, so you caught me in the middle of messing things up by trying to fix the merge conflict from before in the web editor 😅 Anyways, @younesbelkada please be sure to also
We have to add that to the docs as well, once we have those up. |
706ec24
into
bitsandbytes-foundation:main
Awesome - thanks so much @Titus-von-Koeller ! 🚀 |
Hey @akx, Unfortunately a bunch of the integration tests from the Hugging Face side broke based on some of the changes that we merged within the last day. Would you be able to look into this? Unfortunately, this week is very tight for me, as I'm going to be off from this Saturday for 9 days. Actually, that's why we'll also hold off on a release until I'm back (of course we shall also work toward CD in the mid/long-term). I agree that it would be nicer to trigger this pipeline for PRs in this repo, but that's what we currently have to work with for the time being. Definitely sth to improve sooner rather than later, but for now it works. Find the CI logs below: If you're too busy, please let me know and we'll figure out another way. |
@Titus-von-Koeller I can take a look, but those logs don't help an awful lot, unfortunately. I'll try to set up WSL2 on my only Nvidia-equipped machine to try and reproduce, but of course you can also try to As for releasing a new version, is there pressure from... somewhere to do that? AFAICS there aren't very exciting things in |
We should run those tests for every commit on main anyways (as long as we don't have proper CI for PRs within bnb). But for now it's manual trigger or nightly. I wasn't mindful enough to trigger after every PR, lesson learned. Yeah, setting things up on WSL2 with Nvidia would be cool, especially since you're so involved with the repo now :) WSL2 support is also important, so if you come across anything, let us know. Nono, currently there's no pressure to release. No worries. I would aim to have a release in 2-3 weeks. I don't think I'll get around to Git bisect this week. Todo is already too long. |
You did mention earlier that some tests are flaky – can you double-check if those tests that supposedly fail due to recent changes had ever failed flakily in the past?
Well, now that you mention it, to begin with:
I didn't get around to running |
@Titus-von-Koeller Based on
and I really don't think that has anything to do with my changes 😁 Instead, #991 seems like it could be related... Also: practically every test in
warning. Would be nice if they didn't. EDIT: I also finished a run of
These don't seem very related either. For completeness, I did
|
@akx sorry for the trouble ! huggingface/transformers#28788 should fix the failing tests |
This PR applies a bunch of fixes suggested by the Ruff linter (which has been configured since 2c605d0 (#896), just never really run).
Some of these are mechanical auto-fixes (
--fix --select=I,F401
style), but most of them are manual.The commits themselves are atomic and small enough to be able to be reviewed one by one.
It would probably be a good idea to enforce linting with Ruff in CI; if the maintainers like, I can contribute a the requisite GitHub Actions configuration too :)