potiuk commented on code in PR #73607:
URL: https://github.com/apache/airflow/pull/73607#discussion_r4307667288
##########
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)
if "execute_resumable" in source:
return True
Review Comment:
Following the `ast` idea above: this counts only a real
`….execute_resumable(...)` call, so a trailing comment or a docstring naming it
no longer marks the class durable.
```suggestion
if any(
isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "execute_resumable"
for node in ast.walk(ast.parse(textwrap.dedent(source)))
):
return True
```
##########
dev/registry/tests/test_extract_parameters.py:
##########
@@ -706,6 +781,12 @@ def defer(self, *args, **kwargs):
def test_comment_mentioning_defer_is_not_mistaken_for_deferral(self):
assert supports_deferrable(DeferralCommentOnlyOperator) is False
+ def
test_helper_is_resolved_against_the_class_under_test_not_the_hop_target(self):
+ assert supports_deferrable(SubclassOverridingHelperToDefer) is True
+
+ def test_helper_reached_again_with_more_depth_budget_is_rewalked(self):
+ assert supports_deferrable(SharedHelperReachedAtTwoBudgets) is True
Review Comment:
Parametrized, with the walk depth pinned so the second case keeps testing
the depth-aware `visited` key if `_MAX_DEFERRAL_WALK_DEPTH` is ever raised:
```suggestion
@pytest.mark.parametrize(
"operator_cls",
[
pytest.param(SubclassOverridingHelperToDefer,
id="helper-resolved-against-class-under-test"),
pytest.param(SharedHelperReachedAtTwoBudgets,
id="helper-rewalked-with-more-depth-budget"),
],
)
# SharedHelperReachedAtTwoBudgets only exercises the depth-aware
`visited` key at this budget.
@patch("extract_parameters._MAX_DEFERRAL_WALK_DEPTH", 6)
def test_helper_call_is_walked_to_the_deferral(self, operator_cls):
assert supports_deferrable(operator_cls) is True
```
##########
dev/registry/tests/test_extract_parameters.py:
##########
@@ -382,6 +382,17 @@ def execute(self, context):
assert is_durable_capable(CommentedNonDelegatingSubclass,
FakeResumableJobMixin) is False
+ def
test_comment_mentioning_execute_resumable_is_not_mistaken_for_a_call(self):
+ """The mixin walk strips comments the same way the delegation walk
does, or a
+ subclass that only names `execute_resumable` in a comment reports
capable."""
+
+ class CommentedNonResumableOperator(FullyImplementedResumableOperator):
+ def execute(self, context):
+ # this path deliberately skips execute_resumable(context)
+ return None
+
+ assert is_durable_capable(CommentedNonResumableOperator,
FakeResumableJobMixin) is False
Review Comment:
Covers the trailing-comment and docstring cases as well (the
trailing-comment one fails on the current head):
```suggestion
def test_mentioning_execute_resumable_is_not_mistaken_for_a_call(self):
"""Only a real `execute_resumable()` call counts: a whole-line
comment, a trailing
comment or a docstring naming it would otherwise report the subclass
capable."""
class WholeLineComment(FullyImplementedResumableOperator):
def execute(self, context):
# this path deliberately skips execute_resumable(context)
return None
class TrailingComment(FullyImplementedResumableOperator):
def execute(self, context):
return None # deliberately skips execute_resumable(context)
class Docstring(FullyImplementedResumableOperator):
def execute(self, context):
"""Deliberately skips execute_resumable()."""
return None
for operator_cls in (WholeLineComment, TrailingComment, Docstring):
assert is_durable_capable(operator_cls, FakeResumableJobMixin)
is False, operator_cls.__name__
```
--
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]
