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

[WIP] [Formater] Replace black with ruff #335

Closed
wants to merge 2 commits into from

Conversation

Qubitium
Copy link
Contributor

@Qubitium Qubitium commented Mar 28, 2024

Reason for PR:

  1. ruff is faster
  2. ruff is PEP compliant
  3. ruff is much saner than black: black sometimes like to create new lines or bloat code into multiple lines for no logical reason
  4. ruff is compatible with black rules

The PR diff shows the improvements while staying both PEP compliant and retain/improve legibility.

# ruff version 0.3.4
pip install ruff==0.3.4

@Qubitium Qubitium changed the title Replace black with ruff [Formater] Replace black with ruff Mar 28, 2024
@Qubitium Qubitium closed this Mar 28, 2024
@Qubitium Qubitium reopened this Mar 28, 2024
@hnyls2002
Copy link
Collaborator

@Qubitium Why ruff --fix format code like this ?

https://github.com/sgl-project/sglang/pull/335/files#diff-192192a73b4db4b25f305dc49a35aeb3e86789d78b07c01b589243e2f2da9b18R8-R9

I tested but it does not compress the lines.

@Qubitium
Copy link
Contributor Author

@Qubitium Why ruff --fix format code like this ?

https://github.com/sgl-project/sglang/pull/335/files#diff-192192a73b4db4b25f305dc49a35aeb3e86789d78b07c01b589243e2f2da9b18R8-R9

I tested but it does not compress the lines.

  1. ruff removed all the imports that were never used in the file and thus the reduced lines. Is this what you are referring to?
  2. ruff tries to merge as much imports into a single line (88 chars) as possible if legibility rule is not broken. It will use single line for multiple imports for cases where single merge imports does not make sense. This is what I have observed.

@hnyls2002
Copy link
Collaborator

@Qubitium Yeah, I like it removing unused things. But I used ruff --fix to format the interpreter.py files, it did not merge multiple imports into a single line.

My ruff version is 0.3.4, and I do not know why.

@Qubitium
Copy link
Contributor Author

Qubitium commented Mar 28, 2024

@hnyls2002 We have the same ruff version so not sure our results are so different. I will investigate and check back. Did you run ruff python --fix or on a specific file?

@hnyls2002
Copy link
Collaborator

Oh, I just found out why... I did not run isort as isort will merge the imports at first...

@Qubitium
Copy link
Contributor Author

Qubitium commented Mar 28, 2024

@hnyls2002 ruff has isort builtin. I will test, if result same or similar, we can remove isort and directly use ruff.

astral-sh/ruff#465

EDIT: astral-sh/ruff#633

Based on the isort support, it has limitations. We need to stick with isort for now.

@hnyls2002
Copy link
Collaborator

hnyls2002 commented Mar 28, 2024

@Qubitium

ruff python                                                                                 
warning: `ruff <path>` is deprecated. Use `ruff check <path>` instead.

It seems that your usage is not formatting but linting the repos, no wonder why it is likely to compress multiple imports into one line, this is actually done by isort and has no relationship with ruff.

I tested ruff format and it still expands multiple imports into multiple lines, I also think this is the correct way. We need more exploration about this.

@Qubitium
Copy link
Contributor Author

@hnyls2002 Confirmed. isort is dong the import merges and russ is only fixing the safe lints errors. I will work on this more.

@Qubitium Qubitium changed the title [Formater] Replace black with ruff [WIP]: [Formater] Replace black with ruff Mar 28, 2024
@Qubitium Qubitium changed the title [WIP]: [Formater] Replace black with ruff [WIP] [Formater] Replace black with ruff Mar 28, 2024
@Qubitium Qubitium marked this pull request as draft March 28, 2024 10:39
@Qubitium Qubitium closed this Apr 23, 2024
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