ferruzzi commented on code in PR #66702: URL: https://github.com/apache/airflow/pull/66702#discussion_r3222925283
########## task-sdk/src/airflow/sdk/execution_time/callback_supervisor.py: ########## @@ -66,6 +69,7 @@ class _BundleInfoLike(Protocol): __all__ = ["CallbackSubprocess", "supervise_callback"] log: FilteringBoundLogger = structlog.get_logger(logger_name="callback_supervisor") +UNUSUAL_MODULE_PREFIX = "unusual_prefix_" Review Comment: 100% agree. This "must be in a different location" is my personal but of tech debt that I never figured out and I'm very glad to see someone fixing it. Thanks! The prefix should definitely be tied to that constant you linked now that we are fixing this correctly. I think either `airflow._shared` or `airflow.sdk._shared`? I'm not sure which. ########## task-sdk/src/airflow/sdk/execution_time/callback_supervisor.py: ########## Review Comment: I think we need to initialize `bundle_path = None` before this `if` block so execute_callback has a default value, no? Otherwise what happens if there is no bundle_info and it hits line 241? Won't that crash out because bundle_path is undefined? Also, maybe that means we need an additional test case? -- 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]
