Repository navigation
feat!: Return Futures Instead of Coroutines #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,44 +1,46 @@ | ||
| import asyncio | ||
| from typing import Any, Awaitable, Generator, Type, cast | ||
|
|
||
| from cadence.api.v1.common_pb2 import Payload | ||
| from cadence.data_converter import DataConverter | ||
| from cadence.workflow import ResultType, WorkflowContext | ||
|
|
||
|
|
||
| class ChildWorkflowFutureImpl(Awaitable[ResultType]): | ||
| from typing import Any | ||
|
|
||
| from cadence._internal.workflow.statemachine.decision_state_machine import ( | ||
| CancelFn, | ||
| ConverterFn, | ||
| DecisionFuture, | ||
| ) | ||
| from cadence.api.v1.common_pb2 import WorkflowExecution | ||
| from cadence.workflow import ( | ||
| ChildWorkflowFuture, | ||
| ResultType, | ||
| WorkflowContext, | ||
| WorkflowFuture, | ||
| ) | ||
|
|
||
|
|
||
| class ChildWorkflowFutureImpl( | ||
| DecisionFuture[ResultType], ChildWorkflowFuture[ResultType] | ||
| ): | ||
| def __init__( | ||
| self, | ||
| loop: asyncio.AbstractEventLoop | None = None, | ||
| request_cancel: CancelFn | None = None, | ||
| converter: ConverterFn[ResultType] | None = None, | ||
| *, | ||
| workflow_id: str, | ||
| run_id: str, | ||
| result_future: "asyncio.Future[Payload]", | ||
| result_type: Type[ResultType], | ||
| data_converter: DataConverter, | ||
| execution: WorkflowFuture[WorkflowExecution], | ||
| ) -> None: | ||
| super().__init__(loop, request_cancel, converter) | ||
| self._workflow_id = workflow_id | ||
| self._run_id = run_id | ||
| self._result_future = result_future | ||
| self._result_type = result_type | ||
| self._data_converter = data_converter | ||
| self._execution = execution | ||
|
|
||
| @property | ||
| def workflow_id(self) -> str: | ||
| return self._workflow_id | ||
|
|
||
| @property | ||
| def run_id(self) -> str: | ||
| return self._run_id | ||
| def execution(self) -> WorkflowFuture[WorkflowExecution]: | ||
| return self._execution | ||
|
|
||
| def cancel(self) -> bool: | ||
| """Request cancellation of the child workflow.""" | ||
| return self._result_future.cancel() | ||
|
|
||
| async def signal(self, signal_name: str, *args: Any) -> None: | ||
| def signal(self, signal_name: str, *args: Any) -> WorkflowFuture[None]: | ||
| """Send a signal to this child workflow.""" | ||
| ctx = WorkflowContext.get() | ||
| await ctx.signal_child_workflow(self._workflow_id, signal_name, *args) | ||
|
|
||
| def __await__(self) -> Generator[Any, None, ResultType]: | ||
| payload: Payload = yield from self._result_future.__await__() | ||
| result = self._data_converter.from_data(payload, [self._result_type])[0] | ||
| return cast(ResultType, result) | ||
| return WorkflowContext.get().signal_child_workflow( | ||
| self._workflow_id, signal_name, *args | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| from typing import Any | ||
|
|
||
| from cadence._internal.workflow.statemachine.decision_state_machine import ( | ||
| DecisionState, | ||
| DecisionType, | ||
|
|
@@ -10,7 +12,6 @@ | |
| record_immediate_cancel, | ||
| ) | ||
| from cadence.api.v1 import decision, history | ||
| from cadence.api.v1.common_pb2 import Payload | ||
| from cadence.api.v1.workflow_pb2 import TimeoutType | ||
| from cadence.error import ActivityFailure | ||
|
|
||
|
|
@@ -19,12 +20,12 @@ | |
|
|
||
| class ActivityStateMachine(BaseDecisionStateMachine): | ||
| request: decision.ScheduleActivityTaskDecisionAttributes | ||
| completed: DecisionFuture[Payload] | ||
| completed: DecisionFuture[Any] | ||
|
|
||
| def __init__( | ||
| self, | ||
| request: decision.ScheduleActivityTaskDecisionAttributes, | ||
| completed: DecisionFuture[Payload], | ||
| completed: DecisionFuture[Any], | ||
| ) -> None: | ||
| super().__init__() | ||
| self.request = request | ||
|
|
@@ -82,7 +83,7 @@ def handle_completed( | |
| self, event: history.ActivityTaskCompletedEventAttributes | ||
| ) -> 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| @activity_events.event() | ||
| def handle_failed(self, event: history.ActivityTaskFailedEventAttributes) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here and in
cadence-workflow/ai-skillsBreaks 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.executeto return aWorkflowFuture, which is anasyncio.Futuresubclass, where it used to return a coroutine. The change is intentional (feat!). The guidance incadence-workflow/ai-skills, inskills/cadence-developer/knowledge/python/workflows.md, still shows parallel fan-out astg.create_task(parallel_one.with_options(...).execute("1"))insideasyncio.TaskGroup.create_taskonly accepts coroutines and raisesTypeErrorwhen given a Future, so workflows written from that skill will crash on first execution. This PR's own nondeterminism integration tests were migrated fromTaskGroup/create_tasktoasyncio.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 returnWorkflowFuture. Code must useawait,asyncio.gather(...), or.cancel()on the future directly, becauseasyncio.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 withasyncio.gather. Do not wrap them intg.create_task(...).In cadence-workflow/ai-skills -
skills/cadence-developer/knowledge/python/workflows.md:Was this helpful? React with 👍 / 👎