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

[chore] add UP rule to ruff and upgrade to Python 3.9 syntax #25703

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Nov 1, 2024

Summary & Motivation

Hello, I really like running ruff on Dagster.

This PR adds the pyupgrade rule which automatically updates Python syntax & usage to the minimal supported Python version (3.9 in our case).

The only exception is dagster-buildkite since it's still running on 3.8 (@gibsondan how hard would it be to bump Python in BuildKite?).

See pyproject.toml diff:

diff --git a/pyproject.toml b/pyproject.toml
index 7fcaa156fe..0ff8cb69a4 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -98,7 +98,7 @@ filterwarnings = [
 
 [tool.ruff]
 
-target-version = "py38"
+target-version = "py39"
 
 # *.py, *.ipy are included by default
 extend-include = ["*.ipynb"]
@@ -116,7 +116,7 @@ extend-exclude = [
 line-length = 100
 
 # Fail if Ruff is not running this version.
-required-version = "0.5.5"
+required-version = "0.7.2"
 
 [tool.ruff.lint]
 
@@ -181,6 +181,13 @@ ignore = [
   # (assorted perf rules) We have a lot of violations, enable when autofix is available
   "PERF401", # (manual-list-comprehension)
   "PERF402", # (manual-list-copy)
+
+  # (assorted pyupgrade rules) We have a lot of violations, enable when autofix is available
+  "UP035", # (deprecated-import)
+
+  # (assorted import rules) We have a lot of violations, enable when autofix is available
+  "F401", # (unused-import)
+
 ]
 
 # By default, ruff only uses all "E" (pycodestyle) and "F" (pyflakes) rules.
@@ -248,6 +255,9 @@ select = [
 
   # (invalid escape sequence) flag errant backslashes
   "W605",
+
+  # (pyupgrade) automatically upgrade Python syntax to newer versions
+  "UP",
 ]
 
 [tool.ruff.lint.flake8-builtins]

All other changes are automated.

Initial UP violations:

❯ ruff check --statistics
3561	UP006	[*] non-pep585-annotation
2163	UP035	[ ] deprecated-import
 457	UP008	[*] super-call-with-parameters
 162	UP015	[*] redundant-open-modes
  92	UP031	[*] printf-string-formatting
  39	UP027	[*] unpacked-list-comprehension
  39	UP034	[*] extraneous-parentheses
  22	UP037	[*] quoted-annotation
  15	UP028	[*] yield-in-for-loop
   7	UP024	[*] os-error-alias
   7	UP033	[*] lru-cache-with-maxsize-none
   7	UP036	[*] outdated-version-block
   4	UP009	[*] utf8-encoding-declaration
   4	UP013	[*] convert-typed-dict-functional-to-class
   2	UP010	[*] unnecessary-future-import
   1	UP012	[*] unnecessary-encode-utf8
   1	UP018	[*] native-literals
[*] fixable with `ruff check --fix`

Violations after ruff check --fix (before --unsafe-fixes):

❯ ruff check --statistics
3562	UP006 	[*] non-pep585-annotation
1352	UP035 	[ ] deprecated-import
 457	UP008 	[*] super-call-with-parameters
  92	UP031 	[*] printf-string-formatting
  15	UP028 	[*] yield-in-for-loop
   7	UP036 	[*] outdated-version-block
   4	TCH003	[*] typing-only-standard-library-import
[*] fixable with `ruff check --fix`

And before ignoring the unfixable rules:

❯ ruff check --statistics
237	UP035	deprecated-import
 36	F401 	unused-import

Copy link
Contributor Author

danielgafni commented Nov 1, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@danielgafni danielgafni marked this pull request as ready for review November 1, 2024 23:42
Copy link

github-actions bot commented Nov 2, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-5tw7rwtu0-elementl.vercel.app
https://11-02--chore-add-up-rule-to-ruff.dagster.dagster-docs.io

Direct link to changed pages:

Copy link

github-actions bot commented Nov 2, 2024

Deploy preview for dagster-docs-beta ready!

Preview available at https://dagster-docs-beta-owcusk788-elementl.vercel.app

Direct link to changed pages:

@cmpadden
Copy link
Contributor

cmpadden commented Nov 3, 2024

I think this is great, and am in favor of inclusion of pyupgrade rules.

Will wait for approval from at least one other though to ensure consensus.

@danielgafni
Copy link
Contributor Author

danielgafni commented Nov 6, 2024

Anyone wants to give another review?

Copy link
Collaborator

@smackesey smackesey left a comment

Choose a reason for hiding this comment

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

Left a comment on the pyproject.toml.

Also this needs an internal companion PR to keep these OSS/internal in sync.

Otherwise nice!

pyproject.toml Show resolved Hide resolved
@danielgafni
Copy link
Contributor Author

After switching to py39:

ruff check --statistics
3531	UP006 	[*] non-pep585-annotation
1344	UP035 	[ ] deprecated-import
   4	TCH003	[*] typing-only-standard-library-import

After running ruff with --unsafe-fixes:

❯ ruff check --statistics
237	UP035	deprecated-import
 36	F401 	unused-import

Deprecated import errors are similar to:

python_modules/libraries/dagstermill/dagstermill/manager.py:5:1: UP035 `typing.AbstractSet` is deprecated, use `collections.abc.Set` instead
  |
3 | import uuid
4 | from collections.abc import Mapping
5 | from typing import TYPE_CHECKING, AbstractSet, Any, Optional, cast
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP035
6 |
7 | from dagster import (
  |

Unused import errors look like:

python_modules/libraries/dagster-pandera/dagster_pandera/__init__.py:4:45: F401 `typing.Type` imported but unused
|
2 | import re
3 | from collections.abc import Sequence
4 | from typing import TYPE_CHECKING, Callable, Type, Union
|                                             ^^^^ F401
5 |
6 | import dagster._check as check
|
= help: Remove unused import: `typing.Type`

I've added both rules to exclude.

@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch from b7fba29 to 1cd51c5 Compare November 7, 2024 14:51
@danielgafni danielgafni changed the title [chore] add UP rule to ruff [chore] add UP rule to ruff and upgrade to Python 3.9 syntax Nov 7, 2024
@schrockn
Copy link
Member

schrockn commented Nov 8, 2024

  • Can you document (or point to a document) the describes all the autofixes variants being applied here? This diff to is too large to review?

With the above, we could also then announce this to end-headsup so people know what the have to "untrain" themselves from writing.

@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch from 1cd51c5 to aa08de4 Compare November 11, 2024 15:07
@danielgafni
Copy link
Contributor Author

Hey @schrockn, I added violation stats to the PR description.

For sure it's impossible to review, but the number of --unsafe-fixes is pretty low (only 2 rules - check my comment above) and since CI was happy it's probably safe to apply them.

@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch from aa08de4 to 22d263f Compare November 11, 2024 15:15
@danielgafni
Copy link
Contributor Author

danielgafni commented Nov 11, 2024

Actually, there are some failures in BuildKite because some parts of it are still running on Python 3.8 3.7.
I hope there is a single image/entrypoint which we can bump? @gibsondan @smackesey

@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch 2 times, most recently from 871b1b1 to f5b563d Compare November 19, 2024 14:29
@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch 2 times, most recently from 19ef526 to 11dabed Compare November 22, 2024 14:37
@danielgafni
Copy link
Contributor Author

danielgafni commented Nov 22, 2024

@schrockn @cmpadden @smackesey

Hey guys, major tests seem to be passing.

I would like to get an approval here because it's very hard to keep this PR up to date (new conflicts appear all the time).

I'll rebase it again to make sure dagster-blueprints is fine now.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

What I want to see before we land this is the text of an announcement of this to eng-team that explains all changes here.

e.g.

* Use native types for list, dict, set in typing declarations. E.g. list[Foo] over List[Foo]. This reduces the number of imports for common use cases.

Then show a single example. Do this for all the rules.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Requesting changes so that you can produce a team-facing document detailing all the changes.

@danielgafni danielgafni force-pushed the 11-02-_chore_add_up_rule_to_ruff branch from 11dabed to e8fb1ce Compare November 22, 2024 15:51
@danielgafni
Copy link
Contributor Author

got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants