Skip to content

Conversation

czgdp1807
Copy link
Contributor

Why are these changes needed?

This is as suggested by @aslonnie (I hope I have done the right thing).

On my linux system, ruff test passes,

+ pre-commit run ruff --all-files --show-diff-on-failure
[INFO] Installing environment for https://github.com/astral-sh/ruff-pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
ruff.....................................................................Passed
ruff.....................................................................Passed

Related issue number

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 :(

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes a number of Python file paths from the per-file-ignores list for ruff's isort rules. The goal is to apply import sorting checks to more of the codebase. However, my review found that the primary isort rule, I001, is currently disabled globally. This makes the change a no-op, as no import sorting will be enforced on these files. I've added a comment suggesting to either enable I001 in this PR or combine this change with a future PR that does, to ensure configuration changes are effective and not misleading.

@ray-gardener ray-gardener bot added core Issues that should be addressed in Ray Core devprod community-contribution Contributed by the community labels Aug 27, 2025
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

hmm.. I am really surprised there are no changes required..

I guess we also need to add the filters into here:

files: '^python/ray/serve/|^python/ray/train|^python/ray/data|^python/ray/_private/|^python/ray/llm/|^python/ray/tune/'

@@ -61,20 +61,7 @@ afterray = ["psutil", "setproctitle"]
# reformat the code to follow the style guide.
[tool.ruff.lint.per-file-ignores]
"doc/*" = ["I"]
"python/ray/__init__.py" = ["I"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we keep this __init__.py ?

and also remove L59? which says setup-dev.py needs to be exluded?

@czgdp1807 czgdp1807 requested a review from a team as a code owner August 28, 2025 04:48
@czgdp1807 czgdp1807 requested a review from aslonnie August 28, 2025 14:39
@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Aug 28, 2025
@aslonnie
Copy link
Collaborator

triggered full premerge and see how things go.

please split this into smaller PRs with smaller impact radius per PR. thanks.

@czgdp1807
Copy link
Contributor Author

All tests pass. I am starting to work on splitting this PR into smaller chunks. :D.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Contributed by the community core Issues that should be addressed in Ray Core devprod go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants