-
Notifications
You must be signed in to change notification settings - Fork 141
Serialization context changes in anticipation of Standalone Activity #1155
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?
Conversation
| namespace: str | ||
| """Namespace used by the worker executing the workflow.""" | ||
|
|
||
| workflow_id: Optional[str] |
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.
Should this be optional in WorkflowSerializationContext?
|
|
||
|
|
||
| test_traces: dict[str, list[TraceItem]] = defaultdict(list) | ||
| test_traces: dict[Optional[str], list[TraceItem]] = defaultdict(list) |
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.
Unrelated to this change, but a quick glance makes it seem like we might be able to move this global state into individual tests. If possible I'd prefer that vs tests calling del test_traces[] to clean up their expected entries.
| namespace: str | ||
| """Namespace used by the worker executing the activity.""" | ||
|
|
||
| activity_id: Optional[str] |
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.
What situations is this 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.
I think we should consider waiting to do this until we're implementing standalone activities in Python instead of ahead of time. That way if the implementer encounters anything while implementing standalone activities that can affect this, they can adjust accordingly.
Eliminate shared base class -- it's not appropriate in the future when
ActivitySerializationContextwill be applied at the top level (and even in the original workflow-activity case, we moved away from nested scopes)Make workflow identifiers optional in activity context
Add activity ID to activity context
Note
Restructures workflow/activity serialization contexts (removing shared base), makes workflow IDs optional, adds activity IDs, and updates workers/runtime/tests to construct and assert the new contexts.
BaseWorkflowSerializationContext;WorkflowSerializationContextnow directly extendsSerializationContextwithnamespaceand optionalworkflow_id.ActivitySerializationContext: addactivity_id, includenamespace, and makeworkflow_id/workflow_typeoptional; add field docstrings.worker/_activity.pyand_workflow_instance.pyto the new shapes and to populateactivity_id.activity_idto activity executions and expected context dicts; relax types (e.g., trace map key to Optional[str]).EventWorkflowtoWaitForSignalWorkflowin async activity completion test and update signal calls accordingly.Written by Cursor Bugbot for commit 89b721a. This will update automatically on new commits. Configure here.