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

yury-fedotov · 2024-10-23T04:35:18Z

Description

Development notes

This PR brings ruff to kedro-viz to replace:

flake8
isort
pylint
black

It introduces a central ruff.toml file in repo root, to act as a single source of truth for Python codestyle management, and leverages ruff's hierarchical configuration feature to make this config applicable to both the demo-project and package parts of the codebase.

Note

Another enhancement is that linter/formatter configuration, such as:

Directories in scope
Active and ignored rulesets

Is now defined in a single file, once, as opposed to being duplicated in Makefile, pre-commit config etc., which should streamline further code quality management by simplifying addition of rulesets, or directories in scope.

Note for reviewers

While the diff from this PR is quite big (72 files), most of that is very simple auto-fixes from ruff format that are slightly different from black. So reviewing should not be very hard.

Testing plan

make lint passes locally
make pytest passes locally
Waiting for maintainers to release merge-gatekeeper to run full CI
CI to pass
Maintainers to test locally

Other tasks

Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file

Signed-off-by: Yury Fedotov <[email protected]>

…ruff` Signed-off-by: Yury Fedotov <[email protected]>

Signed-off-by: Yury Fedotov <[email protected]>

yury-fedotov · 2024-10-23T04:45:46Z

package/tests/conftest.py

-        ...
+        pass


Otherwise, the ruff formatter reformats that to one liner:

def process_data(raw_data, train_test_split): ...

And test_task_node_metadata in package/tests/test_api/test_rest/test_responses.py fails. See my modification to that test, to make it compatible with pass, below.

yury-fedotov · 2024-10-23T04:50:49Z

Makefile

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.

yury-fedotov · 2024-10-23T04:52:02Z

demo-project/.pre-commit-config.yaml

@@ -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.


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

yury-fedotov · 2024-10-23T04:54:25Z

ruff.toml

+[lint.mccabe]
+max-complexity = 18
+
+[lint.pylint]
+max-args = 12


I checked all arguments that your pylint, isort and flake8 configs used, and they are all default except for those

yury-fedotov · 2024-10-23T04:56:26Z

demo-project/src/demo_project/__init__.py

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.

yury-fedotov · 2024-10-23T04:57:41Z

demo-project/src/demo_project/pipelines/data_ingestion/pipeline.py

-                name='apply_types_to_shuttles',
-                tags='shuttles'
+                name="apply_types_to_shuttles",
+                tags="shuttles",


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.

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

yury-fedotov · 2024-10-23T04:58:20Z

demo-project/src/demo_project/pipelines/reporting/pipeline.py

+    get_top_shuttles_data,
    make_cancel_policy_bar_chart,
    make_price_analysis_image,
    make_price_histogram,
-    get_top_shuttles_data,


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

Signed-off-by: Yury Fedotov <[email protected]>

yury-fedotov · 2024-10-23T05:00:04Z

package/kedro_viz/api/apps.py

@@ -42,7 +43,7 @@ def _create_base_api_app() -> FastAPI:
        @app.middleware("http")
        async def set_secure_headers(request, call_next):
            response = await call_next(request)
-            secure_headers.framework.fastapi(response)  # pylint: disable=no-member


I have deleted all pylint: disable... directives from the entire codebase.

Signed-off-by: Yury Fedotov <[email protected]>

astrojuanlu · 2024-10-23T09:35:59Z

🔥 Thank you @yury-fedotov for this PR!

rashidakanchwala · 2024-10-23T10:25:42Z

package/ruff.toml

+extend = "../ruff.toml"
+
+[lint.isort]
+known-first-party = ["kedro_viz"]


should we also add kedro here as third party?

Yes - added.

Signed-off-by: Yury Fedotov <[email protected]>

ravi-kumar-pilla · 2024-10-23T13:25:58Z

demo-project/ruff.toml

@@ -0,0 +1,5 @@
+extend = "../ruff.toml"
+
+[lint.isort]


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

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:

Makes each config file have isolated responsibility

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

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.

I came up with a better argument :)

It's useful to have ruff configuration in repo root, so that both package, demo-project or other packages can inherit it

Repo root is not a Python package, so ruff.toml is preferred over pyproject.toml in root

(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.

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

rashidakanchwala

LGTM. thanks a lott @yury-fedotov

ravi-kumar-pilla

Thank you @yury-fedotov 💯

ankatiyar · 2024-10-24T10:37:33Z

ruff.toml

+[lint.per-file-ignores]
+"*/cli_steps.py" = ["B011"] # assert False instead of AssertionError
+"*/base_deployer.py" = ["B024"] # ABCs without abstract methods
+"package/kedro_viz/__init__.py" = ["B028"] # Risky usage of positional arguments


Q: does it make sense to ignore some of these specific issues in the code with noqa inline instead of here? Can keep the ignores that apply to multiple files here.

I definitely wouldn't want to ignore the 'broad exception' rule in general, because I think we should avoid it as much as possible. I guess some others like # noqa: ARG002 should be fine.

@rashidakanchwala

Broad exception rule has BLE code in ruff and is selected for entire repo ✅ (See line 14 of this file)

@ankatiyar

Good question!

The way I implemented it is as follows:

Whatever has an inline NOQA before that's applied to 1-2 places, stays as inline NOQA.

Whatever was a file-level pylint: disable statement at the beginning of the file is moved to ruff.toml from that separate file.

Let me explain the rationale for (2).

Consider two implementations, using package/kedro_viz/integrations/kedro/sqlite_store.py as an example:

Before

# pylint: broad-exception-caught

At the beginning of that particular file.

After

"package/kedro_viz/integrations/kedro/sqlite_store.py" = ["BLE"]

In ruff.toml

In both cases, we essentially store the same directive - disable a particular rule in a particular file.

However (2) makes it more explicit and centralizes all those directives, which would otherwise be scattered across tens of files, in a single file.

Long term, that helps gradually make the codebase adhere to those rulesets, and delete those per-file-ignores. As opposed to distributed inline NOQAs that typically just never addressed.

ankatiyar

Overall looks good but left a question regarding the ignoring of errors.
Thanks @yury-fedotov! 💯

rashidakanchwala · 2024-10-25T09:24:47Z

@yury-fedotov , feel free to merge this PR :)

yury-fedotov · 2024-10-25T13:49:30Z

@yury-fedotov , feel free to merge this PR :)

I don't have write access @rashidakanchwala

rashidakanchwala · 2024-10-25T13:59:01Z

no worries, let me merge. Once again Thank you!, this is big improvement to our dev workflow!

yury-fedotov added 21 commits October 22, 2024 16:36

Initialize ruff.toml and replicate isort

c93246f

Signed-off-by: Yury Fedotov <[email protected]>

Extend ruff config to replicate existing linter

831b64f

Signed-off-by: Yury Fedotov <[email protected]>

Remove a few pylint directives that are no longer needed

38a1c0d

Signed-off-by: Yury Fedotov <[email protected]>

Delete 3 config files that are no longer needed

9de9801

Signed-off-by: Yury Fedotov <[email protected]>

Delete 4 tools from requirements and add ruff instead

d3f4bf7

Signed-off-by: Yury Fedotov <[email protected]>

Include ruff linter in Makefile

51455a1

Signed-off-by: Yury Fedotov <[email protected]>

Add a BLE ruleset

3523286

Signed-off-by: Yury Fedotov <[email protected]>

Add NOQAs for BLE ruleset instead of pylint ones

1f3dcfe

Signed-off-by: Yury Fedotov <[email protected]>

Implement N ruleset and add settings for it

a7a5259

Signed-off-by: Yury Fedotov <[email protected]>

Eliminate individual pylint: disable completely

acda98f

Signed-off-by: Yury Fedotov <[email protected]>

Bring risk-free formats

89afdb3

Signed-off-by: Yury Fedotov <[email protected]>

Switch from ... to pass in dummy function definition to satisfy `…

80a9121

…ruff` Signed-off-by: Yury Fedotov <[email protected]>

Get rid of final pylint disable

5812946

Signed-off-by: Yury Fedotov <[email protected]>

Update CONTRIBUTING.md

671ada4

Signed-off-by: Yury Fedotov <[email protected]>

Remove black from requirements and Makefile

51c05a9

Signed-off-by: Yury Fedotov <[email protected]>

Delete reference to pylint issue that's no longer needed

13ba5a1

Signed-off-by: Yury Fedotov <[email protected]>

Sort ruff.toml

afaab24

Signed-off-by: Yury Fedotov <[email protected]>

Introduce hierarchical config

aad3502

Signed-off-by: Yury Fedotov <[email protected]>

Apply ruff lint and format fixes to demo project

582e98e

Signed-off-by: Yury Fedotov <[email protected]>

Remove package directory pointer from make lint

4600f92

Signed-off-by: Yury Fedotov <[email protected]>

Refactor all demo_project dev workflows to ruff

233cf2f

Signed-off-by: Yury Fedotov <[email protected]>

yury-fedotov changed the title ~~Chore/switch to ruff~~ [WIP] Replace flake8, isort, pylint and black by ruff Oct 23, 2024

yury-fedotov commented Oct 23, 2024

View reviewed changes

Replace a few BLE noqas by config exclude

337cce6

Signed-off-by: Yury Fedotov <[email protected]>

yury-fedotov commented Oct 23, 2024

View reviewed changes

Modify RELEASE.md

57a3200

Signed-off-by: Yury Fedotov <[email protected]>

yury-fedotov marked this pull request as ready for review October 23, 2024 05:15

yury-fedotov requested review from rashidakanchwala, astrojuanlu and ravi-kumar-pilla as code owners October 23, 2024 05:15

yury-fedotov changed the title ~~[WIP] Replace flake8, isort, pylint and black by ruff~~ Replace flake8, isort, pylint and black by ruff Oct 23, 2024

ankatiyar self-requested a review October 23, 2024 10:15

rashidakanchwala reviewed Oct 23, 2024

View reviewed changes

Add kedro to 3rd party imports of package

267c50f

Signed-off-by: Yury Fedotov <[email protected]>

rashidakanchwala self-requested a review October 23, 2024 12:58

ravi-kumar-pilla reviewed Oct 23, 2024

View reviewed changes

rashidakanchwala approved these changes Oct 23, 2024

View reviewed changes

ravi-kumar-pilla self-requested a review October 23, 2024 17:15

ravi-kumar-pilla approved these changes Oct 23, 2024

View reviewed changes

ankatiyar reviewed Oct 24, 2024

View reviewed changes

ankatiyar approved these changes Oct 24, 2024

View reviewed changes

yury-fedotov added 2 commits October 25, 2024 09:49

Merge branch 'main' into chore/switch-to-ruff

31c693e

Merge branch 'main' into chore/switch-to-ruff

cbff524

rashidakanchwala merged commit 02ce770 into kedro-org:main Oct 28, 2024
23 checks passed

jitu5 mentioned this pull request Nov 20, 2024

Release v10.1.0 #2199

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yury-fedotov commented Oct 23, 2024 •

edited

Loading

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

ravi-kumar-pilla Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

astrojuanlu commented Oct 23, 2024

rashidakanchwala Oct 23, 2024

yury-fedotov Oct 23, 2024

ravi-kumar-pilla Oct 23, 2024

yury-fedotov Oct 23, 2024

yury-fedotov Oct 23, 2024

ravi-kumar-pilla Oct 23, 2024

rashidakanchwala left a comment

ravi-kumar-pilla left a comment

ankatiyar Oct 24, 2024

rashidakanchwala Oct 24, 2024

yury-fedotov Oct 24, 2024

yury-fedotov Oct 24, 2024 •

edited

Loading

ankatiyar left a comment

rashidakanchwala commented Oct 25, 2024

yury-fedotov commented Oct 25, 2024 •

edited

Loading

rashidakanchwala commented Oct 25, 2024

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

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

Conversation

yury-fedotov commented Oct 23, 2024 • edited Loading

Description

Development notes

Note for reviewers

Testing plan

Other tasks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrojuanlu commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashidakanchwala left a comment

Choose a reason for hiding this comment

ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yury-fedotov Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

ankatiyar left a comment

Choose a reason for hiding this comment

rashidakanchwala commented Oct 25, 2024

yury-fedotov commented Oct 25, 2024 • edited Loading

rashidakanchwala commented Oct 25, 2024

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

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

yury-fedotov commented Oct 23, 2024 •

edited

Loading

yury-fedotov Oct 24, 2024 •

edited

Loading

yury-fedotov commented Oct 25, 2024 •

edited

Loading