temporalio / sdk-python

Temporal Python SDK
MIT License
447 stars 66 forks source link

[Bug] workflow.start_child_workflow() behaving differently than workflow.execute_child_workflow() in tests #586

Open cpg1111 opened 1 month ago

cpg1111 commented 1 month ago

What are you really trying to do?

Hi there, I am writing tests for a workflow in which it spawns N child workflows using workflow.start_child_workflow(), places them in a list, and resolves the result later, like the following example:

class Child:
    async def run(self) -> bool:
        return True

class Parent:
    async def run(self, n: int):
        children = []

        for _ in range(n):
            child = await workflow.start_child_workflow(Child, ...)
            children.append(child)

        ...
        for child in children:
            result = await child.result()
            # do something with result 

and test code like the following:

async def test_parent():
    # setup test env worker with both workflows
    wf = await env.client.start_workflow("parent", ...)
    await env.sleep(duration=timedelta(seconds=5))
    await wf.result()

Describe the bug

Even when I have that exact child workflow, I get an InvalidStateError saying Result is not set. If I do not put the handler in a list and immediately call await child.result() it blocks indefinitely. However if I use workflow.execute_child_workflow() everything works fine. It's also worth noting these workflows work fine in an actual execution, this issue only exists using the testing.WorkflowEnvironment.

Minimal Reproduction

Write a workflow that calls multiple child workflows using workflow.start_child_workflow(), write a test with a single sleep before awaiting the parent's result.

Environment/Versions

Additional context

N/A

cretz commented 1 month ago

await child.result()

This is not valid API and a type checker should catch this. Starting a child workflow returns a ChildWorkflowHandle which is an asyncio.Task that is already waiting on result and should be awaited itself to get the result (e.g. just await child). This is unlike starting a workflow from a client which uses a workflow handle that can be fetched in other ways and therefore is not always a task and therefore does have a separate .result() to start the awaiting process.

cpg1111 commented 1 month ago

Ah ok, yes that fixed it. I didn't see anything around this in the docs regarding ChildWorkflowHandle, just that it inherits from asycnio.Task, I'd be happy to submit a PR including this, assuming the docs are autogenerated?

cretz commented 1 month ago

Unsure which docs you are referring to (there is https://github.com/temporalio/sdk-python/blob/main/README.md, https://python.temporal.io, and https://docs.temporal.io/develop/python/) but yes, we happily accept contributions on them all.

cpg1111 commented 1 month ago

I was referring to the second link, but I can submit PRs to the others where it makes sense.

cretz commented 1 month ago

API docs make sense. That is auto generated from docstrings yes. PRs to this repo welcome to update those API docs.