Skip to content

Commit 0772378

Browse files
amoghrajeshvatsrahul1001
authored andcommitted
[v3-3-test] Convert check-sdk-imports to capture occurences than files (#71212)
(cherry picked from commit d8cfc95) Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
1 parent a4b4c61 commit 0772378

4 files changed

Lines changed: 470 additions & 119 deletions

File tree

‎airflow-core/.pre-commit-config.yaml‎

Lines changed: 1 addition & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -308,113 +308,12 @@ repos:
308308
^tests/unit/core/test_configuration\.py$|
309309
^tests/unit/models/test_renderedtifields\.py$|
310310
^tests/unit/models/test_variable\.py$
311-
- id: check-sdk-imports
311+
- id: check-sdk-imports-in-core
312312
name: Check for SDK imports in core files
313313
entry: ../scripts/ci/prek/check_sdk_imports_in_core.py
314314
language: python
315315
types: [python]
316316
files: ^src/airflow/
317-
exclude: |
318-
(?x)
319-
# Allow SDK imports in these legitimate locations
320-
^src/airflow/example_dags/.*\.py$|
321-
322-
# TODO: These files need to be refactored to remove SDK coupling
323-
^src/airflow/__init__\.py$|
324-
^src/airflow/api/common/mark_tasks\.py$|
325-
^src/airflow/api_fastapi/core_api/datamodels/assets\.py$|
326-
^src/airflow/api_fastapi/core_api/datamodels/connections\.py$|
327-
^src/airflow/api_fastapi/core_api/datamodels/hitl\.py$|
328-
^src/airflow/api_fastapi/core_api/datamodels/variables\.py$|
329-
^src/airflow/api_fastapi/core_api/routes/ui/grid\.py$|
330-
^src/airflow/api_fastapi/core_api/routes/ui/structure\.py$|
331-
^src/airflow/api_fastapi/core_api/services/public/connections\.py$|
332-
^src/airflow/api_fastapi/core_api/services/ui/connections\.py$|
333-
^src/airflow/api_fastapi/core_api/services/ui/grid\.py$|
334-
^src/airflow/api_fastapi/core_api/services/ui/task_group.py$|
335-
^src/airflow/api_fastapi/execution_api/routes/hitl\.py$|
336-
^src/airflow/api_fastapi/execution_api/routes/task_instances\.py$|
337-
^src/airflow/api_fastapi/execution_api/versions/v2026_04_06\.py$|
338-
^src/airflow/api_fastapi/logging/decorators\.py$|
339-
^src/airflow/assets/evaluation\.py$|
340-
^src/airflow/assets/manager\.py$|
341-
^src/airflow/cli/commands/connection_command\.py$|
342-
^src/airflow/cli/commands/task_command\.py$|
343-
^src/airflow/cli/commands/triggerer_command.py$|
344-
^src/airflow/configuration\.py$|
345-
^src/airflow/dag_processing/collection\.py$|
346-
^src/airflow/dag_processing/dagbag\.py$|
347-
^src/airflow/dag_processing/importers/.*\.py$|
348-
^src/airflow/dag_processing/manager\.py$|
349-
^src/airflow/dag_processing/processor\.py$|
350-
^src/airflow/datasets/metadata\.py$|
351-
^src/airflow/exceptions\.py$|
352-
^src/airflow/executors/base_executor\.py$|
353-
^src/airflow/jobs/triggerer_job_runner\.py$|
354-
^src/airflow/lineage/hook\.py$|
355-
^src/airflow/listeners/spec/asset\.py$|
356-
^src/airflow/listeners/spec/taskinstance\.py$|
357-
^src/airflow/logging/remote\.py$|
358-
^src/airflow/models/__init__\.py$|
359-
^src/airflow/models/asset\.py$|
360-
^src/airflow/models/baseoperator\.py$|
361-
^src/airflow/models/callback\.py$|
362-
^src/airflow/models/connection\.py$|
363-
^src/airflow/models/dag\.py$|
364-
^src/airflow/models/dagrun\.py$|
365-
^src/airflow/models/deadline\.py$|
366-
^src/airflow/models/expandinput\.py$|
367-
^src/airflow/models/mappedoperator\.py$|
368-
^src/airflow/models/operator\.py$|
369-
^src/airflow/models/param\.py$|
370-
^src/airflow/models/renderedtifields\.py$|
371-
^src/airflow/models/serialized_dag\.py$|
372-
^src/airflow/models/taskinstance\.py$|
373-
^src/airflow/models/taskinstancekey\.py$|
374-
^src/airflow/models/taskmap\.py$|
375-
^src/airflow/models/taskmixin\.py$|
376-
^src/airflow/models/taskreschedule\.py$|
377-
^src/airflow/models/trigger\.py$|
378-
^src/airflow/stats\.py$|
379-
^src/airflow/models/variable\.py$|
380-
^src/airflow/models/xcom\.py$|
381-
^src/airflow/models/xcom_arg\.py$|
382-
^src/airflow/operators/subdag\.py$|
383-
^src/airflow/plugins_manager\.py$|
384-
^src/airflow/providers_manager\.py$|
385-
^src/airflow/secrets/__init__.py$|
386-
^src/airflow/serialization/decoders\.py$|
387-
^src/airflow/serialization/definitions/[_/a-z]+\.py$|
388-
^src/airflow/serialization/encoders\.py$|
389-
^src/airflow/serialization/enums\.py$|
390-
^src/airflow/serialization/helpers\.py$|
391-
^src/airflow/serialization/serialized_objects\.py$|
392-
^src/airflow/settings\.py$|
393-
^src/airflow/task/task_runner/bash_task_runner\.py$|
394-
^src/airflow/task/task_runner/standard_task_runner\.py$|
395-
^src/airflow/ti_deps/deps/mapped_task_upstream_dep\.py$|
396-
^src/airflow/ti_deps/deps/prev_dagrun_dep\.py$|
397-
^src/airflow/ti_deps/deps/trigger_rule_dep\.py$|
398-
^src/airflow/timetables/assets\.py$|
399-
^src/airflow/timetables/base\.py$|
400-
^src/airflow/timetables/simple\.py$|
401-
^src/airflow/triggers/base\.py$|
402-
^src/airflow/utils/cli\.py$|
403-
^src/airflow/utils/context\.py$|
404-
^src/airflow/utils/dag_cycle_tester\.py$|
405-
^src/airflow/utils/dag_edges\.py$|
406-
^src/airflow/utils/dag_parsing_context\.py$|
407-
^src/airflow/utils/decorators\.py$|
408-
^src/airflow/utils/dot_renderer\.py$|
409-
^src/airflow/utils/edgemodifier\.py$|
410-
^src/airflow/utils/email\.py$|
411-
^src/airflow/utils/helpers\.py$|
412-
^src/airflow/utils/operator_helpers\.py$|
413-
^src/airflow/utils/session\.py$|
414-
^src/airflow/utils/task_group\.py$|
415-
^src/airflow/utils/trigger_rule\.py$|
416-
^src/airflow/utils/json\.py$|
417-
^src/airflow/utils/types\.py$
418317
## ONLY ADD PREK HOOKS HERE THAT REQUIRE CI IMAGE
419318
- id: check-schema-defaults
420319
name: Check schema defaults match server-side defaults
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
airflow-core/src/airflow/__init__.py::1
2+
airflow-core/src/airflow/api_fastapi/execution_api/versions/v2026_04_06.py::1
3+
airflow-core/src/airflow/cli/commands/task_command.py::7
4+
airflow-core/src/airflow/cli/commands/triggerer_command.py::1
5+
airflow-core/src/airflow/configuration.py::1
6+
airflow-core/src/airflow/dag_processing/dagbag.py::1
7+
airflow-core/src/airflow/dag_processing/importers/base.py::1
8+
airflow-core/src/airflow/dag_processing/importers/python_importer.py::7
9+
airflow-core/src/airflow/dag_processing/manager.py::4
10+
airflow-core/src/airflow/dag_processing/processor.py::15
11+
airflow-core/src/airflow/exceptions.py::1
12+
airflow-core/src/airflow/executors/base_executor.py::3
13+
airflow-core/src/airflow/jobs/triggerer_job_runner.py::18
14+
airflow-core/src/airflow/models/__init__.py::5
15+
airflow-core/src/airflow/models/asset.py::2
16+
airflow-core/src/airflow/models/connection.py::5
17+
airflow-core/src/airflow/models/dag.py::1
18+
airflow-core/src/airflow/models/dagrun.py::2
19+
airflow-core/src/airflow/models/taskinstance.py::1
20+
airflow-core/src/airflow/models/trigger.py::4
21+
airflow-core/src/airflow/models/variable.py::8
22+
airflow-core/src/airflow/models/xcom_arg.py::1
23+
airflow-core/src/airflow/plugins_manager.py::1
24+
airflow-core/src/airflow/providers_manager.py::5
25+
airflow-core/src/airflow/secrets/__init__.py::1
26+
airflow-core/src/airflow/serialization/decoders.py::2
27+
airflow-core/src/airflow/serialization/definitions/baseoperator.py::1
28+
airflow-core/src/airflow/serialization/definitions/dag.py::2
29+
airflow-core/src/airflow/serialization/definitions/deadline.py::1
30+
airflow-core/src/airflow/serialization/definitions/mappedoperator.py::5
31+
airflow-core/src/airflow/serialization/encoders.py::11
32+
airflow-core/src/airflow/serialization/serialized_objects.py::16
33+
airflow-core/src/airflow/settings.py::1
34+
airflow-core/src/airflow/stats.py::1
35+
airflow-core/src/airflow/timetables/simple.py::1
36+
airflow-core/src/airflow/triggers/base.py::3
37+
airflow-core/src/airflow/utils/cli.py::2
38+
airflow-core/src/airflow/utils/context.py::2
39+
airflow-core/src/airflow/utils/dag_cycle_tester.py::1
40+
airflow-core/src/airflow/utils/dag_edges.py::1
41+
airflow-core/src/airflow/utils/dot_renderer.py::3
42+
airflow-core/src/airflow/utils/edgemodifier.py::1
43+
airflow-core/src/airflow/utils/json.py::4
44+
airflow-core/src/airflow/utils/operator_helpers.py::2

‎scripts/ci/prek/check_sdk_imports_in_core.py‎

Lines changed: 194 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,64 @@
2222
# "rich>=13.6.0",
2323
# ]
2424
# ///
25+
"""Check that no new ``airflow.sdk`` imports are introduced in ``airflow-core``.
26+
27+
All *existing* imports are recorded in ``generated/known_sdk_imports_in_core.txt``
28+
as ``relative/path::N`` entries (one per file), where ``N`` is the maximum
29+
number of ``airflow.sdk`` import statements allowed in that file. A file whose
30+
current count exceeds the recorded limit is treated as a violation -- core
31+
should not gain new runtime dependencies on the Task SDK. Two escape hatches
32+
exist: ``# noqa: SDK001`` on a single import line, which keeps that line out of
33+
the count and out of the allowlist entirely, or raising the file's recorded
34+
count via ``--generate FILE`` when a file genuinely needs several.
35+
36+
``airflow-core/src/airflow/example_dags`` is skipped entirely: those files are
37+
Dags authored with the Task SDK, not core runtime code.
38+
39+
Modes
40+
-----
41+
Default (files passed by prek/pre-commit):
42+
Check only the supplied files; fail if any file's count exceeds the limit.
43+
When a file's count has *decreased*, the allowlist entry is tightened
44+
automatically and the hook exits with a non-zero code so that pre-commit
45+
reports the modified allowlist -- just stage
46+
``generated/known_sdk_imports_in_core.txt`` and re-run.
47+
48+
``--all-files``:
49+
Walk all of ``airflow-core/src/airflow`` and check every ``.py`` file.
50+
51+
``--cleanup``:
52+
Remove entries for files that no longer exist. Safe to run at any time;
53+
does not add new entries or raise limits.
54+
55+
``--generate [FILE ...]``:
56+
With files, record the current counts for just those files and leave every
57+
other entry alone -- the usual way to approve a deliberate new import.
58+
With no files, *rebuild* the allowlist from scratch; that also approves any
59+
unrelated drift, so it is meant for the initial setup or a clean-up sprint.
60+
"""
61+
2562
from __future__ import annotations
2663

2764
import argparse
28-
import sys
65+
import ast
66+
from collections.abc import Iterable
2967
from pathlib import Path
3068

31-
from common_prek_utils import find_import_violations, report_import_violations
69+
from common_prek_utils import AIRFLOW_CORE_ROOT_PATH, AllowlistManager, find_import_violations
70+
from rich.console import Console
71+
72+
console = Console(color_system="standard", width=200)
73+
74+
REPO_ROOT = AIRFLOW_CORE_ROOT_PATH.parent
75+
CORE_SRC_ROOT = AIRFLOW_CORE_ROOT_PATH / "src" / "airflow"
3276

3377
NOCHECK_CODE = "SDK001"
3478

79+
# Dags are authored *with* the task SDK, so example Dags shipped in core are not core code
80+
# for the purpose of this check and never take part in the ratchet.
81+
EXCLUDED_DIR_NAMES = frozenset({"example_dags"})
82+
3583

3684
def check_file_for_sdk_imports(file_path: Path) -> list[tuple[int, str]]:
3785
"""Check file for airflow.sdk imports. Returns list of (line_num, import_statement)."""
@@ -42,23 +90,153 @@ def check_file_for_sdk_imports(file_path: Path) -> list[tuple[int, str]]:
4290
)
4391

4492

45-
def main():
46-
parser = argparse.ArgumentParser(description="Check for SDK imports in airflow-core files")
47-
parser.add_argument("files", nargs="*", help="Files to check")
48-
args = parser.parse_args()
93+
def _can_parse(path: Path) -> bool:
94+
"""Whether *path* is valid enough Python for its import count to be trustworthy.
4995
50-
if not args.files:
51-
return
96+
``find_import_violations`` swallows parse failures and reports zero violations for
97+
them, which is indistinguishable from a file that genuinely has none. Callers must
98+
filter those files out *before* counting, so a file mid-edit with a syntax error
99+
never looks like a legitimate drop in its allowlist entry.
100+
"""
101+
try:
102+
ast.parse(path.read_text(encoding="utf-8"), filename=str(path))
103+
except (OSError, UnicodeDecodeError, SyntaxError):
104+
return False
105+
return True
52106

53-
report_import_violations(
54-
args.files,
55-
check_func=check_file_for_sdk_imports,
56-
violation_label="SDK import(s) in core files",
57-
nocheck_code=NOCHECK_CODE,
58-
only_python_files=True,
107+
108+
class SdkImportsAllowlistManager(AllowlistManager):
109+
def __init__(self, allowlist_file: Path) -> None:
110+
super().__init__(allowlist_file, repo_root=REPO_ROOT)
111+
112+
def is_excluded(self, path: Path) -> bool:
113+
return not EXCLUDED_DIR_NAMES.isdisjoint(path.parts)
114+
115+
def iter_files(self) -> Iterable[Path]:
116+
return (
117+
path for path in CORE_SRC_ROOT.rglob("*.py") if not self.is_excluded(path) and _can_parse(path)
118+
)
119+
120+
def check(self, files: list[Path], allowlist: dict[str, int]) -> int:
121+
checkable = []
122+
for path in files:
123+
if self.is_excluded(path):
124+
continue
125+
if path.exists() and path.suffix == ".py" and not _can_parse(path):
126+
console.print(
127+
f"[yellow]Skipping unparsable file (not counted, allowlist entry left "
128+
f"untouched):[/yellow] {path}"
129+
)
130+
continue
131+
checkable.append(path)
132+
return super().check(checkable, allowlist)
133+
134+
def count_occurrences(self, path: Path) -> int:
135+
return len(check_file_for_sdk_imports(path))
136+
137+
def generate_for(self, files: list[Path]) -> int:
138+
"""Rewrite the allowlist entries for *files* only, leaving every other entry untouched."""
139+
allowlist = self.load()
140+
updated: list[tuple[str, int, int]] = []
141+
142+
for path in files:
143+
if self.is_excluded(path) or not path.exists() or path.suffix != ".py":
144+
continue
145+
if not _can_parse(path):
146+
console.print(f"[yellow]Skipping unparsable file:[/yellow] {path}")
147+
continue
148+
try:
149+
rel = str(path.resolve().relative_to(self.repo_root.resolve()))
150+
except ValueError:
151+
console.print(f"[yellow]Skipping file outside the repository:[/yellow] {path}")
152+
continue
153+
before = allowlist.get(rel, 0)
154+
after = self.count_occurrences(path)
155+
if before == after:
156+
continue
157+
if after:
158+
allowlist[rel] = after
159+
else:
160+
del allowlist[rel]
161+
updated.append((rel, before, after))
162+
163+
if not updated:
164+
console.print("[green]Allowlist already up to date for the given files.[/green]")
165+
return 0
166+
167+
self.save(allowlist)
168+
console.print(
169+
f"[green]Updated[/green] [cyan]{self.allowlist_file.relative_to(self.repo_root)}[/cyan]:"
170+
)
171+
for rel, before, after in updated:
172+
console.print(f" [cyan]{rel}[/cyan] {before} -> {after}")
173+
return 0
174+
175+
def violation_panel_text(self) -> str:
176+
return (
177+
"New [bold]airflow.sdk[/bold] import detected in airflow-core.\n"
178+
"Core (scheduler/API server) should not gain new runtime dependencies "
179+
"on the Task SDK.\n\n"
180+
"The preferred fix is to drop the import. If it has to stay, pick one:\n\n"
181+
"[bold]1. A single deliberate import line[/bold] -- append `# noqa: SDK001` to it.\n"
182+
" The line is then invisible to this check and never reaches the allowlist.\n\n"
183+
"[bold]2. A file that genuinely needs several[/bold] -- record its new count:\n\n"
184+
" [cyan]uv run ./scripts/ci/prek/check_sdk_imports_in_core.py --generate FILE ...[/cyan]\n\n"
185+
" then commit the updated [cyan]generated/known_sdk_imports_in_core.txt[/cyan].\n"
186+
" Passing no FILE rebuilds the whole allowlist and approves any unrelated\n"
187+
" drift with it, so prefer the scoped form."
188+
)
189+
190+
191+
def main(argv: list[str] | None = None) -> int:
192+
parser = argparse.ArgumentParser(
193+
description="Prevent new airflow.sdk imports in airflow-core.",
194+
formatter_class=argparse.RawDescriptionHelpFormatter,
195+
epilog=__doc__,
196+
)
197+
parser.add_argument("files", nargs="*", metavar="FILE", help="Files to check (provided by prek)")
198+
parser.add_argument(
199+
"--all-files",
200+
action="store_true",
201+
help="Check every Python file under airflow-core/src/airflow",
202+
)
203+
parser.add_argument(
204+
"--cleanup",
205+
action="store_true",
206+
help="Remove stale entries from the allowlist and exit",
59207
)
208+
parser.add_argument(
209+
"--generate",
210+
action="store_true",
211+
help=(
212+
"Record the current counts for the given FILEs and exit; "
213+
"with no FILE, rebuild the whole allowlist from the current codebase"
214+
),
215+
)
216+
args = parser.parse_args(argv)
217+
218+
manager = SdkImportsAllowlistManager(REPO_ROOT / "generated" / "known_sdk_imports_in_core.txt")
219+
files = [Path(f).resolve() for f in args.files]
220+
221+
if args.generate:
222+
return manager.generate_for(files) if files else manager.generate()
223+
224+
if args.cleanup:
225+
return manager.cleanup()
226+
227+
allowlist = manager.load()
228+
229+
if args.all_files:
230+
return manager.check(list(manager.iter_files()), allowlist)
231+
232+
if not files:
233+
console.print(
234+
"[yellow]No files provided. Pass filenames or use --all-files to scan the whole repo.[/yellow]"
235+
)
236+
return 0
237+
238+
return manager.check(files, allowlist)
60239

61240

62241
if __name__ == "__main__":
63-
main()
64-
sys.exit(0)
242+
raise SystemExit(main())

0 commit comments

Comments
 (0)