ferruzzi commented on code in PR #62645:
URL: https://github.com/apache/airflow/pull/62645#discussion_r2992129557
##########
shared/module_loading/src/airflow_shared/module_loading/__init__.py:
##########
@@ -43,6 +44,18 @@
from types import ModuleType
+def accepts_context(callback: Callable) -> bool:
+ """Check if callback accepts a 'context' parameter, *args, or **kwargs."""
+ try:
+ sig = inspect.signature(callback)
+ except (ValueError, TypeError):
+ return True
+ params = sig.parameters
+ return "context" in params or any(
+ p.kind in (inspect.Parameter.VAR_KEYWORD,
inspect.Parameter.VAR_POSITIONAL) for p in params.values()
Review Comment:
> Adding VAR_POSITIONAL (*args) is a behavioral change from the original
_accepts_context, which only matched named context or **kwargs.
That's fine. It's not released yet and it's an experimental feature. If it
was implemented wrong, this is the time to fix it. The same with only passing
context and dropping the rest of the kwargs. If the user provided kwargs then
those need to be passed through. The only question is if it supports getting
context or not in addition, right? We (Airflow) are appending that Context,
whether they expect it or not, so we need to strip it back out if it would
break things.
I can drop the `VAR_POSITIONAL`, I think you are right that this isn't the
answer... what if we use a try/except to attempt with kwargs, catch the
TypeError if it's positional-only, and retry it positionally? Something like:
```python
kwargs_without_context = {k: v for k, v in callback_kwargs.items() if k !=
"context"}
# Call the callable with all kwargs if it accepts context, otherwise strip
context.
if accepts_context(callback_callable):
result = callback_callable(**callback_kwargs)
else:
result = callback_callable(**kwargs_without_context)
# If the callback was a class then it is now instantiated and callable, call
it.
# Try keyword args first. If the callable only accepts positional args (like
# BaseNotifier.__call__(self, *args)), fall back to passing context
positionally.
if callable(result):
try:
if accepts_context(result):
result = result(**callback_kwargs)
else:
result = result(**kwargs_without_context)
except TypeError:
context = callback_kwargs.get("context")
if context is not None:
result = result(context)
else:
result = result()
```
This is pretty complicated, so I used Claude to generate a list with every
permutation and this is what it came up with, assuming we drop VAR_POSITIONAL
and use the try_with_kwargs/except/retry_without_kwargs:
Step 1 — Initial call (function or class instantiation):
Accepts context (called with all callback_kwargs):
- def my_func(context) — ✓
- def my_func(context=None) — ✓
- def my_func(**kwargs) — ✓
- def my_func(message, context=None) — ✓
- class MyClass.__init__(self, context=None) — ✓
- class MyClass.__init__(self, text, context=None) — ✓
- class MyClass.__init__(self, **kwargs) — ✓
Does not accept context (called with callback_kwargs minus context):
- def my_func() — ✓
- def my_func(message) — ✓ (if kwarg names match)
- class MyClass.__init__(self) — ✓
- class MyClass.__init__(self, text) — ✓ (if kwarg names match)
Step 2 — Callable-class __call__ (try keyword call, except fallback to
positional):
Try succeeds — accepts context (called with all callback_kwargs):
- __call__(self, context) — ✓
- __call__(self, context=None) — ✓
- __call__(self, **kwargs) — ✓
- __call__(self, message, context=None) — ✓ gets both message and context
- __call__(self, *args, **kwargs) — ✓ gets everything
Try succeeds — does not accept context (called with callback_kwargs minus
context):
- __call__(self, message) — ✓ gets message (if kwarg names match)
- __call__(self) with no non-context kwargs — ✓ called with no args
TypeError fallback (keyword call failed, retry positionally):
- __call__(self, *args) with only context in kwargs — ✓ gets context
positionally
- __call__(self, *args) with context + other kwargs — context passed
positionally, OTHER KWARGS DROPPED (unavoidable — no way to know positional
order)
- __call__(self) with non-context kwargs — ✓ falls back to result() or
result(context)
The one lossy case is `*args`-only __call__ with non-context kwargs. Those
kwargs are dropped because we can't know the positional order. This is the
best-effort tradeoff.
--
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]
