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] ruff: ban relative imports #23816

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Aug 22, 2024

Summary & Motivation

Internal discussion: https://github.com/dagster-io/internal/discussions/11157

Relative imports are discouraged in Python.

  1. From ruff docs:

Why is this bad?
Absolute imports, or relative imports from siblings, are recommended by PEP 8:
Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) if the import system is incorrectly configured (such as when a directory inside a package ends up on sys.path)

2. I'm trying to migrate to uv for packaging in #23814. Currently, uv only supports src-based project layout (astral-sh/uv#5328), and some of the current relative imports break after switching to it (code smell!). Using absolute imports should fix this issue.

Edit: this turned out to be wrong


Steps performed to produce this PR:

  1. Ban relative imports in the whole project. Ignore this only in docs/ and examples/.
diff --git a/pyproject.toml b/pyproject.toml
index 92761add36..8ae75a70c6 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -211,6 +211,9 @@ select = [
   # (isort) detect improperly sorted imports
   "I001",
 
+  # ban relative imports
+  "TID252",
+
   # (performance) perflint rules
   "PERF",
 
@@ -269,6 +272,10 @@ split-on-trailing-comma = false
 # per line. Useful for __init__.py files that just re-export symbols.
 force-wrap-aliases = true
 
+[tool.ruff.lint.flake8-tidy-imports]
+# Disallow all relative imports.
+ban-relative-imports = "all"
+
 [tool.ruff.lint.per-file-ignores]
 
 # Don't format docstrings in alembic migrations.
diff --git a/docs/pyproject.toml b/docs/pyproject.toml
new file mode 100644
index 0000000000..2144bf49e8
--- /dev/null
+++ b/docs/pyproject.toml
@@ -0,0 +1,8 @@
+[tool.ruff]
+extend = "../pyproject.toml"
+
+[tool.ruff.lint]
+extend-ignore = [
+    # (relative imports): relative imports are acceptable in examples & docs
+    "TID252"
+]
diff --git a/examples/pyproject.toml b/examples/pyproject.toml
index 473c4b9319..4966ec6706 100644
--- a/examples/pyproject.toml
+++ b/examples/pyproject.toml
@@ -4,3 +4,8 @@
 # has the effect of making each example package treated as first-party, where `dagster` and
 # integrations will be treated as third-party. This is appropriate for example code.
 extend = "../pyproject.toml"
+
+[tool.ruff.lint]
+extend-ignore = [
+    "TID252"
+]
  1. run ruff check --fix --unsafe-fixes
    This produces most of the automatic changes in this PR.

  2. fix a single pytest error

  3. fix a few pyright errors

How I Tested These Changes

Existing tests should pass. The only test which required tweaking was testing line numbers lol

diff --git a/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py b/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py
index 2f9dae2684..b643b5ba93 100644
--- a/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py
+++ b/python_modules/dagster/dagster_tests/asset_defs_tests/test_asset_defs_source_metadata.py
@@ -26,7 +26,7 @@
 EXPECTED_ORIGINS = {
     "james_brown": DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/__init__.py:12",
     "chuck_berry": (
-        DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/module_with_assets.py:16"
+        DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/module_with_assets.py:18"
     ),
     "little_richard": (DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/__init__.py:4"),
     "fats_domino": DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/__init__.py:16",
@@ -36,15 +36,14 @@
         + "asset_package/asset_subpackage/another_module_with_assets.py:6"
     ),
     "graph_backed_asset": (
-        DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/module_with_assets.py:39"
+        DAGSTER_PACKAGE_PATH + PATH_IN_PACKAGE + "asset_package/module_with_assets.py:41"
     ),
 }
 
 
 def test_asset_code_origins() -> None:
     from dagster_tests.asset_defs_tests import asset_package
-
-    from .asset_package import module_with_assets
+    from dagster_tests.asset_defs_tests.asset_package import module_with_assets
 
     collection = load_assets_from_modules([asset_package, module_with_assets])
 
@@ -107,8 +106,7 @@ def test_asset_code_origins() -> None:
 
 def test_asset_code_origins_source_control() -> None:
     from dagster_tests.asset_defs_tests import asset_package
-
-    from .asset_package import module_with_assets
+    from dagster_tests.asset_defs_tests.asset_package import module_with_assets
 
     collection = load_assets_from_modules([asset_package, module_with_assets])
 
@@ -164,8 +162,7 @@ def test_asset_code_origins_source_control_custom_mapping() -> None:
     # test custom source_control_file_path_mapping fn
 
     from dagster_tests.asset_defs_tests import asset_package
-
-    from .asset_package import module_with_assets
+    from dagster_tests.asset_defs_tests.asset_package import module_with_assets
 
     collection = load_assets_from_modules([asset_package, module_with_assets])
 
@@ -226,4 +223,4 @@ def convert_to_source_control_path(self, local_path: Path) -> str:
                     asset.metadata_by_key[key]["dagster/code_references"].code_references[-1],
                 )
 
-                assert meta.url == expected_url
+                assert meta.url == expected_url, f"op {op_name} key {key} code reference is wrong"

Changelog [New | Bug | Docs]

NOCHANGELOG

@danielgafni danielgafni marked this pull request as draft August 22, 2024 08:47
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Aug 22, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 August 22, 2024 08:47
Copy link

github-actions bot commented Aug 22, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-kfxuusiik-elementl.vercel.app
https://ruff-ban-relative-imports.dagster.dagster-docs.io

Direct link to changed pages:

@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch from 3a16402 to 5b7c942 Compare August 22, 2024 09:02
Copy link
Contributor Author

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

Join @danielgafni and the rest of your teammates on Graphite Graphite

@danielgafni danielgafni removed the request for review from erinkcochran87 August 22, 2024 09:04
@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch from 5b7c942 to 5e5adcc Compare August 22, 2024 09:10
@danielgafni danielgafni changed the title [chore] ruff: ban relative imports [chore] ruff: full isort rules & ban relative imports Aug 22, 2024
@danielgafni danielgafni changed the title [chore] ruff: full isort rules & ban relative imports [chore] ruff: ban relative imports Aug 22, 2024
@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch 2 times, most recently from 820e45a to 392f182 Compare August 22, 2024 11:40
@danielgafni
Copy link
Contributor Author

Nevermind, it looks like the src layout is not required by uv. So we can continue living with relative imports in our codebase. I can finish this work if we decide to get rid of them for other reasons.

@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch from 392f182 to 99d56d1 Compare August 26, 2024 11:01
@danielgafni
Copy link
Contributor Author

How many pairs of eyes do we want for a diff as huge as this one?

@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch 3 times, most recently from d8e9806 to 98137a5 Compare August 26, 2024 14:56
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.

Ok let's do this. I can announce in eng heads up.

You will also want to do this in internal as well.

@danielgafni danielgafni marked this pull request as ready for review August 26, 2024 15:55
@danielgafni danielgafni force-pushed the ruff-ban-relative-imports branch from 98137a5 to 744d89d Compare August 27, 2024 07:19
@danielgafni
Copy link
Contributor Author

danielgafni commented Aug 27, 2024

All CI failures are also happening in master, so merging anyway.

@danielgafni danielgafni merged commit 053612f into master Aug 27, 2024
1 of 2 checks passed
@danielgafni danielgafni deleted the ruff-ban-relative-imports branch August 27, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants