You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "shahar1 (via GitHub)" <gi...@apache.org> on 2026/10/17 05:39:20 UTC
[PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
shahar1 opened a new pull request, #74911:
URL: https://github.com/apache/airflow/pull/74911
`PythonDagImporter` sets `DagContext.current_autoregister_module_name` before importing a Dag file, and drops that module from `sys.modules` if the import fails. The name was only reset in `_process_modules`, which is skipped when `_load_modules` raises: the `AirflowConfigException` re-raise for an invalid `get_dagbag_import_timeout` value, or the generic `except Exception` early return (for example, a timeout policy that raises). The stale name stayed on the class. After that, every `with DAG(...)` in the same process ran `DagContext.pop()`, which does `sys.modules[stale_name]` and raised `KeyError`. Dags built by other importers in the same process, such as the YAML importer, failed to build.
This PR moves the reset into a `finally` around `_load_modules`.
### How it surfaced
Three YAML importer tests failed in the `Task SDK tests / task-sdk:P3.11 tests` job of #74889 (run 39004464506). That PR moves CI onto 32-CPU runners, so xdist started 32 workers and grouped tests differently. All three failures were on worker `gw12`:
```
test_import_file_ref_outside_dag_dir_is_rejected[/etc/passwd] - assert False
test_import_yaml_inside_zip_bundle - KeyError: 'unusual_prefix_8ffeae589d38742da930da86c044dccc96dd545f_dag'
test_import_template_file_reads_then_templates - KeyError: 'unusual_prefix_8ffeae589d38742da930da86c044dccc96dd545f_dag'
```
The log runs at verbosity 0, so it does not list which tests ran on each worker. The hash identifies the predecessor anyway: `8ffeae58…` is the sha1 of `/tmp/pytest-of-root/pytest-0/popen-gw12/test_invalid_dagbag_import_tim0/bundle/dag.py`. That is the temp path of `TestPythonDagImporter::test_invalid_dagbag_import_timeout_raises_custom_exception` on `gw12`. The `/etc/passwd` case has the same cause: the `KeyError` raised by `DAG.__exit__` replaces the expected "resolves outside" error.
Minimal deterministic reproduction (Breeze, one process, before this fix gives 3 failed / 1 passed):
```
breeze run pytest -p no:randomly \
task-sdk/tests/task_sdk/importers/test_python_importer.py::TestPythonDagImporter::test_invalid_dagbag_import_timeout_raises_custom_exception \
"task-sdk/tests/task_sdk/importers/yaml_importer/test_importer.py::test_import_file_ref_outside_dag_dir_is_rejected[/etc/passwd]" \
task-sdk/tests/task_sdk/importers/yaml_importer/test_importer.py::test_import_yaml_inside_zip_bundle \
task-sdk/tests/task_sdk/importers/yaml_importer/test_importer.py::test_import_template_file_reads_then_templates
```
### Testing
- The new `test_failed_load_does_not_break_later_dags` covers both error paths. Without the fix, both cases fail with the `KeyError`.
- The reproduction above passes with the fix (4 passed).
- `task-sdk/tests/task_sdk/importers` and `shared/module_loading/tests` pass (305 tests).
- The full task-sdk suite passed twice in Breeze with `-n 32 --skip-db-tests` (4057 passed, 6 skipped each time).
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 5.5)
Generated-by: Claude Code (Opus 5.5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #74911:
URL: https://github.com/apache/airflow/pull/74911#discussion_r4368100104
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
+ # Reset on failure too: DagContext.pop() looks this name up in sys.modules, which
+ # no longer holds a module that failed to load.
Review Comment:
Good catch, thanks. Fixed in e206681369. The comment now uses your wording ("Reset on every exit: ... may not hold it if the load did not complete"). With the restructure from the other thread, the settings cases no longer set the name at all, so the comment now only describes the paths where the module actually started executing.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @shahar1 before posting
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
Review Comment:
Agreed, done in e206681369. I went one step closer than `_load_modules`: the name is now set as the first statement of the nested `parse()` and reset in a `finally` on that same `try`. This means:
- A failing or non-numeric `get_dagbag_import_timeout` never sets the name, because it runs before `parse()`.
- Success, a Dag-file error, the import timeout, and `KeyboardInterrupt` all reset it in the `finally`.
After this change, the two settings test cases no longer reach the `finally`. To keep it covered, I added a `keyboard-interrupt-propagates` case: the Dag file raises `KeyboardInterrupt`. That case fails without the `finally`. The two settings cases fail if the set moves back before the settings call. I also rebased onto latest `main` so CI gets a full run.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @shahar1 before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #74911:
URL: https://github.com/apache/airflow/pull/74911#discussion_r4368100104
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
+ # Reset on failure too: DagContext.pop() looks this name up in sys.modules, which
+ # no longer holds a module that failed to load.
Review Comment:
Good catch, thanks. Fixed in 194a2f6879. The comment now uses your wording ("Reset on every exit: ... may not hold it if the load did not complete"). With the restructure from the other thread, the settings cases no longer set the name at all, so the comment now only describes the paths where the module actually started executing.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @shahar1 before posting
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
Review Comment:
Agreed, done in 194a2f6879. I went one step closer than `_load_modules`: the name is now set as the first statement of the nested `parse()` and reset in a `finally` on that same `try`. This means:
- A failing or non-numeric `get_dagbag_import_timeout` never sets the name, because it runs before `parse()`.
- Success, a Dag-file error, the import timeout, and `KeyboardInterrupt` all reset it in the `finally`.
After this change, the two settings test cases no longer reach the `finally`. To keep it covered, I added a `keyboard-interrupt-propagates` case: the Dag file raises `KeyboardInterrupt`. That case fails without the `finally`. The two settings cases fail if the set moves back before the settings call. I also rebased onto latest `main` so CI gets a full run.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @shahar1 before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #74911:
URL: https://github.com/apache/airflow/pull/74911#discussion_r4367961086
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
+ # Reset on failure too: DagContext.pop() looks this name up in sys.modules, which
+ # no longer holds a module that failed to load.
Review Comment:
Nit (non-blocking): this comment says the module "failed to load". In the two settings cases, the import never started: the name is set before `get_dagbag_import_timeout` is called. Something like "Reset on every exit: `DagContext.pop()` looks this name up in `sys.modules`, which may not hold it if the load did not complete" would cover both.
---
Drafted-by: Claude Opus 5.5; reviewed by `potiuk` before posting
##########
task-sdk/src/airflow/sdk/importers/python_importer.py:
##########
@@ -196,6 +196,10 @@ def import_definition(
)
)
return result
+ finally:
Review Comment:
Nit (non-blocking): putting this `try/finally` inside `_load_modules`, next to `DagContext.current_autoregister_module_name = mod_name`, would keep the set and the reset in one function.
---
Drafted-by: Claude Opus 5.5; reviewed by `potiuk` before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 merged PR #74911:
URL: https://github.com/apache/airflow/pull/74911
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reset Dag autoregistration state when a Python Dag file fails to load [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #74911:
URL: https://github.com/apache/airflow/pull/74911#issuecomment-6413687678
### Backport successfully created: v3-4-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-4-test</td>
<td><a href="https://github.com/apache/airflow/pull/75648"><img src="https://img.shields.io/badge/PR-75648-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org