Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| return res | ||
|
|
||
| async def execute(self, *args: P.args, **kwargs: P.kwargs) -> R: | ||
| def execute(self, *args: P.args, **kwargs: P.kwargs) -> WorkflowFuture[R]: |
There was a problem hiding this comment.
⚠️ Cross-Repo: ai-skills TaskGroup fan-out example breaks: execute() returns a Future
Fix here and in cadence-workflow/ai-skills
Breaks in: cadence-workflow/ai-skills - skills/cadence-developer/knowledge/python/workflows.md - parallel fan-out example with asyncio.TaskGroup and tg.create_task(...execute(...))
This PR changes ActivityDefinition.execute to return a WorkflowFuture, which is an asyncio.Future subclass, where it used to return a coroutine. The change is intentional (feat!). The guidance in cadence-workflow/ai-skills, in skills/cadence-developer/knowledge/python/workflows.md, still shows parallel fan-out as tg.create_task(parallel_one.with_options(...).execute("1")) inside asyncio.TaskGroup. create_task only accepts coroutines and raises TypeError when given a Future, so workflows written from that skill will crash on first execution. This PR's own nondeterminism integration tests were migrated from TaskGroup/create_task to asyncio.gather(...) for the same reason. The consumer has to move. On this side, the breaking change should be called out in the release notes so dependent docs and skills get updated.
Here: Keep the new return type. Add a release note or changelog entry (the PR template's 'Release notes' section is empty) saying that execute(), execute_child_workflow(), sleep(), wait_condition() and the signal APIs now return WorkflowFuture. Code must use await, asyncio.gather(...), or .cancel() on the future directly, because asyncio.create_task(...)/TaskGroup.create_task(...) no longer accept these values.
There: In skills/cadence-developer/knowledge/python/workflows.md, rewrite the parallel fan-out example to await the returned futures with asyncio.gather. Do not wrap them in tg.create_task(...).
In cadence-workflow/ai-skills - skills/cadence-developer/knowledge/python/workflows.md:
results = await asyncio.gather(
parallel_one.with_options(
schedule_to_close_timeout=timedelta(seconds=10)
).execute("1"),
parallel_two.with_options(
schedule_to_close_timeout=timedelta(seconds=10)
).execute("2"),
)
Was this helpful? React with 👍 / 👎
| ) -> None: | ||
| self._transition(DecisionState.COMPLETED) | ||
| self._resolve(self.completed, result=event.result) | ||
| self.completed.complete_with_payload(event.result) |
There was a problem hiding this comment.
⚠️ [cross-pr] Merging with #196 loses typed activity results or ActivityError wrapping
Breaks when #199 and #196 both merge (in either order) and the state machine and DecisionManager hunks are combined mechanically. Any activity or child workflow that completes or fails then hits the mismatch.
How
- feat!: Return Futures Instead of Coroutines #199 makes
ActivityStateMachine.handle_completedcallself.completed.complete_with_payload(event.result)(cadence/_internal/workflow/statemachine/activity_state_machine.py:86).DecisionManager.schedule_activity(attrs, converter, result_type)passes aConverterFnintoDecisionFuture(decision_manager.py:159-172). - feat!: Standardize Error Handling and Support Custom Error Reasons #196 gives
ActivityStateMachinea thirddata_converterconstructor argument. Itshandle_failed/handle_timeoutbuild errors withactivity_failed_error(..., self._data_converter), and it resolveshandle_completedwith the rawPayloadvia_resolve. - feat!: Standardize Error Handling and Support Custom Error Reasons #196's
schedule_activitybuildsActivityStateMachine(attrs, future, self._data_converter)with a plain future. feat!: Return Futures Instead of Coroutines #199 buildsActivityStateMachine(attrs, future)with a converting future. - Combining the two means either
handle_failedraisesAttributeErroron_data_converter, orawait execute_activity(..., str)yields a rawPayload. The same split affectsChildWorkflowExecutionStateMachineandschedule_child_workflow.
Fix
- This PR: When rebasing onto feat!: Standardize Error Handling and Support Custom Error Reasons #196, keep
complete_with_payload/ConverterFnfor results. Also passdata_converterintoActivityStateMachineandChildWorkflowExecutionStateMachineso feat!: Standardize Error Handling and Support Custom Error Reasons #196's error builders still work. - feat!: Standardize Error Handling and Support Custom Error Reasons #196: If it lands second, call
complete_with_payloadinhandle_completedinstead of_resolve(..., result=event.result), and adopt feat!: Return Futures Instead of Coroutines #199'sschedule_activity/schedule_child_workflowsignatures.
Compared e7f8d08 with #196 at 59c684d
Was this helpful? React with 👍 / 👎
| async def _run() -> ResultType: | ||
| result_payload = await self._run_workflow_coro( | ||
| definition, child_info, input_payload, headers | ||
| ) | ||
| return cast( | ||
| ResultType, | ||
| self._data_converter.from_data(result_payload, [result_type])[0], | ||
| ) | ||
|
|
||
| loop = get_running_loop() | ||
| execution = WorkflowFuture[WorkflowExecution](loop=loop) | ||
| execution.set_result( | ||
| WorkflowExecution(workflow_id=child_id, run_id=child_run_id) | ||
| ) | ||
| return _drive_future( |
There was a problem hiding this comment.
⚠️ [cross-pr] Test env child failures skip #196's ChildWorkflowError wrapping
Breaks when #199 merges after #196 (or #196 is rebased onto #199), and a child workflow run in TestWorkflowEnvironment raises.
How
- feat!: Return Futures Instead of Coroutines #199 replaces
_start_child_workflowwith_execute_child_workflow. Its_run()coroutine only awaits_run_workflow_coroand converts the result (cadence/testing/_workflow_environment.py:971-978). _drive_futurecopies the task's raw exception into theChildWorkflowFutureImpl(cadence/testing/_workflow_environment.py:118-126).- feat!: Standardize Error Handling and Support Custom Error Reasons #196 wraps child failures with
child_workflow_error_from_exception(...)and re-raisesContinueAsNewErrorin_start_child_workflow. That wrapping has nowhere to go in feat!: Return Futures Instead of Coroutines #199's_run(). - feat!: Standardize Error Handling and Support Custom Error Reasons #196's
_start_child_workflowalso buildsChildWorkflowFutureImpl(workflow_id=, run_id=, result_future=, result_type=, data_converter=). feat!: Return Futures Instead of Coroutines #199 removes that constructor, so the call raisesTypeError.
Fix
- This PR: On rebase, wrap the
_run_workflow_corocall inside_run()in feat!: Standardize Error Handling and Support Custom Error Reasons #196's try/except, which raiseschild_workflow_error_from_exception(...)and re-raisesContinueAsNewError. - feat!: Standardize Error Handling and Support Custom Error Reasons #196: If it merges second, move the wrapping into feat!: Return Futures Instead of Coroutines #199's
_run()coroutine and drop the oldChildWorkflowFutureImpl(...)construction.
Compared e7f8d08 with #196 at 59c684d
Was this helpful? React with 👍 / 👎
| from cadence.error import ( | ||
| ChildWorkflowExecutionFailed, | ||
| StartChildWorkflowExecutionFailed, | ||
| ) |
There was a problem hiding this comment.
⚠️ [cross-pr] New tests import error names that #196 removes, causing ImportError
Breaks when #196 lands without compatibility aliases for its removed error classes, in either merge order. Test collection then fails on import.
How
- feat!: Return Futures Instead of Coroutines #199's
tests/cadence/_internal/workflow/test_context_child_workflow.py:15-18importsChildWorkflowExecutionFailedandStartChildWorkflowExecutionFailedfromcadence.error. - The same file asserts
pytest.raises(ChildWorkflowExecutionFailed, ...)andpytest.raises(StartChildWorkflowExecutionFailed)(test_context_child_workflow.py:326,455). - feat!: Standardize Error Handling and Support Custom Error Reasons #196's
cadence/error.pydefines only the renamedChildWorkflowError/StartChildWorkflowExecutionError/ActivityErrorand has no aliases for the old names, despite what its description says.
Fix
- This PR: After feat!: Standardize Error Handling and Support Custom Error Reasons #196 lands, switch the new tests (and any state machine imports) to
ChildWorkflowError/StartChildWorkflowExecutionError/ActivityError. - feat!: Standardize Error Handling and Support Custom Error Reasons #196: Ship the aliases its description promises (
ChildWorkflowExecutionFailed,StartChildWorkflowExecutionFailed,ActivityFailure) incadence/error.py.
Compared e7f8d08 with #196 at 59c684d
Was this helpful? React with 👍 / 👎
Add WorkflowFuture, a covariant asyncio.Future subclass, to cadence.workflow. Update everything to use it rather than Awaitable or async. Returning coroutines results in confusing behavior, especially as Cadence enforces strict determinism. Decisions are scheduled not when someone calls a method, but when they await on the returned coroutine.
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
This branch has not been deployed
1 participant
Add WorkflowFuture, a covariant asyncio.Future subclass, to cadence.workflow. Update everything to use it rather than Awaitable or async.
Returning coroutines results in confusing behavior, especially as Cadence enforces strict determinism. Decisions are scheduled not when someone calls a method, but when they await on the returned coroutine.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes