Skip to content

Commit

Permalink
apply isort uniformly for a subset of directories (ray-project#25824)
Browse files Browse the repository at this point in the history
Simplify isort filters and move it into isort cfg file.

With this change, isort will not longer apply to diffs other than to files that are in whitelisted directory (isort only supports blacklist so we implement that instead) This is much simpler than building our own whitelist logic since our formatter runs multiple codepaths depending on whether it is formatting a single file / PR / entire repo in CI.
  • Loading branch information
clarng authored Jun 17, 2022
1 parent d699351 commit 2b270fd
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 37 deletions.
6 changes: 3 additions & 3 deletions .buildkite/copy_files.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import argparse
import os
from collections import OrderedDict
import subprocess
import sys
import time
import subprocess
from collections import OrderedDict
from typing import List

from aws_requests_auth.boto_utils import BotoAWSRequestsAuth
import requests
from aws_requests_auth.boto_utils import BotoAWSRequestsAuth


def retry(f):
Expand Down
8 changes: 7 additions & 1 deletion .isort.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@ multi_line_output=3
include_trailing_comma=True
use_parentheses=True
float_to_top=True
filter_files=True

# Skip docs/* as these are carefully crafted and aren't strictly code.
# Skip python/ray/*.py to avoid touching python/ray/__init__.py, which
# is also carefully crafted.
# For the rest we will gradually remove them from the blacklist as we
# reformat the code to follow the style guide.
skip_glob=python/ray/*.py,doc/*,ci/*,python/ray/_raylet*,python/ray/_private/*,python/ray/air/*,python/ray/cloudpickle/*,python/ray/core/*,dashboard/*,python/ray/data/*,python/ray/experimental/*,python/ray/includes/*,python/ray/internal/*,python/ray/ray_operator/*,python/ray/scripts/*,python/ray/serve/*,python/ray/sgd/*,python/ray/streaming/*,python/ray/tests/*,python/ray/tests/*,python/ray/train/*,python/ray/tune/*,python/ray/util/*,python/ray/workers/*,python/ray/workflow/*,rllib/*,release/*,

skip_glob=doc/**/doc_code/*
known_local_folder=ray
known_afterray=psutil,setproctitle
sections=FUTURE,STDLIB,THIRDPARTY,FIRSTPARTY,LOCALFOLDER,AFTERRAY
23 changes: 4 additions & 19 deletions ci/lint/format.sh
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,6 @@ 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/'
'doc/'
)

ISORT_GIT_LS_EXCLUDES=(
':(exclude)doc/**/doc_code/*'
)


BLACK_EXCLUDES=(
'--force-exclude' 'python/ray/cloudpickle/*'
Expand Down Expand Up @@ -201,12 +191,6 @@ 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() {
Expand Down Expand Up @@ -264,7 +248,8 @@ format_all_scripts() {

# Run isort before black to fix imports and let black deal with file format.
echo "$(date)" "isort...."
isort_on_paths "${ISORT_PATHS[@]}"
git ls-files -- '*.py' "${GIT_LS_EXCLUDES[@]}" | xargs -P 10 \
isort
echo "$(date)" "Black...."
git ls-files -- '*.py' "${GIT_LS_EXCLUDES[@]}" | xargs -P 10 \
black "${BLACK_EXCLUDES[@]}"
Expand Down Expand Up @@ -322,8 +307,8 @@ format_changed() {
# exist on both branches.
MERGEBASE="$(git merge-base upstream/master HEAD)"

if ! git diff --diff-filter=ACRM --quiet --exit-code "$MERGEBASE" -- '*.py' "${ISORT_GIT_LS_EXCLUDES[@]}" &>/dev/null; then
git diff --name-only --diff-filter=ACRM "$MERGEBASE" -- '*.py' "${ISORT_GIT_LS_EXCLUDES[@]}" | xargs -P 5 \
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

Expand Down
6 changes: 3 additions & 3 deletions cpp/test_python_call_cpp.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import pytest

import ray
import ray.cluster_utils
from ray.exceptions import CrossLanguageError
from ray.exceptions import RayActorError
import pytest
from ray.exceptions import CrossLanguageError, RayActorError


def test_cross_language_cpp():
Expand Down
9 changes: 3 additions & 6 deletions doc/source/conf.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
# isort: skip_file

import os
import sys
from datetime import datetime

# -*- coding: utf-8 -*-
from pathlib import Path
import os
import sys

sys.path.insert(0, os.path.abspath("."))
from custom_directives import *
from datetime import datetime


# Mocking modules allows Sphinx to work without installing Ray.
Expand Down
8 changes: 3 additions & 5 deletions python/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@
import sys
import tarfile
import tempfile
import zipfile

from itertools import chain
from enum import Enum

import urllib.error
import urllib.parse
import urllib.request
import zipfile
from enum import Enum
from itertools import chain

logger = logging.getLogger(__name__)

Expand Down

0 comments on commit 2b270fd

Please sign in to comment.