Skip to content
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

Changed stubs to use callable instead of def #210

Merged
merged 7 commits into from Feb 15, 2021

Conversation

MHDante
Copy link
Contributor

@MHDante MHDante commented Feb 7, 2021

Changed type generation for grpcio stubs to use the MultiCallable API (see here) . This requires using the grpc-stubs typings for grpcio. This change should allow calling stub methods with common parameters (timeout, metadata, etc.) as well as calling methods on the MultiCallable object (e.g. my_stub.MyRpcMethod.future()).

Fixes #209

@MHDante
Copy link
Contributor Author

MHDante commented Feb 7, 2021

@nipunn1313
Copy link
Owner

looks reasonable to me - but disclaimer, I've never actually used this for grpc.

Would appreciate if you ran the tests and added the generated ".expected" files to the PR - so we can review the different generated code. (Can be easier to see the difference in behavior based on the diff on the generated files)

Notably - want to make sure things are backward compatible (or if they aren't - that we put a disclaimer in our release notes)

If it's looking promising, would also appreciate an entry in CHANGELOG (under upcoming). And you'd be welcome to add yourself in the contributors section (under README) - and make any changes that seem reasonable to you to the GRPC doc under README.

@MHDante
Copy link
Contributor Author

MHDante commented Feb 8, 2021

Hmm. I've run the tests. but it seems to be skipping some of the import mangling.

I don't think this is the result of any of the code I've written/modified. It could be that the expected files were not updated on a previous commit. (or some flag I'm not setting in the test script?)

Please let me know, otherwise, I've updated the changelog and expected files as well.

@nipunn1313
Copy link
Owner

oh interesting - I'll look into it! In theory - the CI testsuite should fail if expected files weren't updated on prior commits, but i agree w/ you - that in practice that doesn't seem to have happened.

We did refactor CI quite a bit in January - so I can double check on that stuff. In any case, thank you for the contribution!

@nipunn1313
Copy link
Owner

this looks good to me! I'll give @Evgenus an opportunity to chime in - but I think that since this is passing the tests that he wrote, this is ok with me!

@nipunn1313
Copy link
Owner

One idea - to make sure we don't regress this in the future -
May want to add some usages in test_grpc_usage.py - which illustrate the use cases which you are enabling (eg a usage w/ timeout parameter).

The testsuite should run those tests as well as run mypy over those tests.

@@ -1,5 +1,7 @@
## Upcoming

- Changed type generation for `grpcio` stubs to use the `MultiCallable` API ([see here](https://grpc.github.io/grpc/python/grpc.html#multi-callable-interfaces)) . This requires using the `grpc-stubs` typings for grpcio. This change should allow calling stub methods with common parameters (`timeout`, `metadata`, etc.) as well as calling methods on the `MultiCallable` object (e.g. `my_stub.MyRpcMethod.future()`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nipunn1313 are you ok that grpc-stubs will be a strict dependency? BTW, this need to be specified as requirement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.. I've forgot that it already is a dependency. Ok let's wait until we can use official master.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this!

@Evgenus
Copy link
Contributor

Evgenus commented Feb 8, 2021

this looks good to me! I'll give @Evgenus an opportunity to chime in - but I think that since this is passing the tests that he wrote, this is ok with me!

This is great. I will take a look on how it works with real code in pycharm. Also I would like to review tests, will do it soon.

@Evgenus
Copy link
Contributor

Evgenus commented Feb 8, 2021

@MHDante Thank you. Great job. I will review shortly.

@MHDante MHDante marked this pull request as ready for review February 9, 2021 16:43
@MHDante
Copy link
Contributor Author

MHDante commented Feb 13, 2021

I added some tests. LGTM?

@nipunn1313
Copy link
Owner

Nice tests! Awesome stuff!

@nipunn1313 nipunn1313 merged commit 40cf157 into nipunn1313:master Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use UnaryUnaryMultiCallable (and others) instead of function definition in Stubs
3 participants