Skip to content

Document supported databricks_retry_args usage for deferrable Databricks operators - #68017

Open
kosiew wants to merge 4 commits into
apache:mainfrom
kosiew:callable-deserialization-02-64609
Open

Document supported databricks_retry_args usage for deferrable Databricks operators#68017
kosiew wants to merge 4 commits into
apache:mainfrom
kosiew:callable-deserialization-02-64609

Conversation

@kosiew

Copy link
Copy Markdown
Contributor

What does this PR do?

Follow up for #64960

Adds documentation clarifying which databricks_retry_args configurations are supported when using Databricks operators in deferrable mode.

The new documentation:

  • Explains that databricks_retry_args must be serialization-safe because it is serialized across the trigger boundary when deferrable=True.
  • Documents supported value types (plain Python primitives and collections of primitives).
  • Provides examples of supported configurations, such as {"reraise": True}.
  • Explains that retry count and delay should be configured through the dedicated retry_limit and retry_delay operator parameters.
  • Documents unsupported configurations, including Tenacity strategy objects (stop_after_attempt, wait_incrementing, etc.) and arbitrary callables.
  • Recommends using the non-deferrable Databricks operators when custom callable retry strategies are required.
  • Adds a Databricks provider changelog entry describing the documentation update.

Does this PR introduce any user-facing change?

Yes. Documentation now explicitly describes the serialization requirements and supported shapes for databricks_retry_args in deferrable Databricks operators, helping users avoid unsupported retry configurations.

How was this patch tested?

No tests were added or modified. This PR contains documentation and changelog updates only.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    ChatGPT

@kosiew
kosiewforce-pushed the callable-deserialization-02-64609 branch from 8b7caa8 to 283df3eCompareJune 4, 2026 14:14
@eladkal

Copy link
Copy Markdown
Contributor

cc @moomindani for review

@moomindanimoomindani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up — this is exactly the docs fix scoped at the end of the #64960 thread, and the structure of the two new sections is clean (heading levels, anchors, and code blocks all check out). However, two of the content claims do not match what the merged code actually does, and two files should not be in the diff:

  1. The "Supported (serialization-safe and runtime-valid)" example {"reraise": True} is not runtime-valid: BaseDatabricksHook replaces (does not merge) the default retry_args, so stop/wait are lost and tenacity falls back to stop_never + wait_none() — an infinite, zero-backoff retry loop. Setting databricks_retry_args at all also makes retry_limit/retry_delay no-ops. Since tenacity strategy objects (the only way to set stop/wait inside retry_args) are exactly what deferrable mode rejects, the honest guidance is "do not use databricks_retry_args in deferrable mode; use retry_limit/retry_delay", rather than a supported example.
  2. "will raise ValueError at task submission" overstates the timing: validation runs in the trigger constructors at defer time, after submit_run/run_now has already launched the Databricks run. (The #64960 thread did say "before any Databricks API call", but re-checking the merged code, the operator path submits first and defers second — operators/databricks.py:834-836.)
  3. generated/provider_dependencies.json.sha256sum is a stale-base rebase artifact — please rebase onto current main and drop it.
  4. The hand-added changelog entry should be removed (release-manager-maintained file; see inline).

Details inline.


Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting

Comment threadgenerated/provider_dependencies.json.sha256sum Outdated
Comment threadproviders/databricks/docs/operators/run_now.rst Outdated
Comment threadproviders/databricks/docs/operators/run_now.rst Outdated
Comment threadproviders/databricks/docs/operators/run_now.rst Outdated
Comment threadproviders/databricks/docs/operators/run_now.rst Outdated
Comment threadproviders/databricks/docs/operators/submit_run.rst
Comment threadproviders/databricks/docs/changelog.rst Outdated
@moomindani

Copy link
Copy Markdown
Contributor

Small correction to one point in my review: generated/provider_dependencies.json.sha256sum was re-added to main by #68775 a few hours before I posted (the same accidental re-add pattern as #67080, after #68801 had removed it), so "a file that no longer exists upstream" is no longer accurate. The requested action is unchanged: rebase onto current main and keep this file out of this PR's diff.


Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting

kosiew added 3 commits July 3, 2026 15:14
…ferrable operators
Add "Retry args in deferrable mode" subsection under
DatabricksSubmitRunDeferrableOperator and DatabricksRunNowDeferrableOperator
explaining:
- Serialization requirement: only plain Python primitives allowed across
the trigger boundary
- Supported shapes (int/float primitives, nested plain-dict)
- Unsupported shapes (Tenacity objects, callables) with note that a
ValueError is raised at task submission
- Recommended workaround: use non-deferrable mode for custom retry strategies
Also update changelog for 7.16.0.
and clean up xrefs
- Fixed all Databricks operator cross-references to use full module
paths
- Verified removal of stale short xrefs, reraise, "plain primitives,"
and "task submission" wording
- Updated changelog and ensured generated checksum remains consistent
with main branch
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 20, 2026
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 20, 2026
@eladkal

Copy link
Copy Markdown
Contributor

@moomindani can you review the recent commits?

@moomindanimoomindani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up, and sorry for the slow round two. I re-checked all seven points
from the first review against 5a750de26d and verified them rather than taking the replies
at face value — all seven are genuinely resolved:

  • generated/provider_dependencies.json.sha256sum and the hand-added changelog entry are
    gone; the diff is now the two .rst files only.
  • The boundary is described as Airflow serde with primitives as the common case, which is
    what #64960 settled (its merged test still pins a datetime value as supported).
  • The "validation is not a correctness check" paragraph is accurate: I confirmed
    {"reraise": True} and {"stop": 3} both pass validate_deferrable_databricks_retry_args,
    and that {"stop": stop_after_attempt(3)}, {"wait": wait_incrementing(...)} and
    {"retry": <function>} all raise ValueError (serde TypeError: cannot serialize object of type ... as the cause).
  • The timing wording now matches the code: validation lives in the trigger constructors, so
    the run is already submitted when it fires.
  • retry_limit/retry_delay really are ignored once databricks_retry_args is set
    (databricks_base.py:157-167 — only the else branch consults them).
  • The :class: xrefs now carry the .databricks module segment, and the classes exist there
    while operators/__init__.py still has no re-exports, so the old short form was indeed
    broken. Heading levels also match each file's own hierarchy (^ in run_now.rst, - in
    submit_run.rst), the new labels are unique, and CI's docs + spellcheck builds are green.

One thing I would like fixed before this merges, because it is the kind of inaccuracy this PR
exists to remove.

The deferrable=False escape hatch does not work for retry (or after).
run_now.rst:155-159 and submit_run.rst:241-245 offer
databricks_retry_args = {"retry": my_custom_retry_callable} as an example of something that
is merely un-serializable, and close with "If you need a custom callable retry strategy, use
the non-deferrable operator (deferrable=False)". But BaseDatabricksHook.__init__
unconditionally overwrites both keys whenever retry_args is truthy:

ifretry_args:
self.retry_args=copy.copy(retry_args)
self.retry_args["retry"] =retry_if_exception(self._retryable_error)
self.retry_args["after"] =my_after_func

I checked this directly: with retry_args={"stop": stop_after_attempt(7), "retry": my_fn} the
hook keeps stop but self.retry_args["retry"] is my_fn is False, and the same for after.
So a user who follows the advice gets their callable silently discarded in both modes.

The redirect also walks them into the failure mode from my first round: retry_argsreplaces
the default policy instead of merging, so {"retry": ...} alone leaves no stop and no wait,
and tenacity falls back to stop_never + wait_none() — a persistently retryable API error
then retries forever with zero backoff. Confirmed: with retry_args set, stop and wait are
absent from hook.retry_args.

Concretely, I would drop retry from the strategy list (run_now.rst:140,
submit_run.rst:226) and from the example (run_now.rst:156, submit_run.rst:242), and make
the closing paragraph something like:

If you need a custom stop/wait strategy, use deferrable=False. Note that setting
databricks_retry_args replaces the hook's default retry policy rather than merging with
it, so supply both stop and wait — otherwise tenacity falls back to stop_never
and wait_none() and a persistent API error is retried forever with no backoff. retry
and after are always replaced by the hook, so supplying them has no effect in either mode.

Two smaller things, neither of which needs to hold the PR up:

  1. The new labels (run_now.rst:123, submit_run.rst:209) are named after
    DatabricksRunNowDeferrableOperator / DatabricksSubmitRunDeferrableOperator, which were
    removed in provider 7.0.0 (changelog.rst:680-681) — the surrounding section headings are
    stale, which is pre-existing and not this PR's problem, but the anchors are new and become
    permanent URLs. Naming them howto/operator:DatabricksRunNowOperator:retry-args and
    howto/operator:DatabricksSubmitRunOperator:retry-args costs nothing and does not collide
    with any existing label. For transparency: I raised the stale-heading point separately on
    #71667, which is the PR that would be the natural place to actually rename the headings.
  2. #71667 edits these same two files in the same region (an Airflow 3.1.0 deferral note just
    above the "It allows to utilize Airflow workers..." line). I test-merged the two heads and
    they auto-merge cleanly, so no action needed — but the pages will end up with two adjacent
    deferrable caveats, and the same databricks_retry_args parameter also exists on the
    notebook/task operators and on the SQL statements operator and sensor, whose triggers run
    the very same validation (triggers/databricks.py:61,160). If you would rather not duplicate
    a third copy later, a shared include would cover all of those surfaces at once. Fine to leave
    for a follow-up.

Happy to re-review quickly once the retry/after wording is fixed.


Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting

- Remove misleading retry callable guidance
- Document hook overwrites retry/after
- Clarify non-deferrable escape hatch (stop/wait only)
- Warn that non‑empty args replace defaults; require both stop and wait
- Rename new anchors to match current operator names
@kosiew
kosiew requested a review from moomindaniAugust 20, 2026 09:31

@moomindanimoomindani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — a951860e7 addresses the round-2 finding and the anchor nit, and both hold up when checked
against the code rather than read.

  • retry is gone from the strategy list and from the example, and the replacement sentence is accurate:
    BaseDatabricksHook.__init__ reassigns self.retry_args["retry"] and ["after"] whenever
    retry_args is truthy (hooks/databricks_base.py:157-160), so "the hook always replaces supplied
    retry and after values … in either mode" is exactly right.
  • The deferrable=False paragraph now carries the replace-not-merge warning, and the two names it cites
    check out: tenacity.BaseRetrying.__init__ defaults to stop_never and wait_none() (I read the
    signature defaults directly), so leaving stop/wait unset really does mean unbounded retries with no
    backoff. "non-empty" is the precise qualifier too, since the hook branches on if retry_args: — an
    empty dict keeps the defaults.
  • The labels now name the live classes. No reference to the old
    …DeferrableOperator:retry-args form survives anywhere in the tree, the two new labels are unique, and
    the suffixed howto/operator:DatabricksSubmitRunOperator:retry-args does not collide with the plain
    howto/operator:DatabricksSubmitRunOperator: label at submit_run.rst:20.
  • Mechanics: the code block is terminated by a blank line before the new paragraph, so that text renders
    as prose instead of being swallowed into the literal block, and the docs and spellcheck builds are green
    on this head. Both pages carry the same change.
  • Also test-merged this against #71667, which edits the same two files in the same region: they merge
    cleanly, so neither needs to wait for the other.

The only thing left from my earlier comments is the optional one — databricks_retry_args exists on the
notebook/task operators and on the SQL statements operator and sensor too, whose triggers run the same
validation, so a shared include would eventually beat a third copy of this text. Good as a follow-up;
nothing here should hold this up.

Thanks for the patience on the round-trip time.


Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kosiew@eladkal@moomindani