Repository navigation
feat(pyamber): reject invalid port booleans - #8240
Conversation
Automated Reviewer SuggestionsBased on the
|
Backport auto-label reportThis
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8240 +/- ##
=========================================
Coverage 92.30% 92.31%
Complexity 5046 5046
=========================================
Files 1260 1260
Lines 53887 53890 +3
Branches 6711 6713 +2
=========================================
+ Hits 49743 49746 +3
Misses 2486 2486
Partials 1658 1658
*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:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 381 | 0.233 | 26,332/31,475/31,475 us | 🔴 +14.2% / 🔴 +117.1% |
| 🔴 | bs=100 sw=10 sl=64 | 783 | 0.478 | 124,478/172,690/172,690 us | 🔴 +22.5% / 🔴 +65.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 910 | 0.555 | 1,102,636/1,213,551/1,213,551 us | ⚪ within ±5% / 🔴 +23.1% |
Baseline details
Latest main 7426431 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 381 tuples/sec | 425 tuples/sec | 822.11 tuples/sec | -10.4% | -53.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.233 MB/s | 0.26 MB/s | 0.502 MB/s | -10.4% | -53.6% |
| bs=10 sw=10 sl=64 | p50 | 26,332 us | 23,058 us | 12,129 us | +14.2% | +117.1% |
| bs=10 sw=10 sl=64 | p95 | 31,475 us | 35,376 us | 15,469 us | -11.0% | +103.5% |
| bs=10 sw=10 sl=64 | p99 | 31,475 us | 35,376 us | 18,357 us | -11.0% | +71.5% |
| bs=100 sw=10 sl=64 | throughput | 783 tuples/sec | 826 tuples/sec | 1,058 tuples/sec | -5.2% | -26.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.478 MB/s | 0.504 MB/s | 0.646 MB/s | -5.2% | -26.0% |
| bs=100 sw=10 sl=64 | p50 | 124,478 us | 120,415 us | 97,399 us | +3.4% | +27.8% |
| bs=100 sw=10 sl=64 | p95 | 172,690 us | 140,979 us | 104,184 us | +22.5% | +65.8% |
| bs=100 sw=10 sl=64 | p99 | 172,690 us | 140,979 us | 117,615 us | +22.5% | +46.8% |
| bs=1000 sw=10 sl=64 | throughput | 910 tuples/sec | 916 tuples/sec | 1,099 tuples/sec | -0.7% | -17.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.559 MB/s | 0.671 MB/s | -0.7% | -17.3% |
| bs=1000 sw=10 sl=64 | p50 | 1,102,636 us | 1,086,650 us | 946,743 us | +1.5% | +16.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,213,551 us | 1,272,312 us | 985,437 us | -4.6% | +23.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,213,551 us | 1,272,312 us | 1,012,622 us | -4.6% | +19.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,524.72,200,128000,381,0.233,26331.54,31475.46,31475.46
1,100,10,64,20,2554.34,2000,1280000,783,0.478,124477.65,172690.17,172690.17
2,1000,10,64,20,21985.43,20000,12800000,910,0.555,1102635.87,1213551.29,1213551.29
|
I am converting this PR to a draft: please follow our PR template and mark it ready for review. |
|
The description now follows the current template, and this is ready for review. |
carloea2
changed the title
|
/request-review @mengw15 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR updates the Python GlobalPortIdentity decoder to reject invalid boolean tokens for isInternal and isInput instead of implicitly treating them as false, and adds tests to enforce the new behavior.
Changes:
- Add explicit validation for
isInternal/isInputtokens to only allow case-insensitive"true"/"false". - Add a parametrized test ensuring invalid boolean values raise
ValueErrorwith a helpful message.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| amber/src/main/python/core/util/virtual_identity.py | Validates boolean tokens and raises on invalid values during deserialization. |
| amber/src/test/python/core/util/test_virtual_identity.py | Adds coverage to ensure invalid boolean strings are rejected for both fields. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@mengw15 Thanks for the review. Required checks are passing. Could you merge this when you have a chance? |
|
@mengw15, this is approved with green checks. Could you change the stale fix label to feature and merge it when you have time? |
Head branch was pushed to by a user without write access
2d38e7b to
94a7e74
Compare
|
@mengw15, #8240 is approved and its refreshed checks pass. The branch labeler restored fix on these feature PRs: #8240, #8242, #8254, #8258, #8142 and #8144. Could you switch those labels to feature? #8142 and #8144 still await the aggregate check. Please merge the approved PRs once their required checks pass. |
|
@mengw15, CI is green after the rebase. Could you take a final look and merge when ready? Please change the stale fix label to feature before merging. |
What changes were proposed in this PR?
The Python GlobalPortIdentity decoder now rejects invalid boolean tokens instead of silently treating them as false. Valid mixed-case true and false values remain supported.
Any related issues, documentation, discussions?
Closes #8239
How was this PR tested?
Retested with Python 3.12 on Windows in a fresh detached worktree from upstream main at
7426431337, which includes the merged loop-completion change #8938. Regression before implementation: 2 failed, 48 passed. After implementation: 50 passed.Generate protobufs with
bash bin/python-proto-gen.sh, then run fromamber:Ruff lint and formatting pass for all 217 Python files.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex