Skip to content

feat(pyamber): route ranges by unique receivers - #8246

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/range-shuffle-unique-receivers
Open

feat(pyamber): route ranges by unique receivers#8246
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/range-shuffle-unique-receivers

Conversation

@carloea2

@carloea2 carloea2 commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Use the unique receiver count when dividing the range. The planner includes every sender-receiver pair, so counting channels makes the Python bucket index too large when a range link has multiple senders.

For two senders and two receivers over -10 to 9, key 9 previously raised IndexError. It now reaches receiver B, matching the JVM partitioner.

Any related issues, documentation, discussions?

Closes #8245. Provides the receiver-count change discussed in #8258.

How was this PR tested?

Added the OutputManager regression before changing production code: four multi-sender cases failed with IndexError; both single-sender controls passed. The six cases cover one to three senders, one or two receivers, partition boundaries, and values outside the range.

Also executed LinkConfig.toPartitioning on JDK 17 and passed its serialized configurations into Python OutputManager. All six matched the regression configurations byte for byte and passed after the change. This checks planner configuration and worker output dispatch, without running a complete workflow.

With Python 3.12 and PYTHONPATH=src/main/python, from amber:

python -m pytest src/test/python/core/architecture/packaging/test_output_manager.py src/test/python/core/architecture/sendsemantics -q --tb=short -p no:cacheprovider
83 passed

python -m pytest src/test/python/core/architecture src/test/python/core/runnables -q --tb=short -p no:cacheprovider
567 passed

ruff check src/main/python src/test/python
All checks passed

ruff format --check src/main/python src/test/python
217 files already formatted

git diff --check passed.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Codex (GPT-6)

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 31, 2026 05:05
@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
⚠️ release/v1.3 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/python/core/architecture/sendsemantics/range_based_shuffle_partitioner.py, amber/src/test/python/core/architecture/sendsemantics/test_partitioners.py). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here.
✅ release/v1.2 Already labeled — this fix is queued to backport here.

Auto-label run.

@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Ma77Ball
    You can notify them by mentioning @Ma77Ball in a comment.

@codecov-commenter

codecov-commenter commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.90%. Comparing base (50321e4) to head (f28d315).
⚠️ Report is 281 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #8246      +/-   ##
============================================
- Coverage     93.93%   93.90%   -0.04%     
+ Complexity     4762     4761       -1     
============================================
  Files          1191     1191              
  Lines         48308    49051     +743     
  Branches       5377     5377              
============================================
+ Hits          45380    46059     +679     
- Misses         1494     1557      +63     
- Partials       1434     1435       +1
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 50321e4
agent-service 99.32% <ø> (ø) Carriedforward from 50321e4
amber 90.02% <ø> (-0.02%) ⬇️ Carriedforward from 50321e4
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 50321e4
config-service 86.86% <ø> (ø) Carriedforward from 50321e4
file-service 87.91% <ø> (ø) Carriedforward from 50321e4
frontend 96.41% <ø> (ø) Carriedforward from 50321e4
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 50321e4
pyamber 97.87% <100.00%> (-1.01%) ⬇️
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 50321e4

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 10 worse · ⚪ 3 noise (<±5%) · 0 without baseline

Compared against main 289b709 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 419 0.256 23,487/27,296/27,296 us 🟢 -28.4% / 🔴 +106.0%
🔴 bs=100 sw=10 sl=64 852 0.52 117,364/134,199/134,199 us 🔴 +8.4% / 🔴 +40.2%
🔴 bs=1000 sw=10 sl=64 982 0.599 1,003,848/1,160,727/1,160,727 us 🔴 +9.4% / 🔴 +24.3%
Baseline details

Latest main 289b709 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 419 tuples/sec 454 tuples/sec 881.37 tuples/sec -7.7% -52.5%
bs=10 sw=10 sl=64 MB/s 0.256 MB/s 0.277 MB/s 0.538 MB/s -7.6% -52.4%
bs=10 sw=10 sl=64 p50 23,487 us 20,443 us 11,399 us +14.9% +106.0%
bs=10 sw=10 sl=64 p95 27,296 us 38,131 us 14,143 us -28.4% +93.0%
bs=10 sw=10 sl=64 p99 27,296 us 38,131 us 17,513 us -28.4% +55.9%
bs=100 sw=10 sl=64 throughput 852 tuples/sec 911 tuples/sec 1,148 tuples/sec -6.5% -25.8%
bs=100 sw=10 sl=64 MB/s 0.52 MB/s 0.556 MB/s 0.701 MB/s -6.5% -25.8%
bs=100 sw=10 sl=64 p50 117,364 us 109,551 us 89,815 us +7.1% +30.7%
bs=100 sw=10 sl=64 p95 134,199 us 123,794 us 95,717 us +8.4% +40.2%
bs=100 sw=10 sl=64 p99 134,199 us 123,794 us 107,566 us +8.4% +24.8%
bs=1000 sw=10 sl=64 throughput 982 tuples/sec 988 tuples/sec 1,175 tuples/sec -0.6% -16.4%
bs=1000 sw=10 sl=64 MB/s 0.599 MB/s 0.603 MB/s 0.717 MB/s -0.7% -16.5%
bs=1000 sw=10 sl=64 p50 1,003,848 us 1,013,089 us 885,446 us -0.9% +13.4%
bs=1000 sw=10 sl=64 p95 1,160,727 us 1,061,298 us 933,809 us +9.4% +24.3%
bs=1000 sw=10 sl=64 p99 1,160,727 us 1,061,298 us 979,021 us +9.4% +18.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,477.45,200,128000,419,0.256,23486.94,27296.23,27296.23
1,100,10,64,20,2347.30,2000,1280000,852,0.520,117364.15,134199.32,134199.32
2,1000,10,64,20,20369.93,20000,12800000,982,0.599,1003848.20,1160727.18,1160727.18

@Yicong-Huang

Yicong-Huang commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I am converting this PR to a draft: please follow our PR template and mark it ready for review.

@Yicong-Huang
Yicong-Huang marked this pull request as draft August 31, 2026 19:56
@carloea2
carloea2 marked this pull request as ready for review August 31, 2026 20:12
@carloea2

carloea2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

The description now follows the current template, and this is ready for review.

@carloea2 carloea2 changed the title fix(pyamber): route ranges by unique receivers feat(pyamber): route ranges by unique receivers Aug 31, 2026
@mengw15 mengw15 removed the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@xuang7 xuang7 removed the fix label Sep 1, 2026
@xuang7
xuang7 removed their request for review September 1, 2026 01:44
@carloea2 carloea2 mentioned this pull request Sep 21, 2026
Open
This was referenced Oct 7, 2026
Closed
Open
@carloea2

carloea2 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang, could you review this before #8258? It addresses the receiver-divisor question left open there. The build checks pass; I have reproduced duplicate receivers in a unit test, but not through the planner.

@carloea2

carloea2 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @Yicong-Huang

@github-actions
github-actions Bot requested a review from Yicong-Huang October 25, 2026 02:03
@carloea2
carloea2 force-pushed the fix/range-shuffle-unique-receivers branch from f28d315 to 13340f5 Compare November 4, 2026 04:09
@github-actions github-actions Bot added the fix label Nov 4, 2026
@carloea2

carloea2 commented Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

The planner case now has a regression test, and CI is green. Please change the bot's fix label to feature; it came from the old branch name.

@github-actions
github-actions Bot requested a review from aglinxinyuan November 4, 2026 04:29

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yicong-Huang Yicong-Huang Awaiting requested review from Yicong-Huang
@aglinxinyuan aglinxinyuan Awaiting requested review from aglinxinyuan

At least 1 approving review is required to merge this pull request.

Assignees

@carloea2 carloea2

Labels

fix pyamber

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Duplicate channels can crash range shuffle routing

5 participants

@carloea2 @codecov-commenter @Yicong-Huang @mengw15 @xuang7