kaxil commented on code in PR #70298:
URL: https://github.com/apache/airflow/pull/70298#discussion_r3768156006
##########
registry/src/provider-version.njk:
##########
@@ -383,6 +383,19 @@ eleventyComputed:
<section class="modules">
<div class="modules-header">
<h2>Modules</h2>
+ {% if pv.isLatest %}
+ {# pv.versionData.modules (older versions) lacks these fields
entirely. #}
+ <div class="capability-filter-toggles">
+ <label class="capability-filter-toggle" title="Reconnects on retry
(ResumableJobMixin) or frees the worker slot while waiting (deferrable) instead
of resubmitting or blocking.">
Review Comment:
"Reconnects on retry (ResumableJobMixin)" names a mechanism neither
durable-marked operator actually uses. `KubernetesPodOperator` is
`(BaseOperator)` (`pod.py:142`) and `AgentOperator` is `(BaseOperator,
HITLReviewMixin)` (`agent.py:120`); both carry the manual marker with a comment
right above it saying "supports durable execution directly, without
ResumableJobMixin" (`pod.py:286-288`, `agent.py:224-226`). `is_durable_capable`
has the two paths precisely so those operators qualify without the mixin, so
the mixin is the one thing the currently-badged modules have in common only by
not using it. The shipped badge copy at :418 describes the behaviour without
naming an SDK class, which reads better for a site visitor anyway.
While you're in here, the Deferrable badge tooltip on :421 says "trigger
state is persisted and reassigned automatically", which is a bit stronger than
what happens: the row persists the classpath and the serialized kwargs, not
progress, so a reassigned trigger re-runs `run()` from the start rather than
resuming. `deferring.rst:170` tells trigger authors to assume exactly that ("a
trigger instance can run *more than once*").
##########
registry/src/provider-version.njk:
##########
@@ -383,6 +383,19 @@ eleventyComputed:
<section class="modules">
<div class="modules-header">
<h2>Modules</h2>
+ {% if pv.isLatest %}
Review Comment:
The `isLatest` gate closes the older-version path, but there's a second
route to the same dead toggle it doesn't cover, and this one hits the pages
people actually land on.
The live catalogue is rebuilt incrementally. `publish-docs-to-s3.yml:655`
passes specific provider ids into `registry-build.yml`, which pulls the
published `modules.json` down from S3, and `merge_registry_data.py:83-84` keeps
every non-targeted provider's module dicts verbatim. `_validate`
(`registry_contract_models.py:233-235`) is `model_validate(payload); return
payload`, so it never writes the Pydantic defaults back into the dict. So after
the next provider release, every provider that wasn't in that build still has
`pv.isLatest` true and no `supports_deferrable` key at all, and ticking either
box blanks its grid.
It isn't only a stale-data problem either. From the current tree, 17 of 105
provider packages contain a class declaring a `deferrable` parameter, and the
durable marker appears in exactly 2 (`pod.py:288`, `agent.py:226`), so 87
packages have neither. Amazon and Google alone account for most of the
deferrable classes. Both toggles are dead controls on the large majority of
provider pages even after a full rebuild.
Counting in the template and rendering each box only when its count is
non-zero fixes the stale-data case and the genuinely-empty case together, and
self-heals when a rebuild lands. The accumulate-in-a-loop pattern already works
in this file (`totalModules` at :38-39), and the type tabs 28 lines down
already gate themselves the same way (`{% if moduleCounts[t.id] > 0 %}` at
:358). Putting the count in the label saves a click too. Worth pairing with the
empty state from `provider-filters.js:81-94`, which already hides the grid and
writes a `#filter-status` count for the providers list; the module grid has no
equivalent, and the "no modules" banner at :454-465 is keyed off `totalModules`
so client-side filtering never reaches it.
##########
registry/src/css/main.css:
##########
@@ -3468,8 +3490,18 @@ main {
font-size: var(--text-xs);
font-weight: var(--font-medium);
border-radius: var(--radius-lg);
+}
+
+.provider-detail-page .modules .module .content h3 .durable-badge {
background: color-mix(in srgb, var(--color-teal-500) 15%, transparent);
- color: var(--color-teal-400);
+ color: var(--color-teal-600);
+ color: light-dark(var(--color-teal-600), var(--color-teal-400));
Review Comment:
This is a real improvement, the light-mode durable badge was effectively
unreadable at 1.62:1 before. It just doesn't quite clear the line: compositing
the 15% `teal-500` wash onto the card's own `--color-card-bg`, which resolves
to `#ffffff` in light mode (`tokens.css:250`), `teal-600` comes out at 3.26:1,
against the 4.5:1 that 12px `--font-medium` text needs. `teal-700` (#0f766e)
gets it to 4.77:1 on the same composite. For what it's worth my last comment
under-called the blue one too: `blue-600` is 4.35:1, just under the line, where
`blue-700` (#1d4ed8) would be 5.64:1. Dark mode is comfortable either way,
7.9:1 and 6.1:1.
##########
registry/src/js/provider-detail.js:
##########
@@ -25,6 +25,9 @@
var extraDepsList = document.getElementById('extra-deps-list');
var extrasDataEl = document.getElementById('extras-data');
var moduleSearch = document.getElementById('module-search');
+ var durableOnlyFilter = document.getElementById('durable-only-filter');
+ var deferrableOnlyFilter = document.getElementById('deferrable-only-filter');
+ var moduleTabs = document.querySelectorAll('.module-tab');
Review Comment:
This one looks like it came back in with the main merge rather than on
purpose. #70190 narrowed the selector to `.module-tab[data-type]`, which is
still there on line 33, so the branch's older copy landing above it leaves two
`var moduleTabs` in the same scope. Nothing breaks today since the second
assignment wins, but the dead line is the exact selector the comment on 31-32
exists to warn against: the natural instinct on seeing two near-identical
declarations is to delete the second one, and then the More button joins
`moduleTabs` and clicking it resets `currentType` to `'all'`.
##########
dev/registry/extract_parameters.py:
##########
@@ -432,6 +433,11 @@ def is_durable_capable(cls: type, resumable_mixin: type |
None) -> bool:
return "execute_resumable" in source
+def supports_deferrable(cls: type) -> bool:
+ """Return True if a class exposes a `deferrable` constructor parameter."""
Review Comment:
`get_params_from_class` unions every ancestor's `__init__` params, so this
answers "does some ancestor name `deferrable`" rather than "can this defer".
Because the union ignores who actually *reads* the parameter, it currently
publishes at least one capability claim that isn't true.
`DiscordWebhookOperator` is `class DiscordWebhookOperator(HttpOperator)` and
is registered in the catalogue (`discord/provider.yaml:74-77`). Its own
`__init__` declares no `deferrable` and forwards `**kwargs` up
(`discord_webhook.py:57-70`), so the union picks the param up from
`HttpOperator.__init__` (`http/operators/http.py:121`). But the only thing that
reads it is `HttpOperator.execute`'s `if self.deferrable:
self.execute_async(...)` (`http.py:164-168`), and
`DiscordWebhookOperator.execute` overrides that outright with a synchronous
`self.hook.execute()` (`discord_webhook.py:95-102`). So it gets the Deferrable
badge, matches both new checkboxes, and ships `supports_deferrable: true` in
`/api/modules.json`, while passing `deferrable=True` frees no worker slot at
all. An inherited knob whose only reader has been overridden is dead.
It misses in the other direction too, which is what makes this a limit of
the proxy rather than a patchable edge case. An operator that defers
*unconditionally* has no flag to expose, so there is no param to find.
`DateTimeSensorAsync` is the clearest: its docstring is "Deferring itself to
avoid taking up a worker slot", its `execute` body is a bare `self.defer(...)`,
and `deferrable` appears nowhere in the file (`sensors/date_time.py:83-143`,
registered at `standard/provider.yaml:102`). I ran the predicate against
`DateTimeSensorAsync`, `MSGraphAsyncOperator`, `AwaitMessageSensor`,
`SSHRemoteJobOperator` and `HITLOperator`: all five return False while each
one's own `execute` or `poke` calls `self.defer(`. So "Deferrable only" hides
exactly the always-deferrable operators, and "Requires deferrable=True" on the
:421 tooltip is unfollowable for them since there is no such param to set. A
sweep suggests a handful of other always-deferring classes worth a spot check,
though I only co
nfirmed these five end to end.
`is_durable_capable` right above already takes the opposite position on
inheritance, and says why: the marker is deliberately name-mangled so it is
*not* inherited, because a subclass that overrides `execute()` "may not
preserve the parent's task_state_store reconnect behavior, so the declaration
must not be inherited". That reasoning applies verbatim here, and this
predicate is the neighbour that ignores it.
I don't have a clean bound to propose. I tried the three that look obvious
and each has a named counter-example in the tree, so this is probably worth
deciding deliberately rather than patching:
- Grep the class's own `execute` for `self.defer(`, the way
`is_durable_capable` greps for `execute_resumable`: false-negatives
`TriggerDagRunOperator`. Its `execute` is `trigger_dagrun.py:233-302` and
delegates, so the `if self.deferrable: self.defer(` sits in `_trigger_dag_af_2`
at `:396-397`, one call away.
- Grep the whole class body instead: false-negatives `TimeDeltaSensorAsync`
(`sensors/time_delta.py:154-167`, registered at `provider.yaml:103`), which
inherits the param *and* the deferring `execute` from `TimeDeltaSensor`, so its
own body contains neither `self.defer(` nor `self.deferrable`.
- Exclude classes that override `execute` at all: drops `HttpOperator`
itself, which defines its own.
A per-class opt-in marker mirroring `__supports_durable_execution` would be
consistent with the durable path and immune to all three, at the cost of
provider-side annotation. Whichever way you go, the Discord shape is worth a
regression test: forwards `**kwargs` to a deferrable parent, overrides
`execute` synchronously, must classify as non-deferrable.
--
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]
