dapr / python-sdk

Dapr SDK for Python
Apache License 2.0
223 stars 125 forks source link

Workflow management python SDK #554

Closed RyanLettieri closed 1 year ago

RyanLettieri commented 1 year ago

Description

This PR covers the implementation of the management portion for workflow.

Note that this PR will fail until the following PRs are submitted: https://github.com/dapr/dapr/pull/6163 https://github.com/dapr/components-contrib/pull/2729

Issue reference

Please reference the issue this PR will close: #542

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

berndverst commented 1 year ago
dapr/clients/grpc/_response.py:963: error: Too many arguments for "__init__" of "object"  [call-arg]
dapr/clients/grpc/_response.py:989: error: Too many arguments for "__init__" of "object"  [call-arg]
codecov[bot] commented 1 year ago

Codecov Report

Merging #554 (04be191) into master (9e1aa17) will decrease coverage by 2.47%. The diff coverage is 49.06%.

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
- Coverage   89.60%   87.14%   -2.47%     
==========================================
  Files          73       73              
  Lines        3319     3531     +212     
==========================================
+ Hits         2974     3077     +103     
- Misses        345      454     +109     
Impacted Files Coverage Δ
dapr/aio/clients/grpc/client.py 70.50% <13.82%> (-20.37%) :arrow_down:
dapr/clients/grpc/client.py 85.41% <72.34%> (-5.12%) :arrow_down:
dapr/clients/grpc/_helpers.py 89.85% <85.71%> (-1.06%) :arrow_down:
dapr/clients/grpc/_response.py 87.57% <100.00%> (+0.43%) :arrow_up:
berndverst commented 1 year ago

It seems you should have made workflow_options an Optional type and set the default value to None.

berndverst commented 1 year ago

Please integrate your example into the demo_workflow folder from @DeepanshuA's PR. There should not be two different examples. So let's combine the authoring with the management in a single example please!

cgillum commented 1 year ago

@berndverst please reconsider regarding support for Any and implicit serialization for the workflow management APIs.

The workflow authoring SDKs already support implicit deserialization of JSON data. In fact, they do not support reading the binary data directly, in spite of the fact that the proto contract defines these payloads as binary. Disallowing implicit serialization in these APIs would mean that every developer would have to write JSON serialization code in their client apps, which is a bunch of boilerplate that they might get wrong and file issues for. The Dapr Client SDKs will be much more usable if we can take care of the serialization for them equally on both sides.

FWIW, we do the implicit client-side serialization in the .NET Dapr SDK already. Having the same behavior in the Python SDK would be nice for consistency.