-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Register workflow via WorkflowDefinition instead of raw callable #37
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
Conversation
Signed-off-by: Tim Li <ltim@uber.com>
Signed-off-by: Tim Li <ltim@uber.com>
…ent into WorkflowDefinition
Signed-off-by: Tim Li <ltim@uber.com>
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 one other question is whether we want to add workflow.defn, which would be analogous to activity.defn. It defines it as a Workflow type that can later be added to a Registry.
For activities this is important because we have activity classes and interfaces, where we need to define and register them separately. That isn't strictly required for Workflows, but users might find it useful. For example, if they wanted to conditionally add a WorkflowDefinition to a registry or add it to multiple registries.
cadence/worker/_registry.py
Outdated
| cls: Optional[Type] = None, | ||
| **kwargs: Unpack[RegisterWorkflowOptions] | ||
| ) -> Callable: | ||
| ) -> Union[Type, Callable[[Type], Type]]: |
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.
Nit: If the return type is the same as the input type, we can use generics to indicate that. We can do this as a followup.
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.
Thanks, I've added a generic for this
tests/cadence/_internal/workflow/test_workflow_engine_integration.py
Outdated
Show resolved
Hide resolved
cadence/workflow.py
Outdated
| return func(*args, **kwargs) | ||
|
|
||
| # Attach metadata to the function | ||
| wrapper._workflow_run = True # type: ignore |
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.
Nit: All of our other decorators treat the parenthesis as optional. I think there's an argument for us not doing that, but we should be consistent across all of them.
cadence/workflow.py
Outdated
| def wrapper(*args, **kwargs): | ||
| return func(*args, **kwargs) | ||
|
|
||
| # Attach metadata to the function |
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.
Nit: Maybe validate the function is async
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.
Added validation logic.
cadence/workflow.py
Outdated
|
|
||
|
|
||
| # Create a simple namespace object for the workflow decorators | ||
| class _WorkflowNamespace: |
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 don't think we need this, I think it's fine if run is in the root of the module.
If they import like this:
from cadence import workflow
Then they'd get:
@workflow.workflow.run
This does mean that it's possible for them to do from cadence.workflow import run but I think that's fine.
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.
Thanks
cadence/workflow.py
Outdated
| return WorkflowDefinition(cls, name) | ||
|
|
||
|
|
||
| def run(func: Callable[..., T]) -> Callable[..., T]: |
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.
Nit: This type signature is slightly wrong. It's saying that it accepts a function with any type of inputs and returns a function with any type of inputs and the same return type as the other function. It doesn't say that they accept the same inputs.
You could use ParamSpec to represent the arguments to the Callable to enforce that they're the same, or alternatively have T represent the Callable and its arguments, rather than just its return type. Having T represent the entire Callable is probably the easiest, and can be done by adding a bound to the TypeVar.
Feel free to address later.
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.
Thanks, I add a bound to TypeVar with callable.
…ent into WorkflowDefinition Signed-off-by: Tim Li <ltim@uber.com>
Signed-off-by: Tim Li <ltim@uber.com>
What changed?
WorkflowDefinitionclass with type-safe wrapper for workflow functions, including.fn,.name, and.paramsproperties for metadata accessWorkflowDefinitioninternally instead of raw callables, withget_workflow()returning the typed wrapperWorkflowParameterdataclass for parameter introspection (name, type hints, defaults) and@defndecorator for standalone workflow definitionWhy?
ActivityDefinitionpatternHow did you test it?
unit tests
Potential risks
Release notes
Documentation Changes