change call_child_workflow to pass correct wf name to durabletask worker #616
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
durabletask.worker uses orchestrator.__name__ at https://github.com/microsoft/durabletask-python/blob/c9990973a7cc18b2db1219d129961cede9eb3948/durabletask/worker.py#L387 but when dapr_workflow_context wraps the real workflow function in `wf`, the correct orchestrator name is lost. This bug results in an exception: durabletask.task.TaskFailedError: Sub-orchestration task #1 failed: A 'wf' orchestrator was not registered. This proposed fix is not pretty, but it works. Signed-off-by: Brad Clements <bkc@murkworks.com>
cgillum
approved these changes
Oct 16, 2023
| @@ -61,6 +61,7 @@ def call_child_workflow(self, workflow: Workflow, *, | |||
| def wf(ctx: task.OrchestrationContext, inp: TInput): | |||
| daprWfContext = DaprWorkflowContext(ctx) | |||
| return workflow(daprWfContext, inp) | |||
| wf.__name__ = workflow.__name__ # copy workflow name so durabletask.worker can find the orchestrator in its registry | |||
| return self.__obj.call_sub_orchestrator(wf, input=input, instance_id=instance_id) | |||
There was a problem hiding this comment.
While hacky, this feels like the right fix given the current limitations of the durabletask-python library. Ideally we add a parameter to call_sub_orchestrator that allows overriding the name so that callers don't need to be aware of the internal implementation details.
@DeepanshuA thoughts?
There was a problem hiding this comment.
I'll merge this and you can consider a better solution in a future release.
There was a problem hiding this comment.
Comment line is too long. Please see Python style guide ;)
berndverst
previously approved these changes
Oct 31, 2023
berndverst
reviewed
Oct 31, 2023
ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_context.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Bernd Verst <github@bernd.dev>
berndverst
previously approved these changes
Oct 31, 2023
berndverst
reviewed
Oct 31, 2023
ext/dapr-ext-workflow/dapr/ext/workflow/dapr_workflow_context.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Bernd Verst <github@bernd.dev>
berndverst
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
durabletask.worker uses
orchestrator.__name__herebut when dapr_workflow_context wraps the real workflow function in
wf, the correct orchestrator name is lost.This bug results in the following exception:
when called as
This proposed fix is not pretty, but it works.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #615
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
There aren't any existing tests or even any examples that use call_child_workflow, I'm sorry I do not have time to write tests or documentation at this time.