docs(workflow): clarify RetryPolicy docstrings and add max_attempts alias#1068
Open
vkbajpaii wants to merge 1 commit into
Open
docs(workflow): clarify RetryPolicy docstrings and add max_attempts alias#1068vkbajpaii wants to merge 1 commit into
vkbajpaii wants to merge 1 commit into
Conversation
…lias Addresses dapr#836. * Rewrite the RetryPolicy class-level and per-field docstrings to spell out the behavior that has historically confused users: - max_number_of_attempts counts *total* attempts, not retries - retry_timeout failures propagate to the workflow unless wrapped in try/except - max_retry_interval only caps the per-attempt delay; retries still occur when it is left as None (the historical 'no retries unless max_retry_interval is set' bug was fixed in the vendored _durabletask engine) * Add a new keyword-only max_attempts argument and matching property as the preferred name. The legacy max_number_of_attempts argument and property remain supported for backward compatibility. Specifying both or neither raises ValueError. * Add ext/dapr-ext-workflow/tests/test_retry_policy.py covering construction, validation, the new alias, and the both/neither cases. * Update the workflow examples (simple.py, simple_aio_client.py, multi-app1.py, multi-app2.py) to use max_attempts. Out of scope for this PR (better as separate, focused changes): * Introducing sensible defaults for fields that are currently required (backward-compatibility risk). * Emitting a DeprecationWarning at runtime for max_number_of_attempts (kept as docs-only deprecation for now). Signed-off-by: vkbajpaii <129022330+vkbajpaii@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: vkbajpaii <129022330+vkbajpaii@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the workflow extension’s RetryPolicy developer experience by clarifying docstrings around retry semantics and introducing max_attempts as a clearer alias for max_number_of_attempts, while keeping backward compatibility.
Changes:
- Reworked
RetryPolicydocstrings (including a usage example) to clarify attempt vs retry semantics,retry_timeoutbehavior, andmax_retry_intervalbehavior. - Added
max_attemptskeyword-only alias + validation to require exactly one ofmax_attempts/max_number_of_attempts. - Added unit tests for construction, validation, and alias behavior; updated workflow examples to use
max_attempts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ext/dapr-ext-workflow/dapr/ext/workflow/retry_policy.py | Adds max_attempts alias, validates “exactly one provided”, and rewrites/expands docstrings. |
| ext/dapr-ext-workflow/tests/test_retry_policy.py | Adds focused unit tests for the new alias and validation paths. |
| examples/workflow/simple.py | Updates example usage to prefer max_attempts. |
| examples/workflow/simple_aio_client.py | Updates example usage to prefer max_attempts. |
| examples/workflow/multi-app1.py | Updates example usage to prefer max_attempts. |
| examples/workflow/multi-app2.py | Updates example usage to prefer max_attempts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
113
to
116
| raise ValueError('first_retry_interval must be >= 0') | ||
| if max_number_of_attempts < 1: | ||
| raise ValueError('max_number_of_attempts must be >= 1') | ||
| if attempts_resolved < 1: | ||
| raise ValueError('max_attempts must be >= 1') | ||
| if backoff_coefficient is not None and backoff_coefficient < 1: |
Comment on lines
+81
to
+83
| backoff_coefficient: Exponential backoff multiplier applied to | ||
| successive retry intervals. Must be ``>= 1``. Defaults to | ||
| ``1.0`` (constant delay between retries). |
Comment on lines
+84
to
+88
| max_retry_interval: Upper bound on the delay between any two | ||
| consecutive attempts. When ``None`` (the default) the | ||
| delay grows unbounded according to the backoff. Retries | ||
| still occur when this is ``None``; it only caps the | ||
| per-attempt delay. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses #836.
The current
dapr.ext.workflow.RetryPolicyhas several user-facing gotchas (called out in the issue) that are not visible from its docstrings or field names. This PR is the docs + naming-clarity slice of that issue; behavior changes are left for follow-up PRs.What this PR does
Rewrites the
RetryPolicydocstrings inext/dapr-ext-workflow/dapr/ext/workflow/retry_policy.pyto spell out:max_number_of_attemptscounts the total number of attempts, not retries —5means up to 4 retries after the first attempt.retry_timeoutfailures behave like any other task failure: the surrounding workflow fails unless the call is wrapped intry/except.max_retry_intervalonly caps the per-attempt delay; retries still happen when it is left asNone. (The historical "won't retry unlessmax_retry_intervalis set" bug was fixed in the now-vendored_durabletaskengine — verified in_durabletask/task.pyRetryableTask.compute_next_delay, which contains no early bail-out for unsetmax_retry_interval.)Adds a new
max_attemptskeyword-only argument and property as the preferred name. The legacymax_number_of_attemptsargument and property remain supported for backward compatibility. Specifying both or neither raisesValueErrorto avoid silent inconsistencies.Adds
ext/dapr-ext-workflow/tests/test_retry_policy.pywith 11 tests covering construction, validation, the new alias, and the both/neither rejection paths.Updates the four workflow examples that construct a
RetryPolicy(examples/workflow/simple.py,simple_aio_client.py,multi-app1.py,multi-app2.py) to use the newmax_attemptsname.Acceptance criteria coverage (from #836)
max_attemptsadded as the new preferred field;max_number_of_attemptskept as a deprecated alias (docs-only deprecation for now — no runtimeDeprecationWarningto keep this PR backward-compatible and small)retry_timeoutdocs updated to indicate the surrounding workflow fails when it firesmax_retry_intervalis unset — already true in the vendored_durabletaskengine; the docs now state this explicitlyTesting
Backward compatibility
Existing call sites using
max_number_of_attempts=...continue to work unchanged. Themax_number_of_attemptsproperty is preserved and returns the same value as the newmax_attemptsproperty.Closes #836 partially (sensible-defaults follow-up tracked in description above).