Skip to content

Commit

Permalink
[Core] Add pre-commit configuration (ray-project#41500)
Browse files Browse the repository at this point in the history
This PR adds a pre-commit configuration to Ray. This has a number of advantages over the current pre-commit hooks over format.sh:

More developers are familiar with pre-commit from other projects, decreasing maintenance burden. No special knowledge of Ray dev work flow required to maintain this.
Isolated environments for these pre-commit hooks mean that there is not need to check what version of a tool the developer has (or doesn't have) on their machine; pre-commit just gets the right version and uses it in an isolated environment without any checks, warnings, or version conflicts; e.g.
image

If we fully migrate to pre-commit, we won't need lint-requirements.txt because the lint requirements will be fully specified in .pre-commit-config.yml. This should simplify our CI system and our build constraints.
Users wouldn't have to download the google-java-formatter as much, as it will be cached by the hook rather than placed in /tmp/
It's easier to upgrade hooks with pre-commit autoupdate, or by updating the yaml config manually
It's easier to add new hooks: just edit the yaml config to make a new hook, instead of learning how the current lint system works with ci/lint/format.sh. This would be useful because there's a lot of lint checks that are run when a PR is made but not locally - see the ci/lint/ directory for examples of this.
Note that this config is not enforced, but in the future we may remove the current lint system in favor of pre-commit.

I'm open to turning on more hooks, or turning some off if people prefer; note also that this PR doesn't include changes suggested by this config. For the most part, the changes that get applied when you run pre-commit with this config due to trim-trailing-whitespace and end-of-file-fixer. There are a few other issues which are legitimate concerns that I'll make issues for later.

Changes
Added a .pre-commit-config.yaml that matches format.sh as closely as possible
Broke check_docstyle out into its own script, which is run as a local hook
Added a .prettierrc.toml; currently prettier only runs on files in doc/

Signed-off-by: pdmurray <[email protected]>
  • Loading branch information
peytondmurray authored Dec 15, 2023
1 parent d3f2951 commit 706e8bf
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 1 deletion.
144 changes: 144 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
exclude: |
(?x)^(
python/ray/core/generated/|
python/ray/serve/generated/|
python/ray/cloudpickle/|
python/ray/_private/runtime_env/_clonevirtualenv.py|
doc/external/
)
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-added-large-files
- id: check-ast
exclude: |
(?x)(
python/ray/serve/tests/test_config_files/syntax_error\.py
)$
- id: check-json
exclude: |
(?x)^(
# Intentionally bad json schema
python/ray/tests/test_runtime_env_validation_bad_2_schema.json|
# json5 comments prevent parsing
python/asv.conf.json|
rllib/asv.conf.json
)
- id: check-toml

- repo: https://github.com/psf/black
rev: 22.10.0
hooks:
- id: black
exclude: |
(?x)^(
python/ray/cloudpickle/|
python/build/|
python/ray/core/src/ray/gcs/|
python/ray/thirdparty_files/|
python/ray/_private/thirdparty/|
python/ray/serve/tests/test_config_files/syntax_error\.py|
doc/external/
)
types_or: [python]

- repo: https://github.com/pycqa/flake8
rev: 3.9.1
hooks:
- id: flake8
additional_dependencies:
[
flake8-comprehensions==3.10.1,
flake8-quotes==2.0.0,
flake8-bugbear==21.9.2,
]

- repo: https://github.com/pre-commit/mirrors-prettier
rev: v3.0.3
hooks:
- id: prettier
files: 'doc/'
types_or: [javascript, ts, tsx, html]

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.7.0
hooks:
- id: mypy
args: ['--follow-imports=skip', '--ignore-missing-imports']
files: |
(?x)^(
python/ray/autoscaler/node_provider.py|
python/ray/autoscaler/sdk/__init__.py|
python/ray/autoscaler/sdk/sdk.py|
python/ray/autoscaler/_private/commands.py|
python/ray/autoscaler/_private/autoscaler.py|
python/ray/_private/gcs_utils.py
)
additional_dependencies:
[
types-PyYAML
]

- repo: https://github.com/pycqa/isort
rev: 5.12.0
hooks:
- id: isort
name: isort (python)
types_or: [python]

- repo: https://github.com/pre-commit/pygrep-hooks
rev: v1.10.0
hooks:
- id: rst-directive-colons
- id: rst-inline-touching-normal
- id: python-no-log-warn
- id: python-check-mock-methods

- repo: https://github.com/koalaman/shellcheck-precommit
rev: v0.9.0
hooks:
- id: shellcheck
args: ['--exclude=1090,1091,2207']
# 1090: Can't follow non-constant source. Use a directive to specify location.
# 1091: Not following {file} due to some error
# 2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting). -- these aren't compatible with macOS's old Bash

- repo: https://github.com/pocc/pre-commit-hooks
rev: v1.3.5
hooks:
- id: clang-format
args: [--version=12.0.1]

- repo: https://github.com/macisamuele/language-formatters-pre-commit-hooks
rev: v2.11.0
hooks:
- id: pretty-format-java
args: [--autofix, --google-java-formatter-version=1.7]
exclude: |
(?x)^(
java/api/src/main/java/io/ray/api/ActorCall.java|
java/api/src/main/java/io/ray/api/CppActorCall.java|
java/api/src/main/java/io/ray/api/PyActorCall.java|
java/api/src/main/java/io/ray/api/RayCall.java
)
- repo: local
hooks:
- id: docstyle
name: Check for Ray docstyle violations
entry: ci/lint/check-docstyle.sh
language: system
types: [python]

- repo: local
hooks:
- id: check-import-order
name: Check for Ray import order violations
entry: python ci/lint/check_import_order.py
language: python
types: [python]
pass_filenames: false
args: [".", "-s", "ci", "-s", "python/ray/thirdparty_files", "-s", "python/build", "-s", "lib"]
2 changes: 2 additions & 0 deletions .prettierrc.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
singleQuote = true
bracketSpacing = false
18 changes: 18 additions & 0 deletions ci/lint/check-docstyle.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/bin/bash

check_docstyle() {
echo "Checking docstyle..."
violations=$(echo "$@" | xargs grep -E '^[ ]+[a-z_]+ ?\([a-zA-Z]+\): ' | grep -v 'str(' | grep -v noqa || true)
if [[ -n "$violations" ]]; then
echo
echo "=== Found Ray docstyle violations ==="
echo "$violations"
echo
echo "Per the Google pydoc style, omit types from pydoc args as they are redundant: https://docs.ray.io/en/latest/ray-contribute/getting-involved.html#code-style "
echo "If this is a false positive, you can add a '# noqa' comment to the line to ignore."
exit 1
fi
return 0
}

check_docstyle "$@"
28 changes: 27 additions & 1 deletion doc/source/ray-contribute/development.rst
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ You can propose changes to the main project by submitting a pull request to the

1. Navigate to the `Ray GitHub repository <https://github.com/ray-project/ray>`_.
2. Follow these `GitHub instructions <https://docs.github.com/en/get-started/quickstart/fork-a-repo>`_, and do the following:

a. `Fork the repo <https://docs.github.com/en/get-started/quickstart/fork-a-repo#forking-a-repository>`_ using your preferred method.
b. `Clone <https://docs.github.com/en/get-started/quickstart/fork-a-repo#cloning-your-forked-repository>`_ to your local machine.
c. `Connect your repo <https://docs.github.com/en/get-started/quickstart/fork-a-repo#configuring-git-to-sync-your-fork-with-the-upstream-repository>`_ to the upstream (main project) Ray repo to sync changes.
Expand Down Expand Up @@ -328,6 +328,32 @@ Dependencies for running Ray unit tests under ``python/ray/tests`` can be instal
Requirement files for running Ray Data / ML library tests are under ``python/requirements/``.

Pre-commit Hooks
----------------

Ray is planning to replace the pre-push hooks that are invoked from ``scripts/format.sh`` with
pre-commit hooks using `the pre-commit python package <https://pre-commit.com/>`_ in the future. At
the moment, we have configured a ``.pre-commit-config.yaml`` which runs all the same checks done by
``scripts/format.sh`` along with a few additional ones too. Currently this developer tooling is
opt-in, with any formatting changes made by ``scripts/format.sh`` expected to be caught by
``pre-commit`` as well. To start using ``pre-commit``:

.. code-block: shell
pip install pre-commit
pre-commit install
This will install pre-commit into the current environment, and enable pre-commit checks every time
you commit new code changes with git. To temporarily skip pre-commit checks, use the ``-n`` or
``--no-verify`` flag when committing:

.. code-block: shell
git commit -n
If you find that ``scripts/format.sh`` makes a change that is different from what ``pre-commit``
does, please report an issue on the Ray github page.

Fast, Debug, and Optimized Builds
---------------------------------

Expand Down

0 comments on commit 706e8bf

Please sign in to comment.