Skip to content
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

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Jan 29, 2024

Follows up on #992.

Also re-wraps the argument list (in the same way black or ruff format would) so the line is a bit more readable.

@akx akx mentioned this pull request Jan 29, 2024
@Titus-von-Koeller
Copy link
Collaborator

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.

@Titus-von-Koeller Titus-von-Koeller merged commit 277ac27 into bitsandbytes-foundation:main Jan 29, 2024
1 check passed
@akx
Copy link
Contributor Author

akx commented Jan 29, 2024

I ran both yapf (using the current rules in the repo) and ruff format (i.e. black, but faster) against the repo and the only difference I can quickly spot between the two is around trailing parentheses (here in an if, but function calls and declarations are affected too); the Black style does

    elif (
        g.dtype == torch.bfloat16
        and state1.dtype == torch.uint8
        and len(str2optimizer8bit_blockwise[optimizer_name]) == 3
    ):

and Yapf would do

    elif (g.dtype == torch.bfloat16 and state1.dtype == torch.uint8
          and len(str2optimizer8bit_blockwise[optimizer_name]) == 3):

I personally find the Black style much more readable and Pythonic...

@Titus-von-Koeller
Copy link
Collaborator

@akx

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.

@Titus-von-Koeller
Copy link
Collaborator

I just wrote the following:

The only other solution that I see is to use a separate formatter for C, C++, CUDA code. In that case we could apply Black selectively to just Python (and does it format.

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.

@akx
Copy link
Contributor Author

akx commented Jan 30, 2024

Yeah, I was kind of wondering why clang-format was mentioned at all in the context of Python code 😅

Both Black and ruff format are strictly Python formatters. I too prefer the clang-format style for C(++, etc) code.

@akx akx deleted the correct-992 branch January 30, 2024 06:06
@Titus-von-Koeller
Copy link
Collaborator

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.

@Titus-von-Koeller
Copy link
Collaborator

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.

@akx akx mentioned this pull request Feb 4, 2024
akx added a commit to akx/bitsandbytes that referenced this pull request Feb 5, 2024
As discussed in bitsandbytes-foundation#994 (comment), YAPF won't be used here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants