-
Notifications
You must be signed in to change notification settings - Fork 638
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
Correct type annotation for quantize_4bit #994
Correct type annotation for quantize_4bit #994
Conversation
Good catch! We should definitely have a Ruff Github Action for immediate feedback on PRs. We would also need immediate feedback on not violating Python syntax of the lowest version we're supporting. Formatting is another topic. Tim doesn't like Black format at all, he's completely against it. He wants 'clang-format', which only Yapf supports, so I added a Yapf config. But still afraid to run it over everything, as there are a few manually formatted things that I don't want to necessarily "destroy", as long as we don't have a formatting config, which I'm not 100% certain about yet, because it's all manual and needs tweaking. |
277ac27
into
bitsandbytes-foundation:main
I ran both
and Yapf would do
I personally find the Black style much more readable and Pythonic... |
From what I can gather Tim is frustrated by Black's formatting of C-related code and this is done better in the c-format standard, which is a subset of the features that yapf offers. Not supporting his favorite C formatting standard is almost an absolute blocker for him. There seems to be no other mature formatter that fulfils this criterium. Its maintenance is handled by Facebook and I would say it's quite future-proof. I agree that the Black style is much more pythonic, but Yapf can also be configured to mimic Black's Python formatting, especially (if I remember correctly) the trailing parenthesis: I think it's just a single config switch. The philosophy of the tool is completely different: It's ultra configurable and I'm really not happy with its default settings. We'll have to iterate with that. Experimenting and choosing sth all at once seems preferable to me, but right now I don't have the time at all. However, having a squeaky clean code base would be super nice along with our plans to document the library, including adding doc-strings (potentially with doc-test) for Api auto-documentation. Personally, I get really bothered by the lack of the aesthetic of unformated text, so seeing a pre-commit hook implemented would give me a lot of joy. |
I just wrote the following:
But then went down the rabbit hole and did some more research and the internet seems to think that Black doesn't affect non-Python code files at all. No idea what issue Tim had there and right now he is not available for comment for such thing; at least for two more months. Hmm, I wonder what he meant. Seems for C++, C, CUDA yapf also doesn't support formatting. For that there are specific formatters, e.g. Clang-Format. Ok, gotta sleep. We can think about this some more. |
Yeah, I was kind of wondering why Both Black and |
I'll check with the other maintainers later this week and get back to you. Would be great to get formatting set up in the coming weeks. |
Short update: We have the go ahead from Tim to use Black + a clang-format compliant formatter for the C, C++, CUDA. Feel free to do a PR if this is sth you care about or I do it when I'm back from my time off on the 12.2. |
As discussed in bitsandbytes-foundation#994 (comment), YAPF won't be used here
Follows up on #992.
Also re-wraps the argument list (in the same way
black
orruff format
would) so the line is a bit more readable.