dapr / python-sdk

Dapr SDK for Python
Apache License 2.0
230 stars 128 forks source link

Update client's configuration methods with stable config api #557

Closed hunter007 closed 1 year ago

hunter007 commented 1 year ago

Description

Please explain the changes you've made

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: #556

Checklist

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

shivamkm07 commented 1 year ago

Subscribe and Unsubscribe method for sdk needs to be updated as per sdk-spec(https://github.com/dapr/sig-sdk-spec/blob/main/spec/alpha/configuration.md). It's also discussed here(https://github.com/dapr/python-sdk/issues/488#issuecomment-1427037534). Subscribe/unsubscribe must happen with subscription Id and not keys. I am creating a PR for the same. This PR shouldn't be merged before that.

shivamkm07 commented 1 year ago

Subscribe and Unsubscribe method for sdk needs to be updated as per sdk-spec(https://github.com/dapr/sig-sdk-spec/blob/main/spec/alpha/configuration.md). It's also discussed here(#488 (comment)). Subscribe/unsubscribe must happen with subscription Id and not keys. I am creating a PR for the same. This PR shouldn't be merged before that.

I have created PR https://github.com/dapr/python-sdk/pull/558 with the required change. Only after it is merged, the current PR should be merged. cc @berndverst

berndverst commented 1 year ago

@hunter007 just to double check - did you make sure to use grpcio-tools<1.49 to generate the protos? We must use an older version because there was a breaking change in protobuf and we must stay on the older protobuf version due to compatibility with many ML SDKs.

berndverst commented 1 year ago

@hunter007 we made some changes to the configuration API -- could you please pull the latest changes from the master branch and resolve conflicts, then update this PR?

I'll merge your PR once that is done! Please keep in mind to generate the protos using grpcio-tools 1.48.2 (< 1.49)

Thanks

hunter007 commented 1 year ago

@hunter007 just to double check - did you make sure to use grpcio-tools<1.49 to generate the protos? We must use an older version because there was a breaking change in protobuf and we must stay on the older protobuf version due to compatibility with many ML SDKs.

In my environment:

➜  python-sdk git:(master) pip3 freeze|grep grpcio
grpcio==1.47.0
grpcio-tools==1.47.0
➜  python-sdk git:(master) protoc --version
libprotoc 3.21.1

Is it OK?

berndverst commented 1 year ago

@hunter007 just to double check - did you make sure to use grpcio-tools<1.49 to generate the protos? We must use an older version because there was a breaking change in protobuf and we must stay on the older protobuf version due to compatibility with many ML SDKs.

In my environment:

➜  python-sdk git:(master) pip3 freeze|grep grpcio
grpcio==1.47.0
grpcio-tools==1.47.0
➜  python-sdk git:(master) protoc --version
libprotoc 3.21.1

Is it OK?

Yes that's ok! Thanks for confirming.

codecov[bot] commented 1 year ago

Codecov Report

Merging #557 (e461992) into master (5c83ffb) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #557   +/-   ##
=======================================
  Coverage   89.64%   89.65%           
=======================================
  Files          62       62           
  Lines        2985     2976    -9     
=======================================
- Hits         2676     2668    -8     
+ Misses        309      308    -1     
Impacted Files Coverage Δ
dapr/aio/clients/grpc/client.py 90.87% <100.00%> (+0.16%) :arrow_up:
dapr/clients/grpc/client.py 90.53% <100.00%> (-0.12%) :arrow_down: