potiuk commented on code in PR #65423: URL: https://github.com/apache/airflow/pull/65423#discussion_r3176651504
########## airflow-core/.pre-commit-config.yaml: ########## @@ -22,6 +22,20 @@ default_language_version: node: 22.19.0 golang: 1.24.0 repos: + - repo: local + hooks: + - id: check-error-code-in-exceptions + name: Check error_code usage in exceptions + entry: ../scripts/ci/prek/check_airflow_error_codes.py + language: python + files: > + (?x) + ^src/airflow/.*\.py$| Review Comment: This regex doesn't do what it looks like. This hook lives in `airflow-core/.pre-commit-config.yaml`, which prek runs from the `airflow-core/` directory. File paths in the `files:` regex are matched relative to that directory, so: - `^src/airflow/.*\.py$` matches `airflow-core/src/airflow/...` files ✅ - `^providers/.*\.py$` ❌ — `providers/` is a sibling of `airflow-core/`, not underneath it; this branch never matches anything - `^task-sdk/.*\.py$` ❌ — same issue So the static check **only validates raises in `airflow-core/src/airflow/`**. Provider raises and task-sdk raises sail right past it, even though the YAML mapping (and the test file `test_check_airflow_error_codes.py`) clearly intend the check to be repo-wide. Two fixes: 1. Move the hook to the **top-level** `.pre-commit-config.yaml` (where paths are repo-rooted) and keep the current regex roughly as-is — that's where you want a check that spans `airflow-core/`, `providers/`, and `task-sdk/`. 2. Or, if the intent really is "airflow-core only for now", drop the `providers/` and `task-sdk/` branches and put a `# TODO(...)` referencing a follow-up tracking issue per [AGENTS.md "Tracking issues for deferred work"](https://github.com/apache/airflow/blob/main/AGENTS.md). CI passing here is misleading because the hook silently no-ops for the intended scope. ########## task-sdk/src/airflow/sdk/exceptions.py: ########## @@ -33,6 +33,26 @@ from airflow.sdk.execution_time.comms import ErrorResponse +class AirflowErrorCodeMixin: + """Mixin class providing ability to pass error_code to an exception.""" + + def __init__(self, *args, error_code=None, **kwargs): + self.error_code = error_code + super().__init__(*args, **kwargs) + + def __str__(self): + base = super().__str__() + if self.error_code is None: + return base + error_code_str = f"[airflow error code={self.error_code}]" Review Comment: The string format `[airflow error code=AERR003]` is observable in user logs, stack traces, and any tooling that pattern-matches on exception messages — so it's effectively a public-API choice. Two suggestions: 1. **Drop the `key=value` form**: convention in most projects is a bare bracketed code (`[AERR003]`) or parenthesized (`(AERR003)`). The current form repeats "airflow" in every message even though the prefix `AERR` already encodes that, and the `=` reads like a kwarg leakage. 2. **Prefix, not suffix**: `[AERR003] schedule interval must be positive…` reads more naturally and is easier to grep for than a trailing tag. Compare to how `errno` works in libc-style messages. Whatever format wins, please also add a `airflow-core/newsfragments/65423.significant.rst` entry — the change in exception message format is observable behavior for anyone scraping logs. ########## task-sdk/src/airflow/sdk/exceptions.py: ########## @@ -33,6 +33,26 @@ from airflow.sdk.execution_time.comms import ErrorResponse +class AirflowErrorCodeMixin: Review Comment: This mixin is duplicated **verbatim** in `airflow-core/src/airflow/exceptions.py` (lines 46-63), inside the `except ModuleNotFoundError` fallback branch. If the two implementations ever drift, behavior of `AirflowNotFoundException` (etc.) will silently differ between the SDK-installed and `_AIRFLOW__AS_LIBRARY` modes. The right home for this is `shared/` — see `shared/providers_discovery/`, `shared/logging/`, etc. for the existing pattern. Move `AirflowErrorCodeMixin` (and the `error_code` field on it) into a small shared distribution like `shared/exceptions/`, and import it from both `airflow-core/src/airflow/exceptions.py` and `task-sdk/src/airflow/sdk/exceptions.py`. That's also what AGENTS.md "Shared libraries" guidance points at. ########## scripts/ci/prek/common_prek_utils.py: ########## @@ -79,6 +79,50 @@ def temporary_tsc_project( yield temp_tsconfig_path +def is_valid_python(contents: str) -> bool: + """ + Checks if contents is valid python code or not. + args: + contents (str): Code contents. + returns: True if valid python code, False otherwise. + """ + try: + compile(contents, "<string>", "exec") + return True + except SyntaxError: + return False + + +@contextmanager +def temporary_python_file(dir_path: Path, contents: str) -> Generator[_TemporaryFileWrapper, None, None]: Review Comment: Two concerns about adding this to the shared `common_prek_utils.py`: 1. **YAGNI** — only one caller exists right now (`test_check_airflow_error_codes.py`). Per AGENTS.md "Don't add features… beyond what the task requires", inline this in the test file (or in a nearby `conftest.py`) until a second caller appears. Keeping `common_prek_utils.py` lean for things actually shared between hooks keeps the module's blast radius small. 2. **Debug prints in a shared utility** (line 115-119) — `console.print(...)` of the *entire python source* on every invocation is going to spam test output noisily. If you keep the helper, drop the print or gate it on a `verbose=False` parameter that defaults off. ########## task-sdk/src/airflow/sdk/exceptions.py: ########## @@ -33,6 +33,26 @@ from airflow.sdk.execution_time.comms import ErrorResponse +class AirflowErrorCodeMixin: + """Mixin class providing ability to pass error_code to an exception.""" + + def __init__(self, *args, error_code=None, **kwargs): + self.error_code = error_code + super().__init__(*args, **kwargs) + + def __str__(self): + base = super().__str__() + if self.error_code is None: + return base + error_code_str = f"[airflow error code={self.error_code}]" + return f"{base} {error_code_str}" if base else error_code_str + + def __repr__(self): + if self.error_code is None: + return super().__repr__() + return f"{self.__class__.__name__}(airflow error code={self.error_code}, args={self.args})" Review Comment: This `__repr__` doesn't round-trip. Conventional `repr` for an exception is `AirflowNotFoundException('msg')` — eval-able when the args themselves are literals. The new form `AirflowNotFoundException(airflow error code=AERR001, args=('msg',))` is neither: - not eval-able (`airflow error code` is not a Python identifier); - the `args=...` framing duplicates information already in the error code prefix. If you need a repr that includes the code, mirror the `__init__` signature: ```python def __repr__(self): args_repr = ", ".join(repr(a) for a in self.args) if self.error_code is None: return f"{type(self).__name__}({args_repr})" return f"{type(self).__name__}({args_repr}, error_code={self.error_code!r})" ``` That's eval-friendly and matches how the exception was actually constructed. ########## devel-common/src/sphinx_exts/error_guide_generator.py: ########## @@ -0,0 +1,133 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import os +from pathlib import Path + +import yaml + +AIRFLOW_ROOT_PATH = Path(os.path.abspath(__file__)).parents[3] +GENERATED_PATH = AIRFLOW_ROOT_PATH / "airflow-core" / "docs" / "contributor" / "error_codes" +GENERATED_PATH.mkdir(parents=True, exist_ok=True) Review Comment: `GENERATED_PATH.mkdir(parents=True, exist_ok=True)` runs at **module import time**. Any code that imports this extension (Sphinx itself, but also any test that imports the module) silently mutates the filesystem at `airflow-core/docs/contributor/error_codes/`. That's a side effect Pythonic style says doesn't belong at module top. Move it inside `generate_error_docs(app)`, right before you start writing files. That way: - importing the module is pure; - the directory only gets created during an actual Sphinx build; - the path is computed once from a real `app.confdir` rather than a `__file__`-derived guess (Sphinx exts commonly use `app.confdir` / `app.srcdir` for this). Two related items while you're in here: - The generated `airflow-core/docs/contributor/error_codes/` directory is **not in any `.gitignore`** I could find. After @ashb's earlier point about not committing the generated `AERR001.rst`, the next dev who runs `breeze build-docs` locally will see a tree of untracked files in their `git status`. Add a `.gitignore` (or `airflow-core/docs/.gitignore` carve-out) covering this directory. - `index.rst` only gets regenerated if `error_mapping.yml` exists. If someone moves/renames the YAML, the build fails with a `toctree` reference to a missing file — which is a confusing error. Consider making `airflow-core/docs/index.rst`'s reference to `contributor/error_codes/index` conditional on the extension having generated something, or fail fast with a clearer message. ########## scripts/tests/ci/prek/test_check_airflow_error_codes.py: ########## @@ -0,0 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from unittest import mock + +from check_airflow_error_codes import check_for_errors +from ci.prek.common_prek_utils import AIRFLOW_CORE_SOURCES_PATH, temporary_python_file + + +class TestCheckAirflowErrorCodes: + def setup_method(self): + self.exc_def = """ +class ErrorCodesTestException(Exception): + pass + +raise ErrorCodesTestException""" + + @mock.patch("check_airflow_error_codes.load_error_mapping") + def test_raise_with_unmapped_error_code(self, mock_load_error_mapping): + mock_load_error_mapping.return_value = {"AERRTEST": "ErrorCodesTestException"} + + nonexistent_error_code = "AERRNOTINMAPPINGFILE" + contents = f"{self.exc_def}('this is a test', error_code='{nonexistent_error_code}')" + with temporary_python_file(AIRFLOW_CORE_SOURCES_PATH, contents) as f: + errors = check_for_errors(paths=[f.name]) + + assert len(errors) == 1 + error_substr = f"'{nonexistent_error_code}' not found in error mapping file" + assert error_substr in errors[0] + + @mock.patch("check_airflow_error_codes.load_error_mapping") + def test_raise_with_mapped_error_code(self, mock_load_error_mapping): + mock_load_error_mapping.return_value = {"AERRTEST": "ErrorCodesTestException"} + mapped_error_code = "AERRTEST" + + contents = f"{self.exc_def}('this is a test', error_code='{mapped_error_code}')" + with temporary_python_file(AIRFLOW_CORE_SOURCES_PATH, contents) as f: Review Comment: These tests write temp files into `AIRFLOW_CORE_SOURCES_PATH` (the actual source tree at `airflow-core/src/airflow/`). Two issues: 1. **Source-tree pollution.** A test crash or KeyboardInterrupt before the `delete=True` runs leaves stale `testing_temp_*.py` files inside `airflow-core/src/airflow/` — they then get picked up by other tools that walk the source tree (mypy, ruff, the very prek hook this PR is adding, etc.). 2. **It's unnecessary.** `validate_file()` opens by path and parses with `ast.parse` — it doesn't care where the file lives. Use the standard pytest `tmp_path` fixture and the tests work identically without ever touching the source tree. Suggested: ```python def test_raise_with_mapped_error_code(self, tmp_path, mock_load_error_mapping): mock_load_error_mapping.return_value = {"AERRTEST": "ErrorCodesTestException"} f = tmp_path / "fake_module.py" f.write_text(f"{self.exc_def}('this is a test', error_code='AERRTEST')") errors = check_for_errors(paths=[str(f)]) assert errors == [] ``` That also removes the dependency on `temporary_python_file` from the test suite, which makes the YAGNI-on-the-helper point in the other comment easier to act on. ########## airflow-core/docs/error_mapping.yml: ########## @@ -0,0 +1,149 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +--- + +error_guide: + description: Quick debug guide for common errors while using Airflow. + error_codes: + # Specify error codes below for exception classes as required. + # Exception class can have multiple error codes representing its respective error states. + # e.g. AirflowNotFoundException can have AERR001, AERR002, etc representing different not found cases. + - error_code: AERR001 + exception_type: AirflowNotFoundException + user_facing_error_message: TaskSDK connection not found for requested connection ID + description: This error occurs when Airflow is unable to locate a connection + with given connection id in any configured secrets backend or metadata + store. It attempted to retrieve the connection via the Task SDK, but no + matching entry was found. + first_steps: Verify that the connection ID is correctly configured in Airflow. + Check your secrets backend, environment variables, or Airflow UI to + ensure the connection exists and is accessible to the current task. + Confirm there are no typos in the conn_id. If using a secrets backend, + ensure it is properly configured and reachable. + documentation: > + https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#using-connections + + - error_code: AERR002 + exception_type: AirflowNotFoundException + user_facing_error_message: Requested connection ID is not defined + description: This error occurs when Airflow attempts to resolve a connection + using its standard secrets resolution flow (SecretCache and configured + secrets backends) and is unable to find any matching connection for + given conn_id. It also indicates that all configured backends were + checked sequentially but none returned a valid connection. + first_steps: > + Confirm that the connection is defined in at least one configured + secrets backend or in the Airflow metadata database. Check whether Review Comment: Stepping back to the design: this PR introduces a developer convention (error codes), a YAML registry, a Sphinx generator, a static check, and a mixin on shared exception classes. That's a substantial new mechanism with cross-cutting impact, and I think it needs more design buy-in before merging. A few questions I'd want answered up-thread: 1. **Is there a dev list thread or AIP that endorses this whole proposal?** The PR body links #43171 (a rather different cleanup) but not a design discussion. @Lee-W referenced an "AirflowException should no longer be used" thread — that's adjacent but it's about *narrowing* what we share on a base class, while this PR *adds* a new shared mechanism on top of it. AGENTS.md "Boundaries → Ask first → Large cross-package refactors" applies; please link the discussion in the PR body, or start one if it hasn't happened. 2. **kwarg vs embedded — Lee-W's open question.** The current shape makes `error_code` an opt-in kwarg that devs remember to pass at each raise site. With ~7 codes added across 7 raises in this PR (1:1), the embed-in-class alternative is barely heavier and gets you: - no per-raise discipline / no "the dev forgot" failure mode; - one source of truth (the class itself) for the code↔exception mapping rather than YAML + raise sites + class hierarchy all needing to agree; - the static check becomes "every `raise X(...)` where X is in the registry has the right code attached" instead of "the code that was passed matches" — stronger invariant. Your counter (multiple raise sites needing different codes) is real but IMO points at the answer being **subclass for the semantically distinct cases** rather than tagging a single class with multiple codes. e.g. `ConnectionNotFound(AirflowNotFoundException)` and `TaskSDKConnectionNotFound(AirflowNotFoundException)` is more pythonic than one `AirflowNotFoundException` with codes `AERR001` and `AERR002` distinguishing the two cases. 3. **YAML as the registry has long-term cost.** Renaming an exception class today requires a code change *plus* a YAML edit *plus* a doc regeneration; the static check catches the YAML drift only on raise sites that opt in. A code-side registry (decorator on the class, or a class-attribute) would keep these in lockstep and remove the YAML round-trip entirely. Worth at least considering before YAML becomes the load-bearing source of truth. I don't think these are blockers in the "code is broken" sense — they're "is this the right shape before we ship it and have to live with it" sense. The implementation work here is solid; the design needs another pass with more maintainer eyes before it lands. -- 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]
