kaxil commented on code in PR #73607:
URL: https://github.com/apache/airflow/pull/73607#discussion_r4201384850
##########
dev/registry/tests/test_extract_parameters.py:
##########
@@ -618,6 +629,70 @@ def _wrap_in_foreign_module(func):
return foreign.__dict__["wrapper"]
+class GrandparentCallingOverridableHelper:
+ """Its execute() dispatches to a helper a subclass is free to replace."""
+
+ def execute(self, context):
+ return self.run_job(context)
+
+ def run_job(self, context):
+ return None
+
+
+class MiddleDelegatingToSuperExecute(GrandparentCallingOverridableHelper):
+ def execute(self, context):
+ return super().execute(context)
+
+
+class SubclassOverridingHelperToDefer(MiddleDelegatingToSuperExecute):
+ """Only the subclass's copy of `run_job` defers, and the call to it lives
two hops up
+ the chain. Resolving `self.<name>()` against the class the hop landed on
instead of the
+ class being asked about finds the grandparent's inert copy and reports not
deferrable.
+ """
+
+ def run_job(self, context):
+ return self.defer()
+
+ def defer(self, *args, **kwargs):
+ return None
+
+
+class SharedHelperReachedAtTwoBudgets:
Review Comment:
This only catches a depth-less `visited` key while
`_MAX_DEFERRAL_WALK_DEPTH` is 6 or lower. I set it to 7 and dropped `depth`
from the helper key: the long leg then reaches `submit` with budget to spare,
`supports_deferrable` still returns True, and the test passes. Pinning the
budget in the test (`patch("extract_parameters._MAX_DEFERRAL_WALK_DEPTH", 6)`)
would keep it meaningful if someone raises the limit.
##########
dev/registry/extract_parameters.py:
##########
@@ -526,14 +526,17 @@ def _execute_chain_calls_resumable(cls: type, depth: int)
-> bool:
"""Return True if some class along `cls`'s resolved `execute()` chain
calls execute_resumable().
Same walk as `_delegates_execute_to`: a delegating override's own source
may not mention
- `execute_resumable` even though the class it hands off to does.
+ `execute_resumable` even though the class it hands off to does. Comments
are stripped
+ for the same reason they are in `_next_execute_hop`: a comment naming the
call is not
+ the call.
"""
owner = _find_owner_of_execute(cls.__mro__, 0)
remaining = depth
while owner is not None:
source = _get_method_source(owner, "execute")
if source is None:
return False
+ source = _strip_comment_lines(source)
Review Comment:
`_strip_comment_lines` only drops whole-line comments, so a trailing `return
None # skips execute_resumable()`, or a docstring that mentions
`execute_resumable`, still marks the class durable. Since the docstring above
says a comment naming the call is not the call, would it be simpler to look for
a real call with `ast` (an `ast.Call` whose `func.attr ==
"execute_resumable"`)? That skips docstrings and string literals too.
--
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]
