chore(amber): remove the fully commented-out PythonWorkflowWorkerSpec by aglinxinyuan · Pull Request #7447 · apache/texera

aglinxinyuan · 2026-08-09T11:35:17Z

What changes were proposed in this PR?

Deletes PythonWorkflowWorkerSpec.scala, which contributes zero test suites — all 201 lines are commented out, including the package declaration, so it compiles to a licence header. −201 lines, no behaviour change.

The commented body targets APIs deleted long ago — InitializeOperatorLogicHandler, UpdateInputLinkingHandler, OpenOperator, engine.common.Constants, the old PhysicalLink shape — so it cannot be un-commented as-is; reviving these tests would mean rewriting them against the current engine.

Reviewer note: live PythonWorkflowWorker coverage is in the @IntegrationTest e2e specs, which this file does not participate in. Nothing here is the only cover for a code path.

Any related issues, documentation, discussions?

Closes #7445

How was this PR tested?

Existing tests only — this PR removes a file that contains no runnable test.

Locally, from the repo root with Java 17:

sbt "WorkflowExecutionService/Test/compile" — success.

Verification, re-runnable by a reviewer:

grep -cvE '^\s*(//|/\*|\*|$)' amber/src/test/scala/org/apache/texera/amber/engine/architecture/pythonworker/PythonWorkflowWorkerSpec.scala   # 0

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

Generated-by: Claude Code (Claude Opus 5)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-08-09T11:35:32Z

Automated Reviewer Suggestions

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

No candidates found from git blame history.

github-actions · 2026-08-09T11:41:38Z

⚠️ Benchmark changes need a look

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

Compared against main e03d971 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	392	0.239	24,448/31,108/31,108 us	🟢 -16.9% / 🔴 +93.0%
🔴	bs=100 sw=10 sl=64	783	0.478	127,726/167,705/167,705 us	🔴 +14.7% / 🔴 +50.8%
🔴	bs=1000 sw=10 sl=64	913	0.557	1,083,465/1,227,930/1,227,930 us	🔴 +6.3% / 🔴 +15.5%

Baseline details

Latest main e03d971 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	392 tuples/sec	397 tuples/sec	752.25 tuples/sec	-1.3%	-47.9%
bs=10 sw=10 sl=64	MB/s	0.239 MB/s	0.242 MB/s	0.459 MB/s	-1.2%	-47.9%
bs=10 sw=10 sl=64	p50	24,448 us	25,048 us	13,006 us	-2.4%	+88.0%
bs=10 sw=10 sl=64	p95	31,108 us	37,437 us	16,117 us	-16.9%	+93.0%
bs=10 sw=10 sl=64	p99	31,108 us	37,437 us	19,286 us	-16.9%	+61.3%
bs=100 sw=10 sl=64	throughput	783 tuples/sec	811 tuples/sec	953.66 tuples/sec	-3.5%	-17.9%
bs=100 sw=10 sl=64	MB/s	0.478 MB/s	0.495 MB/s	0.582 MB/s	-3.4%	-17.9%
bs=100 sw=10 sl=64	p50	127,726 us	120,719 us	105,130 us	+5.8%	+21.5%
bs=100 sw=10 sl=64	p95	167,705 us	146,162 us	111,226 us	+14.7%	+50.8%
bs=100 sw=10 sl=64	p99	167,705 us	146,162 us	121,065 us	+14.7%	+38.5%
bs=1000 sw=10 sl=64	throughput	913 tuples/sec	920 tuples/sec	984.25 tuples/sec	-0.8%	-7.2%
bs=1000 sw=10 sl=64	MB/s	0.557 MB/s	0.562 MB/s	0.601 MB/s	-0.9%	-7.3%
bs=1000 sw=10 sl=64	p50	1,083,465 us	1,083,686 us	1,024,295 us	-0.0%	+5.8%
bs=1000 sw=10 sl=64	p95	1,227,930 us	1,155,247 us	1,062,710 us	+6.3%	+15.5%
bs=1000 sw=10 sl=64	p99	1,227,930 us	1,155,247 us	1,095,400 us	+6.3%	+12.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.72,200,128000,392,0.239,24447.65,31108.41,31108.41
1,100,10,64,20,2554.35,2000,1280000,783,0.478,127725.66,167705.21,167705.21
2,1000,10,64,20,21911.25,20000,12800000,913,0.557,1083464.54,1227930.46,1227930.46

codecov-commenter · 2026-08-09T11:44:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.32%. Comparing base (e03d971) to head (d053b25).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7447      +/-   ##
============================================
- Coverage     85.32%   85.32%   -0.01%     
+ Complexity     4151     4150       -1     
============================================
  Files          1169     1169              
  Lines         46740    46740              
  Branches       5202     5202              
============================================
- Hits          39882    39881       -1     
  Misses         5148     5148              
- Partials       1710     1711       +1

Flag	Coverage Δ		*Carryforward flag
access-control-service	`70.00% <ø> (ø)`		Carriedforward from e03d971
agent-service	`85.50% <ø> (ø)`		Carriedforward from e03d971
amber	`80.88% <ø> (-0.01%)`	⬇️
computing-unit-managing-service	`50.72% <ø> (ø)`		Carriedforward from e03d971
config-service	`65.97% <ø> (ø)`		Carriedforward from e03d971
file-service	`69.05% <ø> (ø)`		Carriedforward from e03d971
frontend	`87.85% <ø> (ø)`		Carriedforward from e03d971
notebook-migration-service	`78.89% <ø> (ø)`		Carriedforward from e03d971
pyamber	`97.51% <ø> (ø)`		Carriedforward from e03d971
workflow-compiling-service	`26.31% <ø> (ø)`		Carriedforward from e03d971

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

Yicong-Huang · 2026-08-10T00:30:43Z

instead of removing it, can we add some unit tests? I know there are e2e tests already but good to guard the behavior with unit tests.

aglinxinyuan · 2026-08-10T00:58:17Z

Good point — opened #7488, which replaces this file with real unit tests instead of only deleting it: PythonProxyServerSpec and PythonProxyClientSpec cover the JVM side of the JVM↔Python Flight bridge (control/data/state/ECM routing, credit acks, connection retries) without needing a Python process, while the worker actor itself stays covered by the e2e tests. Closing this one in favor of that PR.

…onworker proxy unit tests (apache#7488) ### What changes were proposed in this PR? apache#7447 proposed deleting `PythonWorkflowWorkerSpec.scala`, which has been fully commented out for years and no longer compiles against today's APIs. Following the review feedback there (apache#7447 (comment)), this PR replaces the dead file with real unit tests instead of only deleting it. The commented-out spec drove `PythonWorkflowWorker` end-to-end, which needs a live Python process — that path is covered by the e2e tests. What can be unit-tested without Python is the JVM side of the JVM↔Python Arrow Flight bridge, which had no coverage until now: ``` JVM Python ┌───────────────────────────────────────┐ │ PythonWorkflowWorker (actor) │ e2e-tested only (needs Python) │ ├─ PythonProxyClient ──── Flight ────┼────▶ network_receiver.py │ │ PythonProxyClientSpec: fake │ │ │ Python Flight server in Scala │ │ └─ PythonProxyServer ◀─── Flight ────┼───── network_sender.py │ PythonProxyServerSpec: test │ │ plays the Python Flight client │ └───────────────────────────────────────┘ ``` | New spec | Subject | Behavior pinned down | |---|---|---| | `PythonProxyServerSpec` | `PythonProxyServer` / `AmberProducer` | `handshake` completes the port promise and replies `ok`; `control` actions route `ControlInvocation` / `ReturnInvocation` to the output gateway on the control channel and ack with a little-endian credit value; `Data` / `State` / `ECM` puts are reassembled into `DataFrame` / `StateFrame` (loop envelope preserved) / `EmbeddedControlMessage` and acked with credits | | `PythonProxyClientSpec` | `PythonProxyClient` | heartbeat handshake happens before the queue is drained; queued `ControlInvocation` / `ReturnInvocation` / actor commands arrive as `control` / `actor` actions with intact protobuf payloads; `DataFrame` / `StateFrame` / ECM puts arrive under the right `PythonDataHeader` with tuples, loop envelope, and bytes intact; queue-size acks update `getQueuedCredit`; connection retries abort with `WorkflowRuntimeException` (no server listening, non-`ack` heartbeat); `close()` before any connection does not throw | Both specs stand in for the Python worker with plain Arrow Flight components (`FlightClient` / `NoOpFlightProducer`), so no Python process is involved. Together with the existing `WorkerBatchInternalQueueSpec` and `PythonWorkflowWorkerStartupConfigSpec`, every class in the `pythonworker` package except the actor itself now has unit coverage. The commented-out `PythonWorkflowWorkerSpec.scala` is removed, superseded by these tests. ### Any related issues, documentation, discussions? Supersedes apache#7447. ### How was this PR tested? This PR is tests-only. Both new specs pass locally: ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.pythonworker.PythonProxyServerSpec org.apache.texera.amber.engine.architecture.pythonworker.PythonProxyClientSpec" ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Fable 5)

Copilot AI lite review requested due to automatic review settings August 9, 2026 11:35

Copilot AI reviewed Aug 9, 2026

github-actions Bot assigned aglinxinyuan Aug 9, 2026

github-actions Bot added the engine label Aug 9, 2026

chore(amber): remove the fully commented-out PythonWorkflowWorkerSpec

d053b25

aglinxinyuan force-pushed the chore/remove-dead-engine-files branch from 9289ebe to d053b25 Compare August 9, 2026 11:54

This was referenced Aug 9, 2026

chore(amber): remove the unused ElidableStatement #7451

Open

chore(amber): remove the unused EmptyReplayLogger #7452

Open

chore(amber): remove the unused SchemaPropagationResponse #7453

Open

aglinxinyuan changed the title ~~chore(amber): remove dead engine files with no production caller~~ chore(amber): remove the fully commented-out PythonWorkflowWorkerSpec Aug 9, 2026

aglinxinyuan requested a review from Yicong-Huang August 9, 2026 23:38

aglinxinyuan mentioned this pull request Aug 10, 2026

test(amber): replace commented-out PythonWorkflowWorkerSpec with pythonworker proxy unit tests #7488

Merged

aglinxinyuan closed this Aug 10, 2026

Navigation Menu

Uh oh!

chore(amber): remove the fully commented-out PythonWorkflowWorkerSpec - #7447

chore(amber): remove the fully commented-out PythonWorkflowWorkerSpec#7447
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:chore/remove-dead-engine-files

aglinxinyuan commented Aug 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Aug 9, 2026

Uh oh!

github-actions Bot commented Aug 9, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 9, 2026 •

edited

Loading

Uh oh!

Yicong-Huang commented Aug 10, 2026

Uh oh!

aglinxinyuan commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

aglinxinyuan commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this PR?

Any related issues, documentation, discussions?

How was this PR tested?

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

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 9, 2026

Automated Reviewer Suggestions

Uh oh!

github-actions Bot commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Benchmark changes need a look

Uh oh!

codecov-commenter commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Yicong-Huang commented Aug 10, 2026

Uh oh!

aglinxinyuan commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aglinxinyuan commented Aug 9, 2026 •

edited

Loading

github-actions Bot commented Aug 9, 2026 •

edited

Loading

codecov-commenter commented Aug 9, 2026 •

edited

Loading