Add import sorting to format.sh #25678
Conversation
|
Oddly, this change revealed an unused import in the GCP NodeProvider: |
|
This is awesome - I have been manually reordering files I have touched on. Should we also include the sorting step in pre-push/format script? Otherwise, we might just to rely on manual reviews for such things, which is definitely not gonna be sustainable in the long run (of course, if we really want them to be clean all the time). |
Yup that's the idea , might as well just add it to this diff |
Cleaned this up while we are at it |
clarng
requested review from
richardliaw,
ericl,
edoakes and
wuisawesome
as code owners
| @@ -138,6 +145,11 @@ MYPY_FILES=( | |||
| '_private/gcs_utils.py' | |||
| ) | |||
|
|
|||
| ISORT_PATHS=( | |||
| # TODO: Expand this list and remove once it is applied to the entire codebase. | |||
There was a problem hiding this comment.
Starting with autoscaler sgtm.
There was a problem hiding this comment.
Starting with autoscaler sgtm.
Not sure how to do the rest of the rollout -- maybe we can just do the rest in one shot in a follow-up?
After a wider e-mail heads-up, can do something similar to the rollout for Black, which involved auto-commenting open PRs with a warning to rebase/merge master.
are you referring to this e-mail ? https://mail.google.com/mail/u/0/#search/black+ray+/FMfcgzGmthdVKJtxBQZTLrkWQGLlKhxH did we also notify the ray community via some channel? |
Yep.
Not sure. Someone on the relevant internal Slack would know the details. (Balaji, who did the Black rollout, might not be available for another few weeks.) |
|
Mac C++ failure well-known and unrelated. Merging! |
|
Or not! Need more approvals :) |
|
Looks like we need some more code owner approvals for this -- |
| if ! git diff --diff-filter=ACRM --quiet --exit-code "$MERGEBASE" -- '*.py' &>/dev/null; then | ||
| git diff --name-only --diff-filter=ACRM "$MERGEBASE" -- '*.py' | xargs -P 5 \ | ||
| isort | ||
| fi |
There was a problem hiding this comment.
Nice, happy to see the incremental formatting path work here too.
|
Look like I was the last stamp. The java/c++ test failures don't look related. Merging. |
Why are these changes needed?
It will be easier to develop if we could use a tool to organize / sort imports and not have to move them around by hand.
This PR shows how we could do this with isort (black doesn't quite do this per psf/black#333)
After this PR lands everyone will need to update their formatter to include isort if they don't have it already, i.e.
pip install -r ./python/requirements_linters.txt
All future file changes will go through isort and may introduce a slightly larger PR the first time as it will clean up the imports.
The plan is to land this PR and also clean up the rest of the code in parallel by using this PR to format the codebase (so people won't get surprised by the formatter if the file hasn't been touched yet)
Related issue number
#25676
Checks
I've run
scripts/format.shto lint the changes in this PR.I've included any doc changes needed for https://docs.ray.io/en/master/.
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
Test cases
./scripts/format.sh (isort not installed)
pip install -r ./python/requirements_linters.txt && ./scripts/format.sh
./scripts/format.sh --files python/ray/autoscaler/_private/gcp/node_provider.py
./scripts/format.sh --all
./scripts/format.sh --all-scripts