test(pyamber): add unit tests for Python AsyncRPCServer#6492
Open
eugenegujing wants to merge 1 commit into
Open
test(pyamber): add unit tests for Python AsyncRPCServer#6492eugenegujing wants to merge 1 commit into
eugenegujing wants to merge 1 commit into
Conversation
Add the first direct unit tests for amber/src/main/python/core/architecture/rpc/async_rpc_server.py, previously only reached indirectly through MainLoop tests. The 26 tests cover: handler-map construction (lowercased method names), the synthetic stream wrapper, success dispatch (command_id round-trip, unwrapped command, case-insensitive lookup), error wrapping into ControlError with ErrorLanguage.PYTHON (raising handler and unknown method), no-reply suppression for negative command ids, and reply routing (context-based with fallback to swapping the incoming channel). Also pins two surprising current behaviors as characterization tests: a handler that returns no result (or a wrong-typed one) silently yields an empty ControlReturn, and unknown-method error messages carry str(KeyError) quotes.
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6492 +/- ##
============================================
+ Coverage 70.28% 70.31% +0.03%
Complexity 3397 3397
============================================
Files 1142 1142
Lines 44844 44844
Branches 4948 4948
============================================
+ Hits 31517 31533 +16
+ Misses 11682 11666 -16
Partials 1645 1645
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 378 | 0.231 | 25,880/31,565/31,565 us | 🔴 +17.2% / 🔴 +119.8% |
| 🟢 | bs=100 sw=10 sl=64 | 798 | 0.487 | 125,439/137,098/137,098 us | 🟢 -8.5% / 🔴 +38.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 914 | 0.558 | 1,095,829/1,122,216/1,122,216 us | ⚪ within ±5% / 🔴 +20.4% |
Baseline details
Latest main f722c26 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 378 tuples/sec | 407 tuples/sec | 831.47 tuples/sec | -7.1% | -54.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.231 MB/s | 0.249 MB/s | 0.507 MB/s | -7.2% | -54.5% |
| bs=10 sw=10 sl=64 | p50 | 25,880 us | 22,075 us | 11,844 us | +17.2% | +118.5% |
| bs=10 sw=10 sl=64 | p95 | 31,565 us | 35,454 us | 14,359 us | -11.0% | +119.8% |
| bs=10 sw=10 sl=64 | p99 | 31,565 us | 35,454 us | 18,888 us | -11.0% | +67.1% |
| bs=100 sw=10 sl=64 | throughput | 798 tuples/sec | 809 tuples/sec | 1,089 tuples/sec | -1.4% | -26.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.487 MB/s | 0.494 MB/s | 0.665 MB/s | -1.4% | -26.7% |
| bs=100 sw=10 sl=64 | p50 | 125,439 us | 121,919 us | 92,503 us | +2.9% | +35.6% |
| bs=100 sw=10 sl=64 | p95 | 137,098 us | 149,831 us | 99,121 us | -8.5% | +38.3% |
| bs=100 sw=10 sl=64 | p99 | 137,098 us | 149,831 us | 112,422 us | -8.5% | +21.9% |
| bs=1000 sw=10 sl=64 | throughput | 914 tuples/sec | 919 tuples/sec | 1,117 tuples/sec | -0.5% | -18.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.558 MB/s | 0.561 MB/s | 0.682 MB/s | -0.5% | -18.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,095,829 us | 1,091,889 us | 910,046 us | +0.4% | +20.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,122,216 us | 1,114,478 us | 950,716 us | +0.7% | +18.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,122,216 us | 1,114,478 us | 985,832 us | +0.7% | +13.8% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,529.03,200,128000,378,0.231,25879.56,31565.06,31565.06
1,100,10,64,20,2505.23,2000,1280000,798,0.487,125439.12,137097.54,137097.54
2,1000,10,64,20,21884.60,20000,12800000,914,0.558,1095829.23,1122216.08,1122216.08
Contributor
There was a problem hiding this comment.
Pull request overview
Adds unit-test coverage for the Python worker’s AsyncRPCServer control-message dispatch logic by introducing a focused test suite under amber/src/test/python. This improves confidence in RPC handler dispatch, error wrapping, fire-and-forget semantics, and reply routing without needing to spin up the full worker context.
Changes:
- Added a new
test_async_rpc_server.pysuite exercising handler-map normalization, stream wrapping, success/error paths, no-reply behavior, and reply routing. - Uses a stubbed
AsyncRPCHandlerInitializerfor__init__coverage and a__new__-constructed server forreceive()-path unit tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+53
| def _make_handler(func) -> grpclib.const.Handler: | ||
| """Builds a real grpclib Handler namedtuple around an async callable. | ||
|
|
||
| AsyncRPCServer only ever touches `.func`, but using the real namedtuple | ||
| keeps the stub shape identical to what AsyncRPCHandlerInitializer yields. | ||
| """ | ||
| return grpclib.const.Handler( | ||
| func, Cardinality.UNARY_UNARY, ControlRequest, ControlReturn | ||
| ) |
Comment on lines
+69
to
+71
| def _make_invocation( | ||
| method_name: str, command_id: int, context: AsyncRpcContext = None | ||
| ) -> ControlInvocation: |
eugenegujing
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Adds
test_async_rpc_server.py(new, 26 tests) for the Python worker'sAsyncRPCServer(amber/src/main/python/core/architecture/rpc/async_rpc_server.py).What the tests cover:
None.ControlErrormarkedErrorLanguage.PYTHON(including when the error has no message).Any related issues, documentation, discussions?
Closes #6485
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Co-authored by: Claude Code (Fable 5)