potiuk commented on code in PR #70704:
URL: https://github.com/apache/airflow/pull/70704#discussion_r3708428050
##########
task-sdk/src/airflow/sdk/definitions/callback.py:
##########
@@ -66,6 +80,14 @@ def get_callback_path(cls, _callback: str | Callable) -> str:
stripped_callback = _callback.strip()
+ if isinstance(_callback, _SerializedCallbackPath):
+ # This path was already checked when the Callback it belongs to
was created, so
+ # rebuilding that Callback keeps it as it is. Reconstruction
happens in components
+ # which never call the callback themselves, and resolving the path
there is neither
+ # needed nor wanted: the module it names is not necessarily
importable in that
+ # process, and importing it has no bearing on the path we return
either way.
+ return stripped_callback
Review Comment:
Right — it was narrated three times: the class docstring, the branch in
`get_callback_path`, and again in `deserialize`. Kept it at
`get_callback_path`, where the decision is actually made and where the
non-obvious part lives (the module named by a stored path is not necessarily
importable in the process doing the reconstruction). The class docstring is now
one line pointing there, and the `deserialize` comment is gone.
---
Drafted-by: Claude Code (Opus 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]
