Skip to content

fix(amber): preserve evaluated values - #8236

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/evaluated-value-control-return
Open

fix(amber): preserve evaluated values#8236
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/evaluated-value-control-return

Conversation

@carloea2

@carloea2 carloea2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Add the worker's EvaluatedValue reply to the generic ControlReturn transport. This prevents evaluated Python expression results from being replaced by an empty return before they reach the coordinator.

Any related issues, documentation, discussions?

Closes #8235

How was this PR tested?

python -c "import sys,pytest; sys.path[:0]=[worktree amber source]; raise SystemExit(pytest.main(['amber/src/test/python/core/architecture/rpc/test_async_rpc_handler_initializer.py','amber/src/test/python/core/architecture/rpc/test_async_rpc_server.py','amber/src/test/python/core/architecture/handlers/control/test_evaluate_expression_handler.py','-q','-p','no:cacheprovider']))"
81 passed

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

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

The Python protobuf bindings were regenerated from the changed descriptor for verification and remain ignored as required by the repository. A direct post-fix transport probe produced:

packed_type=EvaluatedValue
packed_value=42
round_trip=True

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

Generated-by: Codex

@github-actions

Copy link
Copy Markdown
Contributor

👋 Thanks for opening this pull request, @carloea2!

It looks like the pull request description doesn't quite follow our template yet:

  • The What changes were proposed in this PR? section is missing; please keep the template's headings.
  • The Was this PR authored or co-authored using generative AI tooling? section is missing; please keep the template's headings.

Filling out the template helps reviewers understand and triage your contribution faster. Please edit the description to complete it. This message will disappear automatically once the template is followed.

You can find the template prompts by editing the description, or see CONTRIBUTING.md for the full contribution flow.

@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 04:37
@github-actions

github-actions Bot commented Aug 31, 2026

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/protobuf/org/apache/texera/amber/engine/architecture/rpc/controlreturns.proto, amber/src/test/python/core/architecture/rpc/test_async_rpc_handler_initializer.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

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: @Yicong-Huang, @eugenegujing
    You can notify them by mentioning @Yicong-Huang, @eugenegujing in a comment.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 8aea604 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 378 0.231 25,157/35,389/35,389 us 🔴 +7.6% / 🔴 +122.5%
🟢 bs=100 sw=10 sl=64 803 0.49 121,637/150,396/150,396 us 🟢 -19.8% / 🔴 +35.9%
bs=1000 sw=10 sl=64 931 0.568 1,068,781/1,118,944/1,118,944 us ⚪ within ±5% / 🔴 -6.3%
Baseline details

Latest main 8aea604 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 378 tuples/sec 406 tuples/sec 750.65 tuples/sec -6.9% -49.6%
bs=10 sw=10 sl=64 MB/s 0.231 MB/s 0.248 MB/s 0.458 MB/s -6.9% -49.6%
bs=10 sw=10 sl=64 p50 25,157 us 23,371 us 13,049 us +7.6% +92.8%
bs=10 sw=10 sl=64 p95 35,389 us 35,844 us 15,902 us -1.3% +122.5%
bs=10 sw=10 sl=64 p99 35,389 us 35,844 us 19,184 us -1.3% +84.5%
bs=100 sw=10 sl=64 throughput 803 tuples/sec 801 tuples/sec 960.3 tuples/sec +0.2% -16.4%
bs=100 sw=10 sl=64 MB/s 0.49 MB/s 0.489 MB/s 0.586 MB/s +0.2% -16.4%
bs=100 sw=10 sl=64 p50 121,637 us 118,044 us 104,199 us +3.0% +16.7%
bs=100 sw=10 sl=64 p95 150,396 us 187,427 us 110,645 us -19.8% +35.9%
bs=100 sw=10 sl=64 p99 150,396 us 187,427 us 120,888 us -19.8% +24.4%
bs=1000 sw=10 sl=64 throughput 931 tuples/sec 931 tuples/sec 993.27 tuples/sec 0.0% -6.3%
bs=1000 sw=10 sl=64 MB/s 0.568 MB/s 0.568 MB/s 0.606 MB/s 0.0% -6.3%
bs=1000 sw=10 sl=64 p50 1,068,781 us 1,073,110 us 1,012,191 us -0.4% +5.6%
bs=1000 sw=10 sl=64 p95 1,118,944 us 1,139,031 us 1,054,057 us -1.8% +6.2%
bs=1000 sw=10 sl=64 p99 1,118,944 us 1,139,031 us 1,081,313 us -1.8% +3.5%
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,528.58,200,128000,378,0.231,25156.55,35388.60,35388.60
1,100,10,64,20,2490.41,2000,1280000,803,0.490,121636.81,150395.63,150395.63
2,1000,10,64,20,21473.82,20000,12800000,931,0.568,1068780.61,1118943.92,1118943.92

@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.68%. Comparing base (8aea604) to head (66a6ac5).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8236      +/-   ##
============================================
+ Coverage     94.07%   95.68%   +1.60%     
+ Complexity     4809     2870    -1939     
============================================
  Files          1194      930     -264     
  Lines         48597    39931    -8666     
  Branches       5857     4680    -1177     
============================================
- Hits          45718    38208    -7510     
+ Misses         1428      891     -537     
+ Partials       1451      832     -619     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 8aea604
agent-service 99.32% <ø> (ø) Carriedforward from 8aea604
amber 93.45% <ø> (+3.34%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 8aea604
config-service 87.00% <ø> (ø) Carriedforward from 8aea604
file-service 87.91% <ø> (ø) Carriedforward from 8aea604
frontend 96.82% <ø> (ø) Carriedforward from 8aea604
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 8aea604
pyamber 98.18% <ø> (ø)
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 8aea604

*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.

@carloea2
carloea2 force-pushed the fix/evaluated-value-control-return branch from 03e6028 to 1f1e4ba Compare August 31, 2026 18:59
@carloea2

Copy link
Copy Markdown
Contributor Author

Amber failed in TexeraWebSocketEventSpec after the ActorSystem shutdown error. PyAmber and both Amber integration jobs passed. I cannot rerun repository jobs as a contributor. Could someone rerun Amber?

@Yicong-Huang

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 20:08
@carloea2
carloea2 marked this pull request as ready for review August 31, 2026 20:18
@carloea2

Copy link
Copy Markdown
Contributor Author

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

@carloea2
carloea2 force-pushed the fix/evaluated-value-control-return branch from 1f1e4ba to 4a12f43 Compare September 1, 2026 18:08
@carloea2
carloea2 force-pushed the fix/evaluated-value-control-return branch from 4a12f43 to 66a6ac5 Compare September 1, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix pyamber release/v1.2 back porting to release/v1.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python expression results are dropped by worker transport

3 participants