-
Notifications
You must be signed in to change notification settings - Fork 506
(Do not merge yet) Support step context from Python Workflows SDK #5634
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
9c12971 to
70cc713
Compare
dom96
left a comment
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.
LGTM but should you maybe also add some tests which verify what's inside ctx?
02e449c to
183d2c0
Compare
|
I think determining this positionally is likely to confuse people. How are people to remember whether I think it would be a good idea to shift to a design where we look at the argument names, like the way that fixtures work in pytest. So then you could do: @step.do(concurrent=False)
async def step_3(step_1, step_2, context):
...and it could look at the names of the arguments to determine what should go there. We could also get the name of the step from |
We needed one way of consistently determining where the @step.do(concurrent=False, depends=[context])
async def step_3(context):
return "done"If we search the
Yes this is cleaner, but then how would users get the result of |
183d2c0 to
1530900
Compare
CodSpeed Performance ReportMerging this PR will degrade performance by 18.44%Comparing Summary
Performance Changes
Footnotes
|
1530900 to
9a950f0
Compare
src/workerd/server/tests/python/workflow-entrypoint/workflow-entrypoint.wd-test
Show resolved
Hide resolved
9a950f0 to
44b6547
Compare
3bbe2ca to
702c09e
Compare
702c09e to
bf6dfdc
Compare
We're adding support for context within
step.do's callback. This means adding a new optional parameter to the decorated step method, in a way that doesn't break existing Python Workflows.This solution introspects the signature of the callback and checks for the size of the parameters list given (without
*argsand**kwargs). If there are more arguments than resolved dependencies, then the context parameter is injected into the user workflow.EDIT:
We're also making a bigger change in the sdk regarding dependency resolution. We're following pytest fixtures' approach with a name based approach for steps. This means that we implicitly resolve dependencies based on the param names. Since we're already introspecting the signature for each decorated step, this ensures a consistent approach for declarations. Everything is based on the param name, instead of relying on param positions like previously
Related types PR:
#5625