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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
