-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for gRPC client protocol #80
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
Conversation
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
| # Need to use async APIs for proper cancellation support in Python. | ||
| # Bidirectional streaming not supported | ||
| "--skip", | ||
| "**/full-duplex/**", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My conformance test implementation for full-duplex was wrong, allowing httpx to pass it. I fixed it and now httpx correctly fails it for not supporting bidirectional streaming
| "--skip", | ||
| "**/full-duplex/**", | ||
| # Cancellation delivery isn't reliable | ||
| "--known-flaky", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new client seems to perform just fine for cancellation and timeouts so far. This does reduce my motivation to chase the issue in httpx upstream.
Given it also enables bidirectional streaming, I guess we'll need to consider whether pyqwest should be the default client, but being so new definitely nervous about it, happy to hear any thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it would be nice to consolidate on a single client, although it's tricky; at this point httpx is pretty battle-tested, but I would assume it'll basically never allow us to support bidi streams. I don't have a great sense on the underlying reqwest library, although being similarly pre-1.0 and with a lot of open PRs / issues, not sure how much more API churn we'll see? I'd be curious to get more of your take on the maintenance health of that library and how far it is from a stable release. Still, I've seen it used in a number of places so I imagine it's pretty solid.
It'd be nice to spike out on replacing httpx w/ pyqwest and see how it behaves (better? worse?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions - in terms of pre-1.0, I probably already got used to the standard Rust practice of many production libraries seemingly stuck at 0.x forever ;) I have been checking that repo extensively lately and my impression is it is well maintained, the open PRs are definitely a cause for pause, but I think there is active work as well on higher priority issues. It also seems widely used, with even Cargo optionally using it, with the default being curl, not even a rust project. So it seems like the rust "standard library" http client and a trustworthy dependency.
In terms of impact on connect-python of API changes, anyways those are controlled by pyqwest (me). I am pretty confident in its client API and don't expect much churn especially in raw usage of bytestreams.
It'd be nice to spike out on replacing httpx w/ pyqwest and see how it behaves (better? worse?).
Cool, yeah I think it will be worth seeing this. This will have to come after implementing ASGI / WSGI transports like httpx has which we use extensively, after that will give it a shot
| from google.protobuf.message import Message | ||
| from pyqwest import HTTPTransport, SyncHTTPTransport | ||
| from pyqwest import HTTPVersion as PyQwestHTTPVersion | ||
| from pyqwest.httpx import AsyncPyqwestTransport, PyqwestTransport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, just slots in using this httpx-compatible wrapper
stefanvanburen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| "--skip", | ||
| "**/full-duplex/**", | ||
| # Cancellation delivery isn't reliable | ||
| "--known-flaky", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it would be nice to consolidate on a single client, although it's tricky; at this point httpx is pretty battle-tested, but I would assume it'll basically never allow us to support bidi streams. I don't have a great sense on the underlying reqwest library, although being similarly pre-1.0 and with a lot of open PRs / issues, not sure how much more API churn we'll see? I'd be curious to get more of your take on the maintenance health of that library and how far it is from a stable release. Still, I've seen it used in a number of places so I imagine it's pretty solid.
It'd be nice to spike out on replacing httpx w/ pyqwest and see how it behaves (better? worse?).
This is based on pyqwest an HTTP client I released that supports bidirectional streaming and trailers. The approach is similar to server side, moving some pieces into a
ClientProtocolabstraction and writing gRPC versions.#49