Skip to content

refactor(engine): pass Python worker startup arguments by name#5597

Open
yangzhang75 wants to merge 4 commits into
apache:mainfrom
yangzhang75:refactor/python-worker-named-startup-args
Open

refactor(engine): pass Python worker startup arguments by name#5597
yangzhang75 wants to merge 4 commits into
apache:mainfrom
yangzhang75:refactor/python-worker-named-startup-args

Conversation

@yangzhang75

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Passes Python worker startup configuration by name instead of by argv position, as proposed in the issue.

PythonWorkflowWorker (JVM) previously built ~19 positional command-line arguments, and texera_run_python_worker.py unpacked them positionally. Because the two sides agreed only by index, adding/removing/reordering one argument could silently misassign values.

  • PythonWorkflowWorker.scala: build a single JSON object of named startup-config keys and pass it as one argument.
  • texera_run_python_worker.py: parse that JSON and read each value by key; a missing or renamed key now raises a clear KeyError instead of silently misaligning.

The set of keys written on the Scala side and read on the Python side is identical (19 keys). No behavior change otherwise.

Any related issues, documentation, discussions?

Closes #5547

How was this PR tested?

  • Verified the JSON key set written by PythonWorkflowWorker.scala exactly matches the keys read in texera_run_python_worker.py (19 keys, no drift).
  • WorkflowExecutionService/compile (amber) succeeds.
  • scalafmtCheckAll passes; scalafix rules (RemoveUnused, ProcedureSyntax) are satisfied (the new import is used).
  • Python entry script passes py_compile.
  • End-to-end worker launch is exercised by the existing CI integration jobs (amber-integration), which start a real Python worker.

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

Generated-by: Claude Code (Claude Opus 4.8)

@codecov-commenter

codecov-commenter commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.13793% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.76%. Comparing base (07ca5d4) to head (d4b6a42).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
...chitecture/pythonworker/PythonWorkflowWorker.scala 62.16% 6 Missing and 8 partials ⚠️
amber/src/main/python/texera_run_python_worker.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5597      +/-   ##
============================================
+ Coverage     52.38%   52.76%   +0.37%     
- Complexity     2484     2529      +45     
============================================
  Files          1070     1071       +1     
  Lines         41359    41525     +166     
  Branches       4441     4462      +21     
============================================
+ Hits          21666    21910     +244     
+ Misses        18427    18331      -96     
- Partials       1266     1284      +18     
Flag Coverage Δ *Carryforward flag
access-control-service 64.61% <ø> (ø) Carriedforward from cb714f5
agent-service 33.76% <ø> (ø) Carriedforward from cb714f5
amber 54.00% <62.16%> (+0.68%) ⬆️
computing-unit-managing-service 1.65% <ø> (ø) Carriedforward from cb714f5
config-service 56.06% <ø> (ø) Carriedforward from cb714f5
file-service 38.21% <ø> (ø) Carriedforward from cb714f5
frontend 46.91% <ø> (ø) Carriedforward from cb714f5
pyamber 91.02% <95.23%> (+0.30%) ⬆️
python 90.82% <ø> (+0.06%) ⬆️ Carriedforward from cb714f5
workflow-compiling-service 58.69% <ø> (ø) Carriedforward from cb714f5

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

@xuang7

xuang7 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Could you assign a reviewer for this PR? @chenlica

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change. We need to add tests to guard the behavior.

@yangzhang75

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang I added unit tests for the mapping and missing-key behavior in the latest commit, CI's green. Could you take another look? Thanks!

Comment thread amber/src/test/python/test_run_python_worker.py Outdated
@chenlica

Copy link
Copy Markdown
Contributor

Could you assign a reviewer for this PR? @chenlica

@Yicong-Huang can decide.

Yicong-Huang
Yicong-Huang previously approved these changes Jun 10, 2026

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@Yicong-Huang

Copy link
Copy Markdown
Contributor

Codecov Report

❌ Patch coverage is 47.05882% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.41%. Comparing base (07ca5d4) to head (cb714f5).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...chitecture/pythonworker/PythonWorkflowWorker.scala 13.33% 25 Missing and 1 partial ⚠️
amber/src/main/python/texera_run_python_worker.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5597      +/-   ##
============================================
+ Coverage     52.38%   52.41%   +0.03%     
+ Complexity     2484     2479       -5     
============================================
  Files          1070     1070              
  Lines         41359    41388      +29     
  Branches       4441     4442       +1     
============================================
+ Hits          21666    21695      +29     
+ Misses        18427    18422       -5     
- Partials       1266     1271       +5     
Flag Coverage Δ *Carryforward flag
access-control-service 64.61% <ø> (ø) Carriedforward from 72affa3
agent-service 33.76% <ø> (ø) Carriedforward from 72affa3
amber 53.26% <13.33%> (-0.05%) ⬇️
computing-unit-managing-service 1.65% <ø> (ø) Carriedforward from 72affa3
config-service 56.06% <ø> (ø) Carriedforward from 72affa3
file-service 38.21% <ø> (ø) Carriedforward from 72affa3
frontend 46.91% <ø> (ø) Carriedforward from 72affa3
pyamber 91.07% <95.23%> (+0.35%) ⬆️
python 90.82% <ø> (+0.06%) ⬆️ Carriedforward from 72affa3
workflow-compiling-service 58.69% <ø> (ø) Carriedforward from 72affa3

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

Lines in Scala file are uncovered, but unrelate to this change. We can cover them later

@Yicong-Huang

Copy link
Copy Markdown
Contributor

Wait I take it back. The lines in Scala are related. @yangzhang75 can you improve coverage on Scala side?

@Yicong-Huang Yicong-Huang dismissed their stale review June 10, 2026 22:20

Need to fill the coverage on Scala side

@github-actions

Copy link
Copy Markdown
Contributor

✅ No material benchmark regressions detected

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

CI benchmark results are noisy; treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
bs=10 sw=10 sl=64 392 0.239 24,992/39,468/39,468 us ⚪ within ±5% / 🔴 +9.6%
🟢 bs=100 sw=10 sl=64 979 0.598 98,252/128,217/128,217 us 🟢 +34.4% / 🟢 -14.5%
🟢 bs=1000 sw=10 sl=64 1,105 0.674 910,394/1,002,927/1,002,927 us 🟢 +16.8% / 🟢 +9.4%
Baseline details

Latest main 7ae9b35 from 2026-06-13T00:44:28.840Z

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 392 tuples/sec 394.18 tuples/sec 400.34 tuples/sec -0.6% -2.1%
bs=10 sw=10 sl=64 MB/s 0.239 MB/s 0.241 MB/s 0.244 MB/s -0.7% -2.2%
bs=10 sw=10 sl=64 p50 24,992 us 24,805 us 24,191 us +0.8% +3.3%
bs=10 sw=10 sl=64 p95 39,468 us 38,345 us 36,018 us +2.9% +9.6%
bs=10 sw=10 sl=64 p99 39,468 us 38,345 us 36,018 us +2.9% +9.6%
bs=100 sw=10 sl=64 throughput 979 tuples/sec 729.17 tuples/sec 867.19 tuples/sec +34.3% +12.9%
bs=100 sw=10 sl=64 MB/s 0.598 MB/s 0.445 MB/s 0.529 MB/s +34.4% +13.0%
bs=100 sw=10 sl=64 p50 98,252 us 133,219 us 114,874 us -26.2% -14.5%
bs=100 sw=10 sl=64 p95 128,217 us 173,178 us 142,264 us -26.0% -9.9%
bs=100 sw=10 sl=64 p99 128,217 us 173,178 us 142,264 us -26.0% -9.9%
bs=1000 sw=10 sl=64 throughput 1,105 tuples/sec 946.3 tuples/sec 1,010 tuples/sec +16.8% +9.4%
bs=1000 sw=10 sl=64 MB/s 0.674 MB/s 0.578 MB/s 0.616 MB/s +16.7% +9.4%
bs=1000 sw=10 sl=64 p50 910,394 us 1,059,718 us 995,071 us -14.1% -8.5%
bs=1000 sw=10 sl=64 p95 1,002,927 us 1,104,221 us 1,046,228 us -9.2% -4.1%
bs=1000 sw=10 sl=64 p99 1,002,927 us 1,104,221 us 1,046,228 us -9.2% -4.1%
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,509.74,200,128000,392,0.239,24992.11,39467.96,39467.96
1,100,10,64,20,2042.43,2000,1280000,979,0.598,98252.00,128217.42,128217.42
2,1000,10,64,20,18098.78,20000,12800000,1105,0.674,910394.19,1002927.15,1002927.15

@yangzhang75

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang I extracted the config into buildStartupConfig and added Scala tests for it (all 19 keys + a round-trip).

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pass Python worker startup arguments by name instead of by position

5 participants