feat(pyamber): honor output port lookup - #8276
Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8276 +/- ##
=========================================
Coverage 94.03% 94.03%
Complexity 4821 4821
=========================================
Files 1204 1204
Lines 48991 48993 +2
Branches 5956 5957 +1
=========================================
+ Hits 46067 46069 +2
Misses 1458 1458
Partials 1466 1466
*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 | 374 | 0.229 | 25,687/32,351/32,351 us | 🔴 +5.8% / 🔴 +98.1% |
| 🔴 | bs=100 sw=10 sl=64 | 784 | 0.478 | 126,228/155,454/155,454 us | 🔴 +12.9% / 🔴 +40.9% |
| ⚪ | bs=1000 sw=10 sl=64 | 909 | 0.555 | 1,088,724/1,186,140/1,186,140 us | ⚪ within ±5% / 🔴 +12.7% |
Baseline details
Latest main 1cbe857 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 374 tuples/sec | 393 tuples/sec | 749.39 tuples/sec | -4.8% | -50.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.229 MB/s | 0.24 MB/s | 0.457 MB/s | -4.6% | -49.9% |
| bs=10 sw=10 sl=64 | p50 | 25,687 us | 24,268 us | 13,045 us | +5.8% | +96.9% |
| bs=10 sw=10 sl=64 | p95 | 32,351 us | 33,067 us | 16,328 us | -2.2% | +98.1% |
| bs=10 sw=10 sl=64 | p99 | 32,351 us | 33,067 us | 19,518 us | -2.2% | +65.7% |
| bs=100 sw=10 sl=64 | throughput | 784 tuples/sec | 835 tuples/sec | 968.07 tuples/sec | -6.1% | -19.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.478 MB/s | 0.51 MB/s | 0.591 MB/s | -6.3% | -19.1% |
| bs=100 sw=10 sl=64 | p50 | 126,228 us | 117,048 us | 103,330 us | +7.8% | +22.2% |
| bs=100 sw=10 sl=64 | p95 | 155,454 us | 137,731 us | 110,305 us | +12.9% | +40.9% |
| bs=100 sw=10 sl=64 | p99 | 155,454 us | 137,731 us | 121,264 us | +12.9% | +28.2% |
| bs=1000 sw=10 sl=64 | throughput | 909 tuples/sec | 923 tuples/sec | 998 tuples/sec | -1.5% | -8.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.563 MB/s | 0.609 MB/s | -1.4% | -8.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,088,724 us | 1,086,971 us | 1,009,354 us | +0.2% | +7.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,186,140 us | 1,160,112 us | 1,052,748 us | +2.2% | +12.7% |
| bs=1000 sw=10 sl=64 | p99 | 1,186,140 us | 1,160,112 us | 1,081,982 us | +2.2% | +9.6% |
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,534.17,200,128000,374,0.229,25686.85,32351.00,32351.00
1,100,10,64,20,2551.30,2000,1280000,784,0.478,126227.55,155454.48,155454.48
2,1000,10,64,20,22011.87,20000,12800000,909,0.555,1088723.94,1186139.91,1186139.91
carloea2
changed the title
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 0 must-fix · 4 advisory · 0 polish — the change is correct; the notes are about an exception type, a test's anchor, a missing annotation, and the linked issue's claim.
Correctness (1)
output_manager.py:173— the empty-_portsfailure changes fromIndexErrorto a message-lessStopIteration, which PEP 479 turns intoRuntimeErrorin the emit path (advisory, see inline)
Simplifications (2)
test_output_manager.py:674— the positive case asserts against the private_portsdict rather than the observable schema (advisory, see inline)output_manager.py:173—port_idis unannotated and undocumented although it now carries a two-mode contract (advisory, see inline)
Conventions (1)
- Note in the description that #8274's stated impact is not reachable today — the change is right, the issue's framing is what needs the correction (advisory, detail in the trace)
Verification trace
Checked whether the new exception shape is reachable before deciding severity. is_missing_output_ports documents empty _ports as a legitimate state, so the branch is not dead by construction — but no production caller reaches get_port() there, which is why this is advisory rather than blocking. The downgrade is recorded rather than assumed.
On #8274: no caller passes a port id, and PythonUDFOpDescV2.propagateSchema keys its one schema to outputPorts.head.id, so the multi-port mismatch it describes cannot occur at this head.
Also checked the feat type, which looks wrong at first glance — the branch is fix/… and a bot comment calls it a fix: PR. It is correct under the repo's own rule: CONTRIBUTING.md:71 says implementing support that never worked is a feat. Not raised as a retitle.
The release/v1.2 backport label and its failing preflight are stale artifacts of an earlier head; the current head reports backport: skipped.
990e4b5 to
f06d526
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 4 resolved · 0 open · 1 new (1 new = 1 newly introduced · 0 late catches)
All four items from the last round are genuinely fixed — verified against the tree, not taken from the replies. One new note on a test added since.
Simplifications (1)
test_output_manager.py:678— the no-id test registers one port, so it cannot observe "first" (advisory, see inline)
Verification trace
Re-checked each of the three threads you resolved rather than accepting the replies. output_manager.py:174 is back to list(self._ports.values())[0], so the empty-_ports failure is IndexError again; I ran list({}.values())[0] to confirm CPython's message is literally list index out of range, which is what the new test at :684 matches on, so that regex pins the real message rather than a guessed one. :676 now asserts through get_port(port_b).get_schema(), and :171-172 carry the annotation and docstring.
Also compared against the Scala twin of this manager: OutputManager.scala:286 is the same required-id hard-failing lookup the explicit branch now matches. Scala keeps the no-id case as a separate guarded method (getSingleOutputPortIdentity, :312-315, asserting ports.size == 1). Keeping it a default here is right — three call sites depend on it, and adopting the assertion would break a genuine multi-output Python UDF rather than fix one.
f06d526 to
1d88baf
Compare
|
@Yicong-Huang All review comments are addressed and checks are green. Could you take another look? |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 1 resolved · 0 open · 0 new (0 new = 0 newly introduced · 0 late catches)
The last round's item is genuinely fixed, and a full re-read of both files turned up nothing new. Nothing blocking from my side.
Verification trace
I checked the fixture change by mutation rather than by taking the reply at face value. test_get_port_without_an_id_uses_the_first_port now inserts PortIdentity(id=0) -> schema_a and a second mock behind it (test_output_manager.py:680-682), so an implementation spelled list(self._ports.values())[-1] returns the second mock and the identity assertion fails — "first" is now distinguishable from "only".
The rest of the tree was re-verified at this head rather than carried over. output_manager.py:174 is still the merge target's own list(self._ports.values())[0], so the empty-_ports failure stays IndexError: list index out of range, pinned at :684-686. The explicit-id branch (:175) matches the Scala twin exactly (OutputManager.scala:286, a required-id map lookup that hard-fails). And the description's reachability caveat still holds: grep -rn "get_port(" amber/src/main/python returns only data_processor.py:130, output_manager.py:361 and :363 for this manager, all passing no id.
5 participants
What changes were proposed in this PR?
Honor an explicit port ID when retrieving an output port. Calls without an ID retain the existing first-port behavior and
IndexErroron an empty port collection, while unknown explicit IDs fail withKeyError.No current production caller passes a port ID, so the multi-output mismatch described in #8274 is not reachable today. This change corrects the method contract before a port-aware caller is added.
Any related issues, documentation, discussions?
Closes #8274
How was this PR tested?
python -m pytest src/test/python/core/architecture/packaging/test_output_manager.py -k get_port -qpython -m ruff check src/main/python/core/architecture/packaging/output_manager.py src/test/python/core/architecture/packaging/test_output_manager.pypython -m ruff format --check src/main/python/core/architecture/packaging/output_manager.py src/test/python/core/architecture/packaging/test_output_manager.pyThe five focused tests passed. Ruff checks passed.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex