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.

Sign up for GitHub

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

Jump to bottom

[CI][Linter] Replace flake8 and isort with ruff #49435

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[CI][Linter] Replace flake8 and isort with ruff #49435

wants to merge 8 commits into from

Conversation

MortalHappiness
Copy link
Member

@MortalHappiness MortalHappiness commented Dec 25, 2024 •
edited
Loading

Why are these changes needed?

This PR replaces flake8 and isort with ruff.

To minimize changes, all rules that cause file modifications are ignored.

Although this PR involves many files, the changes to Python files are limited to modifications of flake8 comments.

The latest version of ruff does not support disabling checks for SyntaxError, so python/ray/serve/tests/test_config_files/syntax_error.py is explicitly excluded in pyproject.toml. See https://docs.astral.sh/ruff/rules/syntax-error/ for details.

The context of why to replace flake8 with ruff is that while running the pre-commit hook of flake8, an error occurs if the Python version is 3.12. This is because the version of flake8 is too old. Then, @jjyao suggested that I replace it with ruff instead of upgrading flake8. See #47991 (comment) for details.

Follow-up:

Related issue number

Closes #48508

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@MortalHappiness MortalHappiness added the go add ONLY when ready to merge, run all tests label Dec 25, 2024
@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch 2 times, most recently from 18e5b64 to 18ead33 Compare December 25, 2024 10:47
@MortalHappiness MortalHappiness changed the title [CI][Linter] Migrate to ruff [CI][Linter] Replace flake8 and isort with ruff Dec 25, 2024
@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch from 3474ec9 to aab7d03 Compare December 25, 2024 11:27
@MortalHappiness MortalHappiness marked this pull request as ready for review December 25, 2024 15:20
MortalHappiness added a commit to MortalHappiness/ray that referenced this pull request Dec 25, 2024
@MortalHappiness
[TEST] Debug CI for ray-project#49435
6c6f981 
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch from aab7d03 to 2e84c6a Compare December 25, 2024 18:08
dayshah
dayshah approved these changes Dec 25, 2024
View reviewed changes
Copy link
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blazing fast linting ⚡️

@MortalHappiness MortalHappiness marked this pull request as draft December 27, 2024 00:06
@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch 10 times, most recently from df361dc to e2ae29c Compare December 27, 2024 02:03
@MortalHappiness MortalHappiness marked this pull request as ready for review December 27, 2024 02:22
@rynewang
Copy link
Contributor

Needs Ray Data, RLLib approval, @sven1977 @bveeramani

@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch from e2ae29c to 7a5cd1a Compare December 27, 2024 16:29
@MortalHappiness
[CI][Linter] Migrate to ruff
7c4c513 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Fix SyntaxError files
ef84628 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Run ruff pre-commit rule in CI
513e7a1 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Update noqa comments to use ruff
2fb2394 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Update requirements_compiled.txt
ed6f223 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Ignore autofixable rules
c319ce0 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Pin pre-commit version
4883242 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness
Fix conflicts
3e6224f 
Closes: ray-project#48508
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
@MortalHappiness MortalHappiness force-pushed the feature/#48508-migrate-to-ruff branch from 7a5cd1a to 3e6224f Compare December 28, 2024 00:36
jjyao
jjyao reviewed Dec 30, 2024
View reviewed changes
ci/lint/format.sh
Comment on lines -281 to -288
if [ $HAS_FLAKE8 ]; then
echo "$(date)" "Flake8...."
git ls-files -- '*.py' "${GIT_LS_EXCLUDES[@]}" | xargs -P 5 \
flake8 --config=.flake8

git ls-files -- '*.pyx' '*.pxd' '*.pxi' "${GIT_LS_EXCLUDES[@]}" | xargs -P 5 \
flake8 --config=.flake8 "$FLAKE8_PYX_IGNORES"
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to replace this with ruff?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment here #49435 (comment). These comments were folded.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about *.py?

Copy link
Member Author

@MortalHappiness MortalHappiness Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the Ruff pre-commit hook to automatically locate the corresponding Python files. See

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does CI run pre-commit hook or it only runs when people enabled pre-commit hook locally?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI runs pre-commit hook here:

@jjyao
Copy link
Collaborator

jjyao commented Dec 31, 2024

Let's wait until getting the approval from @aslonnie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjyao jjyao jjyao left review comments

@can-anyscale can-anyscale can-anyscale approved these changes

@dayshah dayshah dayshah approved these changes

@rynewang rynewang rynewang approved these changes

@bveeramani bveeramani bveeramani approved these changes

@edoakes edoakes Awaiting requested review from edoakes edoakes is a code owner

@zcin zcin Awaiting requested review from zcin zcin is a code owner

@GeneDer GeneDer Awaiting requested review from GeneDer GeneDer is a code owner

@akshay-anyscale akshay-anyscale Awaiting requested review from akshay-anyscale akshay-anyscale is a code owner

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo hongpeng-guo is a code owner

@justinvyu justinvyu Awaiting requested review from justinvyu justinvyu is a code owner

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@woshiyyya woshiyyya Awaiting requested review from woshiyyya woshiyyya is a code owner

@sven1977 sven1977 Awaiting requested review from sven1977 sven1977 is a code owner

@maxpumperla maxpumperla Awaiting requested review from maxpumperla maxpumperla is a code owner

@simonsays1980 simonsays1980 Awaiting requested review from simonsays1980 simonsays1980 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@jjyao jjyao

@simonsays1980 simonsays1980

@sven1977 sven1977

@bveeramani bveeramani

@rynewang rynewang

@dayshah dayshah

@aslonnie aslonnie

@can-anyscale can-anyscale

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] Replace flake8 and isort with ruff
10 participants
@MortalHappiness @rynewang @edoakes @can-anyscale @jjyao @bveeramani @dayshah @simonsays1980 @sven1977 @aslonnie