docs(workflow): clarify RetryPolicy docstrings and add max_attempts alias#1068
docs(workflow): clarify RetryPolicy docstrings and add max_attempts alias#1068vkbajpaii 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>
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.
| 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: |
| backoff_coefficient: Exponential backoff multiplier applied to | ||
| successive retry intervals. Must be ``>= 1``. Defaults to | ||
| ``1.0`` (constant delay between retries). |
| 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. |
kunalworldwide
left a comment
kunalworldwide
left a comment
There was a problem hiding this comment.
Good work on this — the docstrings are significantly clearer now and the max_attempts alias is well-handled. A few observations:
Alias resolution logic: The mutual exclusion check (raise if both or neither are provided) is clean. One thing to verify: does the durabletask-python library underneath handle max_number_of_attempts=1 correctly (i.e., no retries, just the initial attempt)? The docstring says "must be >= 1" which implies yes, but worth confirming with a test case for max_attempts=1.
Test coverage: Good spread — construction with legacy param, new param, both-rejected, neither-rejected, and validation for all fields. The test_allows_backoff_coefficient_none case is a nice edge case catch.
Missing test: No test for max_attempts=1 (single attempt, no retries). This is an important boundary case since users might set it expecting "try once, don't retry" behavior.
Examples updated: All four example files updated from max_number_of_attempts to max_attempts — consistent.
Deprecation notice: The docstring marks max_number_of_attempts as deprecated but there's no warnings.warn() call when it's used. Consider adding a DeprecationWarning so users see it in their logs and migrate proactively. Not a blocker for this PR though — could be a follow-up.
Overall this is a solid improvement to the developer experience. The docstrings now actually explain the retry behavior instead of just restating the parameter names.
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).