Adding basic support (untested) for client streaming #45

hozn · 2020-05-18T01:34:27Z

I haven't written tests for this, but added the base methods for stream_unary and stream_stream support. I realize this presents a bit of a challenge when it comes to API consistency since the attrs of the request can't be exploded out into kwargs like the unary_unary and unary_stream methods. I went the simple route and just added request_iterator param to these methods.

Let me know if changes needed, etc.

nat-n

I suspect some tests will be required for this.

Does it make sense to add these without solving the problem of how to expose it to the end user?

nat-n · 2020-05-18T07:43:54Z

betterproto/templates/template.py

    {% for method in service.methods %}
-    async def {{ method.py_name }}(self{% if method.input_message and method.input_message.properties %}, *, {% for field in method.input_message.properties %}{{ field.name }}: {% if field.zero == "None" and not field.type.startswith("Optional[") %}Optional[{{ field.type }}]{% else %}{{ field.type }}{% endif %} = {{ field.zero }}{% if not loop.last %}, {% endif %}{% endfor %}{% endif %}) -> {% if method.server_streaming %}AsyncGenerator[{{ method.output }}, None]{% else %}{{ method.output }}{% endif %}:
+    async def {{ method.py_name }}(self
+        {%- if not method.client_streaming -%}


How does the templating engine handle all this extra whitespace?

I suspect some tests will be required for this.

Yeah, I didn't see a precedent for testing the generated services, but I can look at adding these. I figured I'd get this out here for discussion before investing that time, in case this wasn't the direction you wanted to go.

Does it make sense to add these without solving the problem of how to expose it to the end user?

Well, I don't have a better idea than letting the caller provider an iterator of the request objects. It doesn't match the other methods, but I'm not sure how one would match those unary_* methods. I suppose another direction could be a method that feeds into an iterator under the hood (wrapped within the method), but I don't know how you then signal the end of the method call, etc. The request_iterator feels clean and simple and certainly works for my own use cases.

How does the templating engine handle all this extra whitespace?

The extra whitespace is mitigated by the {%- stripping tag as well as the params to the jinja environment. Finally, running everything through black in the end also removes any lingering artifacts, so the output is fine / matches current [albeit with the new request_iterator when appropriate]..

I guess using the request_iterator argument as you describe is probably the most straightforward approach. Could also do it with a two way generator or a context provider, but neither of these is as simple to use.

I also have a PR open where I add a service test: c762c9c And I think an example usage in the readme would also be necessary.

~~That said, I'm not a designated maintainer so can't promise anything about what will be acceptable to merge.~~

@nat-n , thanks for that link to the test; that certainly looks less ceremonious than the testing setup I was seeing in grpclib itself. Thanks!

betterproto/templates/template.py

betterproto/__init__.py

cetanu · 2020-05-21T06:57:52Z

If we can get a test for this I think it's a worthwhile addition. The jinja seems sane to me, but I would be particularly interested to remove all concern for whitespace weirdness, I know that the hyphens do some trimming but with an indent sensitive language like python it can be interesting.

If you already know how to add a test for this then go ahead, otherwise I (or we) will have to dig around a bit

cetanu · 2020-05-21T07:10:09Z

Also, can I get you to pull in the changes from #34 ?
It's a small change that hits the same lines as yours

nat-n

Please note the recent changes to the ServiceStub class: 3546f55#diff-433ccf9072e57d9dc251fbcb3972c153R1040

You'll need to rebase and tweak the two new methods to be consistent.

nat-n · 2020-05-22T11:06:52Z

betterproto/__init__.py

+            route, grpclib.const.Cardinality.STREAM_STREAM, request_type, response_type
+        ) as stream:
+            for message in request_iterator:
+                await stream.send_message(message)


It can't be assumed that the client will want to send all the messages before receiving any.

Ideally I think this method should:

accept request_iterator: Union[Iterator["IProtoMessage"], AsyncIterator["IProtoMessage", None]]

Return type AsyncIterator[T], since it doesn't actually accept data being sent in

if Iterator["IProtoMessage"] is given then wrap it in an AsyncIterator to proceed

then iterate to receive data from the caller and send it to the server and to receive the data from he server and yield it concurrently, thus allowing upload/download to occur in any valid order.

Or alternatively it might more elegant for this method to just return an AsyncGenerator, and then it's up to the caller to manage sending and receiving from that generator. This would imply an update to the _unary_stream method for consistency, though it should be possible to keep is backwards compatible. I've not totally thought this through but I think it's worth exploring.

Sorry if you were hoping this would be simpler :P I'd be happy to help iterate towards a better solution.

Good suggestions; I'll incorporate these.

hozn · 2020-05-22T16:09:49Z

Thanks @nat-n and others! I'll work to update the PR, though I should say that this has been degraded in my priorities, as I've rewritten the compiler portion to match our idiosyncratic protobuf/grpc needs a little more closely. (We predominantly use the threaded grpcio classes, so I needed generated client/server code to work with those. We also require google.protobuf.Any support, so I implemented that.). I am hoping that most of my local changes can be rebased off upstream in future.

nat-n · 2020-05-23T14:46:40Z

@hozn I see this as a high priority issue, with a few more problems to solve, do you mind if I work on it a bit from here?

hozn · 2020-05-23T16:13:03Z

@nat-n -- I don't mind at all; I would appreciate it greatly! I'm eager to see the implementation for concurrently consuming from the request iterator (note, perhaps better would be to have that be an Iterable[IProtoMesage] -- feels like a list should be valid input?) and emitting to the response stream. I bookmarked some notes on how to combine the iterators like that, but it sounds like you probably had a clear vision there.

nat-n · 2020-05-23T16:16:02Z

@hozn

Sure thing. Since you've already spent some time thinking about it, if you have more thoughts to share or could spare some time to review related changes then that would be appreciated.

hozn · 2020-05-23T16:23:30Z

@nat-n , sounds great; yes, I'll continue to participate; I'm sure I'll learn a lot. I appreciate you taking this on, and apologize that I didn't get it to a more complete stage (w/ tests).

I'll also be happy to share the work that I've done with wrapping the default (grpio-based) client/server code. I'm not sure that it's worthy of a pull request, as it's really ugly and uses protected members from that package (I don't really see a practical way around that), but it may be worth separating out the services from the generated messages. In my fork of the compiler code, I ended up creating a structure like:

nspkg/
  __init__.py  # <-- namespace pkg
  module/
    __init__.py  # <-- message classes
    services.py  # <-- grpcio-baesd (threaded) services+stubs
    services_aio.py # <-- grpclib-based (asyncio) services+stubs

Unrelated to this, I also am testing out an implementation of google.protobuf.Any; once I've spent more time figuring out the edge cases there, I'll write that up as a pull request/discussion.

nat-n · 2020-05-23T20:55:35Z

@hozn That's sounds similar to what I had in mind. I also have a PoC for grpcio integration, though I think this problem is important and complex enough that it's worth exploring different approaches in order to get it right. If you push what you have to your fork I'll have a look.

Also if you have any ideas about the plugin CLI for supporting grpcio + grpclib please share: #56

nat-n · 2020-06-07T17:17:09Z

Closing in favour of #83

Adding basic support (untested) for client streaming

fced8ef

nat-n reviewed May 18, 2020

View reviewed changes

hozn added 4 commits May 18, 2020 08:54

Removing unused Sequence import.

92650c1

Running black.

21fdb1b

Fixing comments on the new service methods.

2d0b873

Cleanup if/then formatting of the template.

d77c1b8

cetanu self-assigned this May 21, 2020

cetanu added the enhancement New feature or request label May 21, 2020

nat-n requested changes May 22, 2020

View reviewed changes

nat-n self-assigned this May 23, 2020

nat-n marked this pull request as draft May 24, 2020 17:02

nat-n mentioned this pull request Jun 7, 2020

Client streaming #83

Merged

nat-n closed this Jun 7, 2020

Adding basic support (untested) for client streaming #45

Adding basic support (untested) for client streaming #45

Uh oh!

Conversation

hozn commented May 18, 2020

Uh oh!

nat-n left a comment

Choose a reason for hiding this comment

Uh oh!

nat-n May 18, 2020

Choose a reason for hiding this comment

Uh oh!

hozn May 18, 2020

Choose a reason for hiding this comment

Uh oh!

nat-n May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hozn May 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cetanu commented May 21, 2020

Uh oh!

cetanu commented May 21, 2020

Uh oh!

nat-n left a comment

Choose a reason for hiding this comment

Uh oh!

nat-n May 22, 2020

Choose a reason for hiding this comment

Uh oh!

hozn May 22, 2020

Choose a reason for hiding this comment

Uh oh!

hozn commented May 22, 2020

Uh oh!

nat-n commented May 23, 2020

Uh oh!

hozn commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nat-n commented May 23, 2020

Uh oh!

hozn commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nat-n commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nat-n commented Jun 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nat-n May 18, 2020 •

edited

Loading

hozn commented May 23, 2020 •

edited

Loading

hozn commented May 23, 2020 •

edited

Loading

nat-n commented May 23, 2020 •

edited

Loading