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