Add import sorting to format.sh (#25678)

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) Co-authored-by: Clarence Ng <[email protected]>
ray-project · Jun 13, 2022 · 73e1131 · 73e1131
1 parent 5e9a8eb
commit 73e1131
Show file tree

Hide file tree

Showing 46 changed files with 379 additions and 357 deletions.
diff --git a/.isort.cfg b/.isort.cfg
@@ -0,0 +1,11 @@
+[settings]
+# This is to make isort compatible with Black. See
+# https://black.readthedocs.io/en/stable/the_black_code_style.html#how-black-wraps-lines.
+line_length=88
+multi_line_output=3
+include_trailing_comma=True
+use_parentheses=True
+float_to_top=True
+
+
+known_first_party=ray
diff --git a/ci/lint/format.sh b/ci/lint/format.sh
@@ -9,6 +9,7 @@ FLAKE8_VERSION_REQUIRED="3.9.1"
 BLACK_VERSION_REQUIRED="21.12b0"
 SHELLCHECK_VERSION_REQUIRED="0.7.1"
 MYPY_VERSION_REQUIRED="0.782"
+ISORT_VERSION_REQUIRED="5.10.1"
 
 check_python_command_exist() {
     VERSION=""
@@ -22,6 +23,9 @@ check_python_command_exist() {
         mypy)
             VERSION=$MYPY_VERSION_REQUIRED
             ;;
+        isort)
+            VERSION=$ISORT_VERSION_REQUIRED
+            ;;
         *)
             echo "$1 is not a required dependency"
             exit 1
@@ -49,6 +53,7 @@ check_docstyle() {
 check_python_command_exist black
 check_python_command_exist flake8
 check_python_command_exist mypy
+check_python_command_exist isort
 
 # this stops git rev-parse from failing if we run this from the .git directory
 builtin cd "$(dirname "${BASH_SOURCE:-$0}")"
@@ -68,6 +73,7 @@ else
 fi
 FLAKE8_VERSION=$(flake8 --version | head -n 1 | awk '{print $1}')
 MYPY_VERSION=$(mypy --version | awk '{print $2}')
+ISORT_VERSION=$(isort --version | grep VERSION | awk '{print $2}')
 GOOGLE_JAVA_FORMAT_JAR=/tmp/google-java-format-1.7-all-deps.jar
 
 # params: tool name, tool version, required version
@@ -80,6 +86,7 @@ tool_version_check() {
 tool_version_check "flake8" "$FLAKE8_VERSION" "$FLAKE8_VERSION_REQUIRED"
 tool_version_check "black" "$BLACK_VERSION" "$BLACK_VERSION_REQUIRED"
 tool_version_check "mypy" "$MYPY_VERSION" "$MYPY_VERSION_REQUIRED"
+tool_version_check "isort" "$ISORT_VERSION" "$ISORT_VERSION_REQUIRED"
 
 if command -v shellcheck >/dev/null; then
     SHELLCHECK_VERSION=$(shellcheck --version | awk '/^version:/ {print $2}')
@@ -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.
+    'python/ray/autoscaler/_private/'
+)
+
 BLACK_EXCLUDES=(
     '--force-exclude' 'python/ray/cloudpickle/*'
     '--force-exclude' 'python/build/*'
@@ -183,14 +195,21 @@ mypy_on_each() {
     popd
 }
 
+isort_on_paths() {
+    for path in "$@"; do
+       echo "Running isort on files under $path"
+       isort "$path"
+    done
+}
+
 # Format specified files
 format_files() {
     local shell_files=() python_files=() bazel_files=()
 
     local name
     for name in "$@"; do
       local base="${name%.*}"
-      local suffix="${name#${base}}"
+      local suffix="${name#"${base}"}"
 
       local shebang=""
       read -r shebang < "${name}" || true
@@ -215,6 +234,7 @@ format_files() {
     done
 
     if [ 0 -lt "${#python_files[@]}" ]; then
+      isort "${python_files[@]}"
       black "${python_files[@]}"
     fi
 
@@ -236,6 +256,9 @@ format_all_scripts() {
     command -v flake8 &> /dev/null;
     HAS_FLAKE8=$?
 
+    # Run isort before black to fix imports and let black deal with file format.
+    echo "$(date)" "isort...."
+    isort_on_paths "${ISORT_PATHS[@]}"
     echo "$(date)" "Black...."
     git ls-files -- '*.py' "${GIT_LS_EXCLUDES[@]}" | xargs -P 10 \
       black "${BLACK_EXCLUDES[@]}"
@@ -293,6 +316,11 @@ format_changed() {
     # exist on both branches.
     MERGEBASE="$(git merge-base upstream/master HEAD)"
 
+    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
+
     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 \
             black "${BLACK_EXCLUDES[@]}"

diff --git a/python/ray/autoscaler/_private/_azure/config.py b/python/ray/autoscaler/_private/_azure/config.py
@@ -1,7 +1,7 @@
 import json
 import logging
-from pathlib import Path
 import random
+from pathlib import Path
 from typing import Any, Callable
 
 from azure.common.credentials import get_cli_profile

diff --git a/python/ray/autoscaler/_private/_azure/node_provider.py b/python/ray/autoscaler/_private/_azure/node_provider.py
@@ -10,18 +10,18 @@
 from azure.mgmt.resource import ResourceManagementClient
 from azure.mgmt.resource.resources.models import DeploymentMode
 
+from ray.autoscaler._private._azure.config import (
+    bootstrap_azure,
+    get_azure_sdk_function,
+)
 from ray.autoscaler.node_provider import NodeProvider
 from ray.autoscaler.tags import (
     TAG_RAY_CLUSTER_NAME,
-    TAG_RAY_NODE_NAME,
-    TAG_RAY_NODE_KIND,
     TAG_RAY_LAUNCH_CONFIG,
+    TAG_RAY_NODE_KIND,
+    TAG_RAY_NODE_NAME,
     TAG_RAY_USER_NODE_TYPE,
 )
-from ray.autoscaler._private._azure.config import (
-    bootstrap_azure,
-    get_azure_sdk_function,
-)
 
 VM_NAME_MAX_LEN = 64
 VM_NAME_UUID_LEN = 8

diff --git a/python/ray/autoscaler/_private/_kubernetes/config.py b/python/ray/autoscaler/_private/_kubernetes/config.py
@@ -6,8 +6,8 @@
 from kubernetes import client
 from kubernetes.client.rest import ApiException
 
-from ray.autoscaler._private._kubernetes import auth_api, core_api, log_prefix
 import ray.ray_constants as ray_constants
+from ray.autoscaler._private._kubernetes import auth_api, core_api, log_prefix
 
 logger = logging.getLogger(__name__)
 

diff --git a/python/ray/autoscaler/_private/_kubernetes/node_provider.py b/python/ray/autoscaler/_private/_kubernetes/node_provider.py
@@ -3,22 +3,17 @@
 import time
 from typing import Dict
 from uuid import uuid4
+
 from kubernetes.client.rest import ApiException
 
-from ray.autoscaler._private.command_runner import KubernetesCommandRunner
-from ray.autoscaler._private._kubernetes import (
-    core_api,
-    log_prefix,
-    networking_api,
-)
+from ray.autoscaler._private._kubernetes import core_api, log_prefix, networking_api
 from ray.autoscaler._private._kubernetes.config import (
     bootstrap_kubernetes,
     fillout_resources_kubernetes,
 )
+from ray.autoscaler._private.command_runner import KubernetesCommandRunner
 from ray.autoscaler.node_provider import NodeProvider
-from ray.autoscaler.tags import NODE_KIND_HEAD
-from ray.autoscaler.tags import TAG_RAY_CLUSTER_NAME
-from ray.autoscaler.tags import TAG_RAY_NODE_KIND
+from ray.autoscaler.tags import NODE_KIND_HEAD, TAG_RAY_CLUSTER_NAME, TAG_RAY_NODE_KIND
 
 logger = logging.getLogger(__name__)
 

diff --git a/python/ray/autoscaler/_private/aliyun/node_provider.py b/python/ray/autoscaler/_private/aliyun/node_provider.py
@@ -1,31 +1,29 @@
+import logging
 import random
 import threading
-from collections import defaultdict
-import logging
 import time
+from collections import defaultdict
 from typing import Any, Dict, List, Optional
 
+from ray.autoscaler._private.aliyun.config import (
+    PENDING,
+    RUNNING,
+    STOPPED,
+    STOPPING,
+    bootstrap_aliyun,
+)
+from ray.autoscaler._private.aliyun.utils import AcsClient
+from ray.autoscaler._private.cli_logger import cli_logger
+from ray.autoscaler._private.constants import BOTO_MAX_RETRIES
+from ray.autoscaler._private.log_timer import LogTimer
 from ray.autoscaler.node_provider import NodeProvider
 from ray.autoscaler.tags import (
     TAG_RAY_CLUSTER_NAME,
-    TAG_RAY_NODE_NAME,
     TAG_RAY_LAUNCH_CONFIG,
     TAG_RAY_NODE_KIND,
-    TAG_RAY_USER_NODE_TYPE,
+    TAG_RAY_NODE_NAME,
     TAG_RAY_NODE_STATUS,
-)
-from ray.autoscaler._private.constants import BOTO_MAX_RETRIES
-from ray.autoscaler._private.log_timer import LogTimer
-
-from ray.autoscaler._private.cli_logger import cli_logger
-
-from ray.autoscaler._private.aliyun.utils import AcsClient
-from ray.autoscaler._private.aliyun.config import (
-    PENDING,
-    STOPPED,
-    STOPPING,
-    RUNNING,
-    bootstrap_aliyun,
+    TAG_RAY_USER_NODE_TYPE,
 )
 
 logger = logging.getLogger(__name__)

diff --git a/python/ray/autoscaler/_private/aliyun/utils.py b/python/ray/autoscaler/_private/aliyun/utils.py
@@ -1,43 +1,43 @@
-import logging
 import json
+import logging
 
 from aliyunsdkcore import client
 from aliyunsdkcore.acs_exception.exceptions import ClientException, ServerException
-from aliyunsdkecs.request.v20140526.CreateInstanceRequest import CreateInstanceRequest
-from aliyunsdkecs.request.v20140526.DeleteInstanceRequest import DeleteInstanceRequest
-from aliyunsdkecs.request.v20140526.DeleteInstancesRequest import DeleteInstancesRequest
-from aliyunsdkecs.request.v20140526.StartInstanceRequest import StartInstanceRequest
-from aliyunsdkecs.request.v20140526.StopInstanceRequest import StopInstanceRequest
-from aliyunsdkecs.request.v20140526.StopInstancesRequest import StopInstancesRequest
-from aliyunsdkecs.request.v20140526.DescribeInstancesRequest import (
-    DescribeInstancesRequest,
-)
-from aliyunsdkecs.request.v20140526.TagResourcesRequest import TagResourcesRequest
 from aliyunsdkecs.request.v20140526.AllocatePublicIpAddressRequest import (
     AllocatePublicIpAddressRequest,
 )
-from aliyunsdkecs.request.v20140526.ImportKeyPairRequest import ImportKeyPairRequest
-from aliyunsdkecs.request.v20140526.DescribeKeyPairsRequest import (
-    DescribeKeyPairsRequest,
+from aliyunsdkecs.request.v20140526.AuthorizeSecurityGroupRequest import (
+    AuthorizeSecurityGroupRequest,
 )
+from aliyunsdkecs.request.v20140526.CreateInstanceRequest import CreateInstanceRequest
 from aliyunsdkecs.request.v20140526.CreateKeyPairRequest import CreateKeyPairRequest
-from aliyunsdkecs.request.v20140526.RunInstancesRequest import RunInstancesRequest
 from aliyunsdkecs.request.v20140526.CreateSecurityGroupRequest import (
     CreateSecurityGroupRequest,
 )
+from aliyunsdkecs.request.v20140526.CreateVpcRequest import CreateVpcRequest
+from aliyunsdkecs.request.v20140526.CreateVSwitchRequest import CreateVSwitchRequest
+from aliyunsdkecs.request.v20140526.DeleteInstanceRequest import DeleteInstanceRequest
+from aliyunsdkecs.request.v20140526.DeleteInstancesRequest import DeleteInstancesRequest
 from aliyunsdkecs.request.v20140526.DeleteKeyPairsRequest import DeleteKeyPairsRequest
-from aliyunsdkecs.request.v20140526.AuthorizeSecurityGroupRequest import (
-    AuthorizeSecurityGroupRequest,
+from aliyunsdkecs.request.v20140526.DescribeInstancesRequest import (
+    DescribeInstancesRequest,
+)
+from aliyunsdkecs.request.v20140526.DescribeKeyPairsRequest import (
+    DescribeKeyPairsRequest,
 )
 from aliyunsdkecs.request.v20140526.DescribeSecurityGroupsRequest import (
     DescribeSecurityGroupsRequest,
 )
-from aliyunsdkecs.request.v20140526.CreateVSwitchRequest import CreateVSwitchRequest
-from aliyunsdkecs.request.v20140526.CreateVpcRequest import CreateVpcRequest
 from aliyunsdkecs.request.v20140526.DescribeVpcsRequest import DescribeVpcsRequest
 from aliyunsdkecs.request.v20140526.DescribeVSwitchesRequest import (
     DescribeVSwitchesRequest,
 )
+from aliyunsdkecs.request.v20140526.ImportKeyPairRequest import ImportKeyPairRequest
+from aliyunsdkecs.request.v20140526.RunInstancesRequest import RunInstancesRequest
+from aliyunsdkecs.request.v20140526.StartInstanceRequest import StartInstanceRequest
+from aliyunsdkecs.request.v20140526.StopInstanceRequest import StopInstanceRequest
+from aliyunsdkecs.request.v20140526.StopInstancesRequest import StopInstancesRequest
+from aliyunsdkecs.request.v20140526.TagResourcesRequest import TagResourcesRequest
 
 
 class AcsClient: