shahar1 commented on code in PR #65621:
URL: https://github.com/apache/airflow/pull/65621#discussion_r4500760526
##########
scripts/ci/prek/check_shared_distributions_structure.py:
##########
@@ -489,40 +490,104 @@ def allowed_shared_import_predicate(node, module_name,
is_from_import):
return True
-def check_task_sdk_distribution() -> bool:
- """Check task-sdk for airflow_shared and cross-distribution _shared
imports."""
- dist_name = "task-sdk"
+def check_distribution(dist_path: Path, dist_name: str, allowed_shared_prefix:
str) -> bool:
+ """
+ Check a distribution for proper _shared imports usage.
+
+ Args:
+ dist_path: Path to the distribution directory
+ dist_name: Name of the distribution for display
+ allowed_shared_prefix: Allowed prefix for _shared imports (e.g.,
'airflow.sdk._shared')
+
+ Returns:
+ True if all checks pass, False otherwise
+ """
console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not TASK_SDK_DIR.exists():
+
+ if not dist_path.exists():
console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
return True
+
all_ok = True
- if not check_no_airflow_shared_imports(TASK_SDK_DIR, dist_name):
+
+ # Check 1: No airflow_shared imports
+ if not check_no_airflow_shared_imports(dist_path, dist_name):
all_ok = False
- if not check_only_allowed_shared_imports(TASK_SDK_DIR, dist_name,
"airflow.sdk._shared"):
+
+ # Check 2: Only allowed _shared imports
+ if not check_only_allowed_shared_imports(dist_path, dist_name,
allowed_shared_prefix):
all_ok = False
+
if all_ok:
console.print(f"[bold green]Summary: {dist_name} is OK[/bold green]")
+
return all_ok
+def check_task_sdk_distribution() -> bool:
+ """Check task-sdk distribution for proper _shared imports usage."""
+ return check_distribution(TASK_SDK_DIR, "task-sdk", "airflow.sdk._shared")
+
+
def check_airflow_core_distribution() -> bool:
- """Check airflow-core for airflow_shared imports.
+ """Check airflow-core distribution for proper _shared imports usage."""
+ return check_distribution(AIRFLOW_CORE_DIR, "airflow-core",
"airflow._shared")
- Note: ``check_only_allowed_shared_imports`` is intentionally not run here
yet —
- airflow-core currently imports a few symbols from ``airflow.sdk._shared``
(e.g.
- ``SecretsMasker``) that need to be relocated before that check can be
enabled.
- """
- dist_name = "airflow-core"
- console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not AIRFLOW_CORE_DIR.exists():
- console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
+
+def check_no_airflow_imports_devel_common(dist_path: Path) -> bool:
+ """Check that no Python files in devel-common use airflow imports."""
+ src_path = dist_path / "src"
+ if not src_path.exists():
+ console.print(" [yellow]src/ directory does not exist for
[magenta]devel-common[/magenta][/yellow]")
+ return True
+
+ def airflow_import_predicate(node, module_name, is_from_import):
+ """Check if import is from airflow package."""
+ if module_name == "airflow" or module_name.startswith("airflow."):
+ if is_from_import:
+ imported_names = ", ".join(alias.name for alias in node.names)
+ return True, f"from {module_name} import {imported_names}"
+ return True, f"import {module_name}"
+ return False, ""
+
+ py_files = list(src_path.rglob("*.py"))
+ violations = _check_imports_in_files(py_files, dist_path,
airflow_import_predicate, "devel-common")
+
+ if violations:
+ console.print(" [red]Found airflow imports in
[magenta]devel-common[/magenta]:[/red]")
+ for file_path, lineno, import_stmt in violations:
+ rel_path = file_path.relative_to(dist_path)
+ console.print(f" [red]{rel_path}:{lineno}: {import_stmt}[/red]")
+ console.print()
+ console.print(" [red]Please remove airflow imports from
[magenta]devel-common[/magenta][/red]")
+ console.print(
+ " [yellow]devel-common should not depend on airflow packages to
remain independent[/yellow]\n\n"
+ " [yellow]Those imports should be converted to `from
airflow_shared` or "
Review Comment:
**major** — Allowlist the existing `devel-common` imports instead of failing
on all of them.
> Shared utilities live in `common_prek_utils.py` — always check there
before duplicating logic.
(`scripts/ci/prek/AGENTS.md`)
> Each of the distributions should declare other needed distributions
(`AGENTS.md`, Architecture Boundaries)
The failing job lists 328 `devel-common` imports at this head. The same AST
rules find 352 in 49 files on current `main`: `tests_common/pytest_plugin.py`
has 113, `test_utils/db.py` 54, `test_utils/compat.py` 35.
The printed remedy does not cover them:
- "moved to the devel-common distribution" is circular, because the
violations are already there.
- `from airflow_shared` applies to at most 12 of the 352, the ones that
import through `_shared`. `devel-common/pyproject.toml` also declares no shared
distribution.
`pytest_plugin.py` and `compat.py` exist to drive Airflow itself, so most of
these imports will not go away. `check_sdk_imports_in_core.py` already solves
this shape with `AllowlistManager` and `find_import_violations` from
`common_prek_utils.py`: record the current per-file counts and fail only on
growth. A narrower alternative is to scan only what shared tests reach. By grep
they import `test_utils/config.py` (7 airflow imports), `otel_utils.py` (1) and
`markers.py` (0).
##########
scripts/ci/prek/check_shared_distributions_structure.py:
##########
@@ -489,40 +490,104 @@ def allowed_shared_import_predicate(node, module_name,
is_from_import):
return True
-def check_task_sdk_distribution() -> bool:
- """Check task-sdk for airflow_shared and cross-distribution _shared
imports."""
- dist_name = "task-sdk"
+def check_distribution(dist_path: Path, dist_name: str, allowed_shared_prefix:
str) -> bool:
+ """
+ Check a distribution for proper _shared imports usage.
+
+ Args:
+ dist_path: Path to the distribution directory
+ dist_name: Name of the distribution for display
+ allowed_shared_prefix: Allowed prefix for _shared imports (e.g.,
'airflow.sdk._shared')
+
+ Returns:
+ True if all checks pass, False otherwise
+ """
console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not TASK_SDK_DIR.exists():
+
+ if not dist_path.exists():
console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
return True
+
all_ok = True
- if not check_no_airflow_shared_imports(TASK_SDK_DIR, dist_name):
+
+ # Check 1: No airflow_shared imports
+ if not check_no_airflow_shared_imports(dist_path, dist_name):
all_ok = False
- if not check_only_allowed_shared_imports(TASK_SDK_DIR, dist_name,
"airflow.sdk._shared"):
+
+ # Check 2: Only allowed _shared imports
+ if not check_only_allowed_shared_imports(dist_path, dist_name,
allowed_shared_prefix):
all_ok = False
+
if all_ok:
console.print(f"[bold green]Summary: {dist_name} is OK[/bold green]")
+
return all_ok
+def check_task_sdk_distribution() -> bool:
+ """Check task-sdk distribution for proper _shared imports usage."""
+ return check_distribution(TASK_SDK_DIR, "task-sdk", "airflow.sdk._shared")
+
+
def check_airflow_core_distribution() -> bool:
- """Check airflow-core for airflow_shared imports.
+ """Check airflow-core distribution for proper _shared imports usage."""
+ return check_distribution(AIRFLOW_CORE_DIR, "airflow-core",
"airflow._shared")
Review Comment:
**major** — Enabling the core check removes an approved exception and adds
no coverage.
> Shared utilities live in `common_prek_utils.py` — always check there
before duplicating logic.
(`scripts/ci/prek/AGENTS.md`)
The docstring this PR deletes said the check was "intentionally not run here
yet" until these symbols are relocated. They have not been: the job flags 4
imports, and current `main` has 5.
`check-sdk-imports-in-core` already fails on any new `airflow.sdk` import in
core (`"airflow.sdk" in module`), so a new `airflow.sdk._shared` import is
already caught. What this adds is a second gate with no escape hatch.
`triggerer_job_runner.py:66` carries `# noqa: SDK001`, which this check does
not read, so it will fail after a rebase.
Rewriting the imports to `airflow._shared` is not a safe fix either.
`settings.py:785` configures the core and SDK maskers separately, so the SDK
one would be left unconfigured. I would keep the core half out until the masker
move lands, or honour `SDK001` through `find_import_violations`.
##########
scripts/ci/prek/check_shared_distributions_structure.py:
##########
@@ -546,6 +611,8 @@ def main() -> None:
all_ok = False
if not check_airflow_core_distribution():
all_ok = False
+ if not check_devel_common_distribution():
Review Comment:
**minor** — The hook's `files:` filter in `.pre-commit-config.yaml` has no
`devel-common` entry. `prek run --from-ref` skips this check for a
`devel-common`-only change; only CI's `--all-files` run catches it.
##########
scripts/ci/prek/check_shared_distributions_structure.py:
##########
@@ -489,40 +490,104 @@ def allowed_shared_import_predicate(node, module_name,
is_from_import):
return True
-def check_task_sdk_distribution() -> bool:
- """Check task-sdk for airflow_shared and cross-distribution _shared
imports."""
- dist_name = "task-sdk"
+def check_distribution(dist_path: Path, dist_name: str, allowed_shared_prefix:
str) -> bool:
+ """
+ Check a distribution for proper _shared imports usage.
+
+ Args:
+ dist_path: Path to the distribution directory
+ dist_name: Name of the distribution for display
+ allowed_shared_prefix: Allowed prefix for _shared imports (e.g.,
'airflow.sdk._shared')
+
+ Returns:
+ True if all checks pass, False otherwise
+ """
console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not TASK_SDK_DIR.exists():
+
+ if not dist_path.exists():
console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
return True
+
all_ok = True
- if not check_no_airflow_shared_imports(TASK_SDK_DIR, dist_name):
+
+ # Check 1: No airflow_shared imports
Review Comment:
**nit** — `# Check 1:`, `# Check 2:` and `# Check: No airflow imports` (line
584) restate the call below them.
Rule: "Do not narrate code, repeat code".
##########
scripts/ci/prek/check_shared_distributions_structure.py:
##########
@@ -489,40 +490,104 @@ def allowed_shared_import_predicate(node, module_name,
is_from_import):
return True
-def check_task_sdk_distribution() -> bool:
- """Check task-sdk for airflow_shared and cross-distribution _shared
imports."""
- dist_name = "task-sdk"
+def check_distribution(dist_path: Path, dist_name: str, allowed_shared_prefix:
str) -> bool:
+ """
+ Check a distribution for proper _shared imports usage.
+
+ Args:
+ dist_path: Path to the distribution directory
+ dist_name: Name of the distribution for display
+ allowed_shared_prefix: Allowed prefix for _shared imports (e.g.,
'airflow.sdk._shared')
+
+ Returns:
+ True if all checks pass, False otherwise
+ """
console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not TASK_SDK_DIR.exists():
+
+ if not dist_path.exists():
console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
return True
+
all_ok = True
- if not check_no_airflow_shared_imports(TASK_SDK_DIR, dist_name):
+
+ # Check 1: No airflow_shared imports
+ if not check_no_airflow_shared_imports(dist_path, dist_name):
all_ok = False
- if not check_only_allowed_shared_imports(TASK_SDK_DIR, dist_name,
"airflow.sdk._shared"):
+
+ # Check 2: Only allowed _shared imports
+ if not check_only_allowed_shared_imports(dist_path, dist_name,
allowed_shared_prefix):
all_ok = False
+
if all_ok:
console.print(f"[bold green]Summary: {dist_name} is OK[/bold green]")
+
return all_ok
+def check_task_sdk_distribution() -> bool:
+ """Check task-sdk distribution for proper _shared imports usage."""
+ return check_distribution(TASK_SDK_DIR, "task-sdk", "airflow.sdk._shared")
+
+
def check_airflow_core_distribution() -> bool:
- """Check airflow-core for airflow_shared imports.
+ """Check airflow-core distribution for proper _shared imports usage."""
+ return check_distribution(AIRFLOW_CORE_DIR, "airflow-core",
"airflow._shared")
- Note: ``check_only_allowed_shared_imports`` is intentionally not run here
yet —
- airflow-core currently imports a few symbols from ``airflow.sdk._shared``
(e.g.
- ``SecretsMasker``) that need to be relocated before that check can be
enabled.
- """
- dist_name = "airflow-core"
- console.print(f"\n[bold blue]Checking:[/bold blue]
[magenta]{dist_name}[/magenta] distribution")
- if not AIRFLOW_CORE_DIR.exists():
- console.print(f" [yellow]{dist_name} directory does not
exist[/yellow]")
+
+def check_no_airflow_imports_devel_common(dist_path: Path) -> bool:
+ """Check that no Python files in devel-common use airflow imports."""
+ src_path = dist_path / "src"
+ if not src_path.exists():
+ console.print(" [yellow]src/ directory does not exist for
[magenta]devel-common[/magenta][/yellow]")
+ return True
+
+ def airflow_import_predicate(node, module_name, is_from_import):
Review Comment:
**minor** — No tests for the new checks.
`scripts/tests/ci/prek/test_check_sdk_imports_in_core.py` is a usable model.
Rule: "Flag any changed or added behaviour without a corresponding test".
##########
airflow-core/src/airflow/cli/commands/triggerer_command.py:
##########
@@ -73,6 +73,7 @@ def triggerer_run(
@providers_configuration_loaded
def triggerer(args):
"""Start Airflow Triggerer."""
+ # TODO: Decide what to do with Triggerer
Review Comment:
**nit** — `# TODO: Decide what to do with Triggerer` names no decision and
no issue. It also does not change the hook result.
Rule: "Comment only when context is not readily apparent from the code."
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -2126,7 +2126,9 @@ def provider_dependency_bump(self) -> bool:
console_print(
"[warning]Only Release Managers should change >= conditions
for apache-airflow-providers "
"dependencies.[/]\n\nIf you want to refer to a future version
of the dependency, please add a "
- "comment [info]'# use next version'[/info] in the line of the
dependency instead.\n"
+ "comment [info]'# use next version'[/info] in the line of the
dependency instead.\n\n"
+ "If there is already the same provider before the `#
Additional devel dependencies` without "
Review Comment:
**minor** — This message is unrelated to the import checks and could be its
own PR.
Rule: "Unrelated files included: Changes to files that have nothing to do
with the stated purpose of the PR."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
