chore: refactor http-client, http-client-sample-issuer#42
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Summary of ChangesHello @viraatc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant refactor and cleanup of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the HTTP client APIs to be more Pythonic and simplifies the futures-based test client. The changes focus on making the client initialization more automatic and the API more intuitive for Python developers.
Key Changes:
- HTTPEndpointClient now auto-starts on initialization (when loop=None), eliminating manual start/stop calls
- Renamed async methods to follow Python conventions:
async_start→_start_async,async_shutdown→shutdown_async - Simplified FuturesHttpClient to use thread-safe concurrent.futures instead of asyncio futures
- Removed redundant streaming features (first chunk access) from test utilities
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/futures_client.py | Simplified to use concurrent.futures, removed StreamingFuture class and first-chunk access |
| tests/integration/endpoint_client/conftest.py | Updated fixture to use simplified client API |
| tests/integration/endpoint_client/test_http_client_core.py | Updated tests to use concurrent.futures and new auto-start behavior |
| tests/integration/endpoint_client/test_http_client_streaming.py | Removed streaming first-chunk tests, simplified to use concurrent.futures |
| tests/integration/endpoint_client/test_external_serving.py | Updated to use new client API |
| tests/integration/endpoint_client/test_zmq_communication.py | Increased async sleep timeouts for connection stability |
| tests/integration/test_end_to_end_oracle.py | Removed manual start() calls (now auto-starts) |
| tests/performance/endpoint_client/test_http_client_performance_single.py | Removed wait_for_all_complete() call |
| src/inference_endpoint/endpoint_client/http_client.py | Added auto-start logic, renamed async methods, refactored initialization |
| src/inference_endpoint/endpoint_client/http_sample_issuer.py | Simplified response handling, removed wait_for_all_complete() |
| src/inference_endpoint/endpoint_client/worker.py | Improved error message handling for empty exceptions |
| src/inference_endpoint/endpoint_client/zmq_utils.py | Moved uvloop event loop policy setup to zmq_utils |
| src/inference_endpoint/endpoint_client/async_utils.py | Added with_semaphore utility for cleaner concurrency control |
| src/inference_endpoint/endpoint_client/README.md | Updated documentation for renamed methods |
| src/inference_endpoint/commands/probe.py | Updated to use new API methods |
| src/inference_endpoint/commands/benchmark.py | Removed manual start() calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request refactors the HTTPEndpointClient to improve its asynchronous capabilities and simplify its API, particularly for testing and integration. Key changes include renaming async methods (e.g., async_start to start_async, async_shutdown to shutdown_async, get_ready_responses_async to try_recv_response_async), and introducing a with_semaphore utility for concurrency control. The client's __init__ method now supports auto-starting its event loop in a dedicated thread if no loop is provided, or integrating with an existing loop. The HttpClientSampleIssuer was simplified by removing its explicit start() method and n_inflight tracking, with its response handler now initiated directly in its constructor. The FuturesHttpClient test utility was updated to use concurrent.futures.Future for thread-safe futures and now relies on the HTTPEndpointClient's auto-start mechanism, removing explicit async_start() and async_shutdown() calls from tests. Additionally, error message generation in worker.py was enhanced, and the uvloop event loop policy for ZMQ was centralized in zmq_utils.py. Review comments highlighted a confusing comment about _start() vs _start_async() in http_client.py, suggested removing or clarifying a TODO about try_recv_response's synchronous API, and questioned the implicit start behavior of HttpClientSampleIssuer's response handler. A concern was also raised about increasing asyncio.sleep durations in ZMQ tests, suggesting a more robust readiness check instead of arbitrary delays.
1c31437 to
6f1b2f1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nv-alicheng
left a comment
There was a problem hiding this comment.
I remember you asked about removing 'stop_sample_issuer_on_test_end' from BenchmarkSession.start, but it wasn't included in this PR - Was that missed or intentional?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5a50f39 to
bd918e9
Compare
1f88625 to
4cff236
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
included in separate MR : EDIT:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4677f10 to
34ac55b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What does this PR do?
Cleanup of Http-Client APIs:
http_client.start(); cleanuphttp_sample_issuer.start(); cleanupwait_for_all_completefrom http-client (unused, causing overhead in hotpath)Type of change
Related issues
Testing
Checklist