Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace flake8, isort, pylint and black by ruff #2149

Merged
merged 26 commits into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c93246f
Initialize `ruff.toml` and replicate `isort`
yury-fedotov Oct 22, 2024
831b64f
Extend `ruff` config to replicate existing linter
yury-fedotov Oct 22, 2024
38a1c0d
Remove a few `pylint` directives that are no longer needed
yury-fedotov Oct 22, 2024
9de9801
Delete 3 config files that are no longer needed
yury-fedotov Oct 22, 2024
d3f4bf7
Delete 4 tools from requirements and add `ruff` instead
yury-fedotov Oct 22, 2024
51455a1
Include `ruff` linter in `Makefile`
yury-fedotov Oct 22, 2024
3523286
Add a `BLE` ruleset
yury-fedotov Oct 22, 2024
1f3dcfe
Add NOQAs for `BLE` ruleset instead of `pylint` ones
yury-fedotov Oct 22, 2024
a7a5259
Implement `N` ruleset and add settings for it
yury-fedotov Oct 22, 2024
acda98f
Eliminate individual `pylint: disable` completely
yury-fedotov Oct 23, 2024
89afdb3
Bring risk-free formats
yury-fedotov Oct 23, 2024
80a9121
Switch from `...` to `pass` in dummy function definition to satisfy `…
yury-fedotov Oct 23, 2024
5812946
Get rid of final pylint disable
yury-fedotov Oct 23, 2024
671ada4
Update CONTRIBUTING.md
yury-fedotov Oct 23, 2024
51c05a9
Remove `black` from requirements and Makefile
yury-fedotov Oct 23, 2024
13ba5a1
Delete reference to `pylint` issue that's no longer needed
yury-fedotov Oct 23, 2024
afaab24
Sort `ruff.toml`
yury-fedotov Oct 23, 2024
aad3502
Introduce hierarchical config
yury-fedotov Oct 23, 2024
582e98e
Apply `ruff` lint and format fixes to demo project
yury-fedotov Oct 23, 2024
4600f92
Remove `package` directory pointer from `make lint`
yury-fedotov Oct 23, 2024
233cf2f
Refactor all `demo_project` dev workflows to `ruff`
yury-fedotov Oct 23, 2024
337cce6
Replace a few BLE noqas by config exclude
yury-fedotov Oct 23, 2024
57a3200
Modify RELEASE.md
yury-fedotov Oct 23, 2024
267c50f
Add `kedro` to 3rd party imports of `package`
yury-fedotov Oct 23, 2024
31c693e
Merge branch 'main' into chore/switch-to-ruff
yury-fedotov Oct 25, 2024
cbff524
Merge branch 'main' into chore/switch-to-ruff
yury-fedotov Oct 25, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ make pytest
make e2e-tests
```

#### Linting tests (`isort`, `black`, `pylint`, `flake8` and `mypy`)
#### Linting tests (`ruff` linter and formatter, and `mypy`)

```bash
make lint
Expand Down
13 changes: 5 additions & 8 deletions Makefile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note how this simplifies after we define all settings, including directories and rulesets, once, in a central ruff.toml configuration file. This allows all "parties" that use ruff:

  • Developers
  • CI
  • Pre-commit hooks

To run just ruff check and ruff format from anywhere in the repo, and it applies a corresponding configuration automatically.

Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,15 @@ e2e-tests:
lint: format-fix lint-check

format-fix:
isort package/kedro_viz package/tests package/features
black package/kedro_viz package/tests package/features
ruff check --fix
ruff format

format-check:
isort --check package/kedro_viz package/tests package/features
black --check package/kedro_viz package/tests package/features
ruff check
ruff format --check

lint-check:
pylint --rcfile=package/.pylintrc -j 0 package/kedro_viz
pylint --rcfile=package/.pylintrc -j 0 --disable=protected-access,missing-docstring,redefined-outer-name,invalid-name,too-few-public-methods,no-member,unused-argument,duplicate-code,abstract-class-instantiated package/tests
pylint --rcfile=package/.pylintrc -j 0 --disable=missing-docstring,no-name-in-module,unused-argument package/features
flake8 --config=package/.flake8 package
ruff check
mypy --config-file=package/mypy.ini package/kedro_viz package/features
mypy --disable-error-code abstract --config-file=package/mypy.ini package/tests

Expand Down
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Please follow the established format:
- Fix unserializable parameters value (#2122)
- Display full dataset type with library prefix in metadata panel (#2136)
- Enable SQLite WAL mode for Azure ML to fix database locking issues (#2131)
- Replace `flake8`, `isort`, `pylint` and `black` by `ruff` (#2149)


# Release 10.0.0
Expand Down
30 changes: 5 additions & 25 deletions demo-project/.pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,11 @@ repos:
- id: check-json # Checks json files for parseable syntax.
- id: check-case-conflict # Check for files that would conflict in case-insensitive filesystems
- id: check-merge-conflict # Check for files that contain merge conflict strings.
- id: debug-statements # Check for debugger imports and py37+ `breakpoint()` calls in python source.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ruff's T100 ruleset, which I'm enabling, checks and autofixes that, so this pre-commit hook becomes redundant.

- id: requirements-txt-fixer # Sorts entries in requirements.txt
- id: flake8
args:
- "--max-line-length=100"
- "--max-complexity=18"
- "--max-complexity=18"
- "--select=B,C,E,F,W,T4,B9"
- "--ignore=E203,E266,E501,W503"

- repo: local
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.7.0
hooks:
- id: isort
name: "Sort imports"
language: system
types: [file, python]
entry: isort
- id: black
name: "Black"
language: system
types: [file, python]
entry: black
- id: kedro lint
name: "Kedro lint"
language: python_venv
types: [file, python]
entry: kedro lint
stages: [commit]
- id: ruff
args: [--fix]
- id: ruff-format
1 change: 1 addition & 0 deletions demo-project/docs/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

from kedro.framework.cli.utils import find_stylesheets
from recommonmark.transform import AutoStructify

from demo_project import __version__ as release

# -- Project information -----------------------------------------------------
Expand Down
8 changes: 0 additions & 8 deletions demo-project/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,6 @@ package_name = "demo_project"
project_name = "modular-spaceflights"
kedro_init_version = "0.19.0"

[tool.isort]
multi_line_output = 3
include_trailing_comma = true
force_grid_wrap = 0
use_parentheses = true
line_length = 88
known_third_party = "kedro"

[tool.pytest.ini_options]
addopts = """
--cov-report term-missing \
Expand Down
5 changes: 5 additions & 0 deletions demo-project/ruff.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
extend = "../ruff.toml"

[lint.isort]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a best practice to have ruff.toml ? Can we do this in demo-project/pyproject.toml -> table [tool.ruff.lint.isort] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ruff supports both pyproject.toml section and dedicated ruff.toml - see here. So yes what you propose is feasible.

There is no explicit recommendation in ruff docs on which to prefer, and I have not heard of what's generally considered best practice. My thinking in general is that whenever possible, I'd prefer a dedicated config for every tool, because it:

  1. Makes each config file have isolated responsibility
  2. Simplifies version control a bit - e.g. 2 PRs where one changes dependencies and another changes the lint rulesets wouldn't touch the same file
  3. Makes pyproject.toml smaller and more manageable

So I use ruff.toml in all of my projects, and never put that in pyproject.toml because of above.

Let me know if you disagree & still prefer the pyproject.toml - I can change to that. If so, then probably we'd need to configure ruff via pyproject.toml not only in demo-project but also in repo root and package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I came up with a better argument :)

  1. It's useful to have ruff configuration in repo root, so that both package, demo-project or other packages can inherit it
  2. Repo root is not a Python package, so ruff.toml is preferred over pyproject.toml in root
  3. (1 + 2) sort of require us to use ruff.toml in the root, so for consistency, I suggest using ruff.toml files in package and demo-project too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I am happy to have ruff.toml, I wanted to understand if this was a better practice suggested from ruff. Thanks for the PR

known-first-party = ["demo_project"]
known-third-party = ["kedro"]
3 changes: 0 additions & 3 deletions demo-project/setup.cfg

This file was deleted.

3 changes: 1 addition & 2 deletions demo-project/src/demo_project/__init__.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diffs like this, or e.g. adding a blank line between module docstring and first import line, is what ruff format does automatically due to its minor differences with black. There's no config for ruff format to make it fully backwards compatible.

Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
"""demo-project
"""
"""demo-project"""

__version__ = "0.1"
1 change: 1 addition & 0 deletions demo-project/src/demo_project/__main__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""demo-project file for ensuring the package is executable
as `demo-project` and `python -m demo_project`
"""

from pathlib import Path

from kedro.framework.project import configure_project
Expand Down
1 change: 1 addition & 0 deletions demo-project/src/demo_project/hooks.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Project hooks."""

import logging
import time
from typing import Any
Expand Down
4 changes: 3 additions & 1 deletion demo-project/src/demo_project/pipeline_registry.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Project pipelines."""

from typing import Dict

from kedro.pipeline import Pipeline
Expand All @@ -8,6 +9,7 @@
from demo_project.pipelines import modelling as mod
from demo_project.pipelines import reporting as rep


def register_pipelines() -> Dict[str, Pipeline]:
"""Register the project's pipelines.

Expand All @@ -24,7 +26,7 @@ def register_pipelines() -> Dict[str, Pipeline]:
)

reporting_pipeline = rep.create_pipeline()

return {
"__default__": (
ingestion_pipeline
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,21 @@ def create_pipeline(**kwargs) -> Pipeline:
func=apply_types_to_companies,
inputs="companies",
outputs="int_typed_companies",
name='apply_types_to_companies',
tags='companies'
name="apply_types_to_companies",
tags="companies",
),
node(
func=apply_types_to_shuttles,
inputs="shuttles",
outputs="int_typed_shuttles@pandas1",
name='apply_types_to_shuttles',
tags='shuttles'
name="apply_types_to_shuttles",
tags="shuttles",
Comment on lines -37 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another autofix from ruff format - makes sense, since now single and double quotes are mixes within the same node definition. I'm curious why previous formatter didn't flag that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The formatter was run only on package/ . The demo-project is outside of the formatter.

),
node(
func=apply_types_to_reviews,
inputs=["reviews", "params:typing.reviews.columns_as_floats"],
outputs="int_typed_reviews",
name='apply_types_to_reviews'
name="apply_types_to_reviews",
),
node(
func=aggregate_company_data,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
generated using Kedro 0.18.1
"""


from kedro.pipeline import Pipeline, node
from kedro.pipeline.modular_pipeline import pipeline

Expand Down
10 changes: 3 additions & 7 deletions demo-project/src/demo_project/pipelines/modelling/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,11 @@ def evaluate_model(
"""
y_pred = regressor.predict(X_test)
score = r2_score(y_test, y_pred)
a2_score = random.randint(0,100)*0.1
b2_score = random.randint(0,100)*0.1
a2_score = random.randint(0, 100) * 0.1
b2_score = random.randint(0, 100) * 0.1
logger = logging.getLogger(__name__)
logger.info(
f"Model has a coefficient R^2 of {score:.3f} on test data using a "
f"regressor of type '{type(regressor)}'"
)
return {
"r2_score": score,
"a2_score":a2_score,
"b2_score":b2_score
}
return {"r2_score": score, "a2_score": a2_score, "b2_score": b2_score}
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ def new_train_eval_template() -> Pipeline:
func=train_model,
inputs=["X_train", "y_train", "params:dummy_model_options"],
outputs=["regressor", "experiment_params"],
tags="train"
tags="train",
),
node(
func=evaluate_model,
inputs=["regressor", "X_test", "y_test"],
outputs="r2_score",
tags="evaluate"
tags="evaluate",
),
]
)
Expand Down Expand Up @@ -83,7 +83,7 @@ def create_pipeline(model_types: List[str]) -> Pipeline:
pipeline(
pipe=new_train_eval_template(),
parameters={"dummy_model_options": f"model_options.{model_type}"},
inputs={k: k for k in test_train_refs},
inputs={k: k for k in test_train_refs},
namespace=model_type,
)
for model_type in model_types
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ def __init__(self, _df: pd.DataFrame, x: int = 500, y: int = 200):
self._populate(_df)

def _draw_grid(self):

width, height = self.image.size
row_step = (height - self.border * 2) / (self.rows)
col_step = (width - self.border * 2) / (self.cols)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@
This is a boilerplate pipeline 'reporting'
generated using Kedro 0.18.1
"""

from typing import Dict

import matplotlib.pyplot as plt
import numpy as np
import pandas as pd
import PIL
import plotly.express as px
import seaborn as sn
from plotly import graph_objects as go
from typing import Dict

from .image_utils import DrawTable


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
from demo_project.pipelines.reporting.nodes import (
create_feature_importance_plot,
create_matplotlib_chart,
get_top_shuttles_data,
make_cancel_policy_bar_chart,
make_price_analysis_image,
make_price_histogram,
get_top_shuttles_data,
Comment on lines +11 to -14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious why isort didn't catch that in the past...

)


Expand Down
4 changes: 1 addition & 3 deletions demo-project/src/demo_project/requirements.in
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
black~=22.0
flake8>=3.7.9, <4.0
ipython~=7.0
isort~=5.0
jupyter~=1.0
jupyter_client>=5.1, <7.0
jupyterlab~=3.0
Expand All @@ -16,4 +13,5 @@ wheel>=0.35, <0.37
pillow~=9.0
matplotlib==3.5.0
pre-commit~=1.17
ruff==0.7.0
seaborn>=0.13.0
1 change: 1 addition & 0 deletions demo-project/src/tests/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

To run the tests, run ``kedro test``.
"""

from pathlib import Path

import pytest
Expand Down
7 changes: 0 additions & 7 deletions package/.flake8

This file was deleted.

9 changes: 0 additions & 9 deletions package/.isort.cfg

This file was deleted.

Loading
Loading