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.

Sign up for GitHub

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

Jump to bottom

Client streaming #83

Merged
merged 13 commits into from
Jun 24, 2020
Merged

Client streaming #83

merged 13 commits into from
Jun 24, 2020

Conversation

nat-n
Copy link
Collaborator

@nat-n nat-n commented Jun 7, 2020 •
edited
Loading

Picks up where #45 left off. @hozn

  • Adds full support for stream_unary and stream_stream method types in the generated grpclib based client
  • support independent timing of messages on upload/download streams as demonstrated in the test with the help of an AsyncChannel
  • Also did some work on the generate script, initially just cos tests were failing due to the lack of special handling in case there's a pycache in the inputs, but I might have gotten carried away.
  • Also did a bit of general cleanup of type annotations guided by mypy

@nat-n nat-n mentioned this pull request Jun 7, 2020
Closed
@hozn
Copy link
Contributor

hozn commented Jun 7, 2020

@nat-n -- this looks like a great set of changes. That bi-directional streaming implementation is really slick!

@boukeversteegh boukeversteegh added enhancement New feature or request has test Has a (xfail) test that verifies the bugfix or feature medium Medium effort issue, can fit in a single PR and removed medium Medium effort issue, can fit in a single PR labels Jun 8, 2020
@boukeversteegh boukeversteegh self-assigned this Jun 11, 2020
@nat-n nat-n requested a review from boukeversteegh June 13, 2020 12:06
@nat-n nat-n force-pushed the client-streaming branch 2 times, most recently from caa840e to fa54e6f Compare June 13, 2020 19:58
boukeversteegh
boukeversteegh reviewed Jun 13, 2020
View reviewed changes
Copy link
Collaborator

@boukeversteegh boukeversteegh left a comment

Choose a reason for hiding this comment

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

Awesome work! I only found very minor improvements 🥇 👍

If its too much of a hassle to take out the parallelization of generate.py, we could leave it in and I'll see later if I can correct the output. But it would be great if the path whitelisting could still work as before.

betterproto/grpc/grpclib_client.py Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/grpclib_client.py Show resolved Hide resolved
betterproto/templates/template.py.j2 Show resolved Hide resolved
betterproto/tests/generate.py Show resolved Hide resolved
betterproto/tests/grpc/test_grpclib_client.py Show resolved Hide resolved
betterproto/tests/grpc/test_grpclib_client.py Show resolved Hide resolved
betterproto/tests/grpc/test_grpclib_client.py Show resolved Hide resolved
betterproto/tests/test_inputs.py Show resolved Hide resolved
@nat-n nat-n force-pushed the client-streaming branch from fa54e6f to 57f51cd Compare June 14, 2020 20:17
@nat-n nat-n force-pushed the client-streaming branch from 57f51cd to 3c7eeb6 Compare June 14, 2020 20:29
@nat-n
Finish implementation and testing of client
4b6f55d 
Including stream_unary and stream_stream call methods.

Also
- improve organisation of relevant tests
- fix some generated type annotations
- Add AsyncChannel utility cos it's useful
@nat-n nat-n force-pushed the client-streaming branch from 3c7eeb6 to 9a323bc Compare June 14, 2020 21:23
nat-n added 2 commits June 15, 2020 00:19
@nat-n
Improve generate script
3185c67 
- Fix issue with __pycache__ dirs getting picked up
- parallelise code generation with asyncio for 3x speedup
- silence protoc output unless -v option is supplied
- Use pathlib ;)
@nat-n
Fix most mypy warnings
c8229e5
@nat-n nat-n force-pushed the client-streaming branch from 9a323bc to c8229e5 Compare June 14, 2020 22:20
boukeversteegh
boukeversteegh requested changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@boukeversteegh boukeversteegh left a comment

Choose a reason for hiding this comment

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

Some things are not working yet. I am making a PR with test-cases :-)

betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
betterproto/grpc/util/async_channel.py Outdated Show resolved Hide resolved
@nat-n nat-n force-pushed the client-streaming branch from 50bb67b to d6a7427 Compare June 15, 2020 21:47
@nat-n nat-n force-pushed the client-streaming branch from d6a7427 to e1ccd54 Compare June 15, 2020 22:07
@nat-n nat-n force-pushed the client-streaming branch from c3db325 to 5c9a12e Compare June 16, 2020 17:37
@boukeversteegh boukeversteegh merged commit 9532844 into danielgtaylor:master Jun 24, 2020
@adriangb
Copy link
Contributor

Hi, just curious why parallelizing the generator script was necessary? It seemed fast enough already (it took like 5 seconds on my laptop). Previously I was just using ptvsd, but that doesn't work now since there can't be multiple debug servers on a single port, so it's now much harder to debug. I think this is the PR where the change was implemented.

@nat-n
Copy link
Collaborator Author

nat-n commented Sep 20, 2020

@adament The generator was slow enough to be annoying before. As I recall it this change more than halved the time taken to generate all the test cases though I can't remember the specific times.

What are you trying to debug exactly? the plugin as called by protoc? If you need to be able to run generate with only one concurrent call to protoc then I think it shouldn't be too hard to add an option for that (similar to the -v option). Basically it would just need to switch the two calls to asyncio.gather for a function with the same signature that instead runs the given coroutines in series.

@adriangb
Copy link
Contributor

the generator was slow enough to be annoying before

If it was slow enough to be annoying, then yeah this change is warranted. For me the time it took was totally bearable, but maybe I have a fast CPU 🚤

What are you trying to debug exactly? the plugin as called by protoc?

Yes, precisely.

I think it shouldn't be too hard to add an option for that (similar to the -v option).

Yup that would be nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hozn hozn hozn left review comments

@boukeversteegh boukeversteegh boukeversteegh approved these changes

Assignees

@boukeversteegh boukeversteegh

Labels
enhancement New feature or request has test Has a (xfail) test that verifies the bugfix or feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nat-n @hozn @adriangb @boukeversteegh