Skip to content

fix: prevent LoopStart state variable collision - #8126

Open
VuMartin wants to merge 4 commits into
apache:mainfrom
VuMartin:fix/7248-loop-state-variable-collision
Open

fix: prevent LoopStart state variable collision#8126
VuMartin wants to merge 4 commits into
apache:mainfrom
VuMartin:fix/7248-loop-state-variable-collision

Conversation

@VuMartin

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Fixes LoopStart state variables being silently overwritten by upstream state. LoopStart now detects collisions between existing loop variables and incoming state variables and raises an error instead of overwriting the loop variable.

Before:
Screenshot 2026-08-27 at 6 39 29 PM

Screenshot 2026-08-28 at 12 14 43 AM

After:
Screenshot 2026-08-28 at 12 03 05 AM

Screenshot 2026-08-28 at 12 07 52 AM

Any related issues, documentation, discussions?

Fixes #7248

How was this PR tested?

Added a unit test verifying that LoopStart rejects incoming state that attempts to overwrite an existing loop state variable.

Ran the targeted pytest test successfully: 1 passed.

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

Generated-by: ChatGPT (5.5 mini)

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

@VuMartin VuMartin changed the title Fix LoopStart state variable collision fix: prevent LoopStart state variable collision Aug 30, 2026
@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 30, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 30, 2026 08:01
@github-actions

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/models/operator.py, amber/src/test/python/core/models/test_operator.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 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 98588bf 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 569 0.347 17,567/34,252/34,252 us 🔴 +32.5% / 🔴 +132.0%
🟢 bs=100 sw=10 sl=64 1,317 0.804 73,779/102,903/102,903 us 🟢 -7.7% / 🟢 -22.0%
bs=1000 sw=10 sl=64 1,554 0.948 640,764/689,027/689,027 us ⚪ within ±5% / 🟢 +39.8%
Baseline details

Latest main 98588bf from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 569 tuples/sec 672 tuples/sec 842.25 tuples/sec -15.3% -32.4%
bs=10 sw=10 sl=64 MB/s 0.347 MB/s 0.41 MB/s 0.514 MB/s -15.4% -32.5%
bs=10 sw=10 sl=64 p50 17,567 us 14,530 us 11,860 us +20.9% +48.1%
bs=10 sw=10 sl=64 p95 34,252 us 25,841 us 14,764 us +32.5% +132.0%
bs=10 sw=10 sl=64 p99 34,252 us 25,841 us 17,870 us +32.5% +91.7%
bs=100 sw=10 sl=64 throughput 1,317 tuples/sec 1,311 tuples/sec 1,084 tuples/sec +0.5% +21.5%
bs=100 sw=10 sl=64 MB/s 0.804 MB/s 0.8 MB/s 0.662 MB/s +0.5% +21.5%
bs=100 sw=10 sl=64 p50 73,779 us 73,840 us 94,616 us -0.1% -22.0%
bs=100 sw=10 sl=64 p95 102,903 us 111,541 us 100,981 us -7.7% +1.9%
bs=100 sw=10 sl=64 p99 102,903 us 111,541 us 108,141 us -7.7% -4.8%
bs=1000 sw=10 sl=64 throughput 1,554 tuples/sec 1,554 tuples/sec 1,112 tuples/sec 0.0% +39.8%
bs=1000 sw=10 sl=64 MB/s 0.948 MB/s 0.949 MB/s 0.679 MB/s -0.1% +39.7%
bs=1000 sw=10 sl=64 p50 640,764 us 641,575 us 930,623 us -0.1% -31.1%
bs=1000 sw=10 sl=64 p95 689,027 us 709,762 us 974,308 us -2.9% -29.3%
bs=1000 sw=10 sl=64 p99 689,027 us 709,762 us 1,002,806 us -2.9% -31.3%
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,351.57,200,128000,569,0.347,17566.85,34251.50,34251.50
1,100,10,64,20,1519.17,2000,1280000,1317,0.804,73779.38,102902.89,102902.89
2,1000,10,64,20,12872.32,20000,12800000,1554,0.948,640764.21,689027.20,689027.20

@codecov-commenter

codecov-commenter commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.03%. Comparing base (98588bf) to head (dc5faff).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8126   +/-   ##
=========================================
  Coverage     94.03%   94.03%           
  Complexity     4762     4762           
=========================================
  Files          1181     1181           
  Lines         47877    47880    +3     
  Branches       5324     5324           
=========================================
+ Hits          45020    45023    +3     
  Misses         1433     1433           
  Partials       1424     1424           
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 98588bf
agent-service 99.32% <ø> (ø) Carriedforward from 98588bf
amber 90.03% <ø> (ø) Carriedforward from 98588bf
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 98588bf
config-service 86.86% <ø> (ø) Carriedforward from 98588bf
file-service 87.91% <ø> (ø) Carriedforward from 98588bf
frontend 96.68% <ø> (ø) Carriedforward from 98588bf
notebook-migration-service 79.31% <ø> (ø) Carriedforward from 98588bf
pyamber 98.87% <100.00%> (+<0.01%) ⬆️
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 98588bf

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

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stateful operator upstream of a Loop Start silently overwrites loop variables

3 participants