-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,14 @@ | |
| _client_py_path = str(_current_dir / "client.py") | ||
| _config_path = str(_current_dir / "config.yaml") | ||
|
|
||
| _skipped_tests = [ | ||
| # Not implemented yet, | ||
| _skipped_tests_sync = [ | ||
| # Need to use async APIs for proper cancellation support in Python. | ||
| "--skip", | ||
| "Client Cancellation/**", | ||
| ] | ||
|
|
||
| _httpx_opts = [ | ||
| # Trailers not supported | ||
| "--skip", | ||
| "**/Protocol:PROTOCOL_GRPC/**", | ||
| "--skip", | ||
|
|
@@ -24,20 +30,28 @@ | |
| "gRPC Empty Responses/**", | ||
| "--skip", | ||
| "gRPC Proto Sub-Format Responses/**", | ||
| ] | ||
|
|
||
| _skipped_tests_sync = [ | ||
| *_skipped_tests, | ||
| # Need to use async APIs for proper cancellation support in Python. | ||
| # Bidirectional streaming not supported | ||
| "--skip", | ||
| "**/full-duplex/**", | ||
| # Cancellation delivery isn't reliable | ||
| "--known-flaky", | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It'd be nice to spike out on replacing httpx w/ pyqwest and see how it behaves (better? worse?).
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 |
||
| "Client Cancellation/**", | ||
| "--known-flaky", | ||
| "Timeouts/**", | ||
| ] | ||
|
|
||
|
|
||
| def test_client_sync() -> None: | ||
| @pytest.mark.parametrize("client", ["httpx", "pyqwest"]) | ||
| def test_client_sync(client: str) -> None: | ||
| args = maybe_patch_args_with_debug( | ||
| [sys.executable, _client_py_path, "--mode", "sync"] | ||
| [sys.executable, _client_py_path, "--mode", "sync", "--client", client] | ||
| ) | ||
|
|
||
| opts = [] | ||
| match client: | ||
| case "httpx": | ||
| opts = _httpx_opts | ||
|
|
||
| result = subprocess.run( | ||
| [ | ||
| "go", | ||
|
|
@@ -47,6 +61,7 @@ def test_client_sync() -> None: | |
| _config_path, | ||
| "--mode", | ||
| "client", | ||
| *opts, | ||
| *_skipped_tests_sync, | ||
| "--", | ||
| *args, | ||
|
|
@@ -59,18 +74,17 @@ def test_client_sync() -> None: | |
| pytest.fail(f"\n{result.stdout}\n{result.stderr}") | ||
|
|
||
|
|
||
| _skipped_tests_async = [ | ||
| *_skipped_tests, | ||
| # Cancellation currently not working for full duplex | ||
| "--skip", | ||
| "Client Cancellation/**/full-duplex/**", | ||
| ] | ||
|
|
||
|
|
||
| def test_client_async() -> None: | ||
| @pytest.mark.parametrize("client", ["httpx", "pyqwest"]) | ||
| def test_client_async(client: str) -> None: | ||
| args = maybe_patch_args_with_debug( | ||
| [sys.executable, _client_py_path, "--mode", "async"] | ||
| [sys.executable, _client_py_path, "--mode", "async", "--client", client] | ||
| ) | ||
|
|
||
| opts = [] | ||
| match client: | ||
| case "httpx": | ||
| opts = _httpx_opts | ||
|
|
||
| result = subprocess.run( | ||
| [ | ||
| "go", | ||
|
|
@@ -80,9 +94,7 @@ def test_client_async() -> None: | |
| _config_path, | ||
| "--mode", | ||
| "client", | ||
| *_skipped_tests_async, | ||
| "--known-flaky", | ||
| "Client Cancellation/**", | ||
| *opts, | ||
| "--", | ||
| *args, | ||
| ], | ||
|
|
||
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