Skip to content

test(verify): run a Python operator the way the engine runs it - #8357

Open
kz930 wants to merge 23 commits into
apache:mainfrom
kz930:feat/verify-run-operator-python
Open

kz930 wants to merge 23 commits into
apache:mainfrom
kz930:feat/verify-run-operator-python

Conversation

@kz930

@kz930 kz930 commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

A Python operator is not called; it is handed to an interpreter that imports
the generated module and drives it through the same open, process and close
the engine uses. PyOpExecHarness writes that module, starts the driver, and
reads back what the operator emitted.

The driver is the engine's side of the contract written out plainly: it is
what makes the answer this side produces the engine's answer rather than an
approximation of it.

A timestamp reaches the operator the way the engine hands one to a Python worker: a naive datetime at microsecond resolution. It is read without pandas, so a year past 2262 arrives intact. That is the zoneless microsecond Arrow type #8704 makes the engine send.

A port that carried no rows reaches the operator with its declared columns, as #8766 makes the engine hand them. The driver declares each input port's schema right before that port finishes, where the worker does.

Eight lines of build.sbt are unrelated to the export and fixed while this change is in the file: File Service's test grouping read its fork options inside the lambda, which sbt hoists anyway, so the warning it printed on every build said the code meant something it did not.

Any related issues, documentation, discussions?

Part of #8325, 4 of 27; that issue lists the set in order.

Closes #8409, the task this change is the whole of.

How was this PR tested?

The tests in this change cover it. PyOpExecHarnessSpec also checks that a row the output schema does not describe is refused. It also checks that 2500-01-01 00:00:00.123456789 reaches the operator as 2500-01-01 00:00:00.123456. It also checks that a port that carried no rows hands the operator its declared columns. Without the driver's change the operator sees a table with no columns. The whole set is exercised together once the last piece lands: every operator run through the engine and through its generated script, and the two answers compared.

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

Generated-by: Claude Code (Claude Opus 5, Claude Opus 5.5)

A Python operator is not called; it is handed to an interpreter that imports
the generated module and drives it through the same open, process and close
the engine uses. `PyOpExecHarness` writes that module, starts the driver, and
reads back what the operator emitted.

The driver is the engine's side of the contract written out plainly: it is
what makes the answer this side produces the engine's answer rather than an
approximation of it.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@github-actions github-actions Bot added feature dependencies Pull requests that update a dependency file common platform Non-amber Scala service paths labels Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026 •

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: @Xiao-zhen-Liu
    You can notify them by mentioning @Xiao-zhen-Liu in a comment.

@codecov-commenter

codecov-commenter commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.79%. Comparing base (2f0089f) to head (3ea81b5).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8357      +/-   ##
============================================
- Coverage     91.79%   91.79%   -0.01%     
  Complexity     5020     5020              
============================================
  Files          1274     1274              
  Lines         56301    56301              
  Branches       7352     7352              
============================================
- Hits          51682    51681       -1     
  Misses         2766     2766              
- Partials       1853     1854       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 77.44% <ø> (ø)
agent-service 99.53% <ø> (ø) Carriedforward from 379f9a7
amber 85.75% <ø> (ø)
computing-unit-managing-service 64.05% <ø> (ø)
config-service 87.61% <ø> (ø)
file-service 82.29% <ø> (ø)
frontend 96.53% <ø> (-0.01%) ⬇️ Carriedforward from 379f9a7
notebook-migration-service 83.77% <ø> (ø)
pyamber 98.46% <ø> (ø) Carriedforward from 379f9a7
workflow-compiling-service 77.95% <ø> (ø)

*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 Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 7d05326 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 428 0.261 22,240/30,358/30,358 us 🔴 +11.9% / 🔴 +114.0%
🟢 bs=100 sw=10 sl=64 955 0.583 103,175/122,306/122,306 us 🟢 -19.1% / 🔴 +26.9%
🔴 bs=1000 sw=10 sl=64 1,100 0.671 896,174/1,093,499/1,093,499 us 🔴 +19.6% / 🔴 +19.1%
Baseline details

Latest main 7d05326 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 428 tuples/sec 484 tuples/sec 880.16 tuples/sec -11.6% -51.4%
bs=10 sw=10 sl=64 MB/s 0.261 MB/s 0.295 MB/s 0.537 MB/s -11.5% -51.4%
bs=10 sw=10 sl=64 p50 22,240 us 19,883 us 11,544 us +11.9% +92.7%
bs=10 sw=10 sl=64 p95 30,358 us 29,355 us 14,186 us +3.4% +114.0%
bs=10 sw=10 sl=64 p99 30,358 us 29,355 us 17,122 us +3.4% +77.3%
bs=100 sw=10 sl=64 throughput 955 tuples/sec 975 tuples/sec 1,151 tuples/sec -2.1% -17.0%
bs=100 sw=10 sl=64 MB/s 0.583 MB/s 0.595 MB/s 0.703 MB/s -2.0% -17.0%
bs=100 sw=10 sl=64 p50 103,175 us 98,817 us 89,900 us +4.4% +14.8%
bs=100 sw=10 sl=64 p95 122,306 us 151,130 us 96,346 us -19.1% +26.9%
bs=100 sw=10 sl=64 p99 122,306 us 151,130 us 110,344 us -19.1% +10.8%
bs=1000 sw=10 sl=64 throughput 1,100 tuples/sec 1,122 tuples/sec 1,189 tuples/sec -2.0% -7.5%
bs=1000 sw=10 sl=64 MB/s 0.671 MB/s 0.685 MB/s 0.726 MB/s -2.0% -7.6%
bs=1000 sw=10 sl=64 p50 896,174 us 892,395 us 873,675 us +0.4% +2.6%
bs=1000 sw=10 sl=64 p95 1,093,499 us 914,670 us 918,440 us +19.6% +19.1%
bs=1000 sw=10 sl=64 p99 1,093,499 us 914,670 us 953,075 us +19.6% +14.7%
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,467.25,200,128000,428,0.261,22239.82,30358.10,30358.10
1,100,10,64,20,2094.44,2000,1280000,955,0.583,103174.84,122305.50,122305.50
2,1000,10,64,20,18186.94,20000,12800000,1100,0.671,896173.84,1093499.27,1093499.27

Eight lines this change is already in the file for. sbt lifts a `.value`
written inside a lambda to the top of the task, so the options were read once
rather than per suite either way; written where they were, they said the other
thing, and sbt warned on it in every build.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
kz930 added a commit to Nicoleee1108/texera_workflow_to_py that referenced this pull request Sep 3, 2026
Matches apache#8357, which carries this upstream. sbt lifts a `.value` written
inside a lambda to the top of the task, so the options were read once either
way; written where they were, they said the other thing.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
kz930 and others added 2 commits September 3, 2026 15:11
The two helpers here were a second copy of OpExecHarness's, carrying a note that
they were kept inline so each harness read on its own and would be consolidated
if a third arrived. Two copies of the same twenty lines is already the cost that
note was deferring: they have to be changed together, and nothing says so at
either site.

Preparing the plan does not vary with the executor, so it happens once now. What
differs between the harnesses stays here: how the prepared op is run.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Three kinds of comment came out. A drawing of the string the code below
assembles. A restatement of a branch the reader can see. And the word MVP,
which dated the scope to a moment rather than stating it.

What replaces them says the same thing shorter, or says what the code cannot:
which cases the harness does not drive and why none of them has an operator
asking for it.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@carloea2 carloea2 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.

The Python operator harness looks good.

@kz930
kz930 marked this pull request as draft September 4, 2026 17:44
`PhysicalPlan` is named only by `getPhysicalPlan` and by a comment, so scalafix
reports the import and the check fails before any test runs.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@carloea2 carloea2 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.

The unused import was removed. Looks good.

@carloea2 carloea2 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.

Reviewed alongside the related export and verification PRs. These findings are based on code inspection and focused Python checks, not a full Scala suite run.

The driver projected the operator's output onto the schema's attribute
names and coerced each value with int/float/str. That is looser than the
engine, which hands every yielded value to all_output_to_tuple and
finalizes the result: an operator putting 1.5 in an INTEGER field, or a
row with a field the schema does not have, or one missing a field, all
raise there and none of the three raised here. A reference path that
accepts output the engine rejects cannot say what the engine would do,
so call the engine's own two functions instead of re-deriving them.

finalize pickles a BINARY field's object itself and tags it, where the
standalone side pickles the same object untagged, so drop the tag on the
way out and both sides carry the same payload.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@carloea2 carloea2 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.

Further review with local regression tests and connected execution checks.

kz930 and others added 2 commits September 10, 2026 13:02
The driver collected everything the operator yielded and converted the
list afterwards. `DataProcessor` reads a yield where it happens, turning
it into tuples and finalizing them on the spot, so a UDF that yields one
dict twice and changes it in between is recorded as the two values it
produced. The driver recorded the last one twice.

The operator now takes a callback and hands each value over at the
yield, before the generator is advanced.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
DataProcessor writes each input port's schema into the executor before
calling on_finish, so a TableOperator handed no rows can still say what
its columns were. The driver stands in for DataProcessor here, so it has
to do the same or the reference path answers on a frame of no columns
where the engine answers on the declared ones.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@carloea2 carloea2 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.

The large binary handling is now covered on both the Python and Scala sides, including input and output. The earlier blocker is fixed. Looks good.

apache#8488 is closed: a table with no rows carries no schema by
design, so there is nothing for the driver to hand the operator and no
engine behavior here to match.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

@carloea2 carloea2 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.

One input-decoding mismatch reproduced against the actual Arrow tuple provider. Ordinary bytes passed as a control.

kz930 and others added 2 commits September 23, 2026 16:16
The worker's ArrowTableTupleProvider unpickles a BINARY cell that starts
with the cast's pickle marker, so a model column reaches the next
operator as a model. The driver handed the operator the bytes instead,
and a predict on them raised AttributeError.

Decoding the cell alone was not enough. The driver then finalized each
input tuple, and the cast in finalize pickles any non-bytes value back
into bytes. The worker's InputManager attaches the schema without
finalizing, so the driver now builds input tuples the same way.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
… spec

The pickled-cell test used a pickled list in place of a model. It now
pickles a fitted DecisionTreeClassifier with the interpreter the harness
runs and has the operator call predict on it, which is the call that
raised AttributeError when the driver handed over bytes.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…and refuse a row the schema does not describe

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@kz930

kz930 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 A Python operator now receives a timestamp as the engine sends it after #8704: a naive datetime at microsecond resolution.

@carloea2 carloea2 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.

The current driver passes 12 direct Python probes with the declared #8766 SDK dependency. Seven Scala harness cases pass too, but the new model fixture fails on Windows before reaching the driver. scalafmtCheckAll passes.

Windows drops the quotes from a multiline `-c` argument, so the bytes
literal reached Python as `bpickle` and the fixture failed before the
harness ran. Write the script to a temporary .py file and run that.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@kz930

kz930 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 The model helper now runs its script from a temporary .py file (b581ae0). Could you take another look when you have time?

@carloea2 carloea2 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.

The Windows model-fixture fix passes. With the declared #8766 SDK selected through TEXERA_AMBER_PYTHON_HOME, all 8 Scala tests and 12 direct Python 3.12 driver probes pass. Formatting passes. No new finding in this update; the SDK prerequisite is still needed.

sbt -batch 'WorkflowCompilingService/testOnly *PyOpExecHarnessSpec -- -oDF' 'scalafmtCheckAll'

kz930 added a commit to Nicoleee1108/texera_workflow_to_py that referenced this pull request Oct 13, 2026
As the worker's EndChannelHandler now does (apache#8506). Without it the native
side of the HashJoin right-outer variant put the right columns into the left
ones. Moves to apache#8357 once apache#8506 is merged upstream.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
As the worker's EndChannelHandler does since apache#8506: a HashJoin probe that
received no build rows names its right columns from the build port's
declared schema.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@kz930
kz930 requested a review from carloea2 October 13, 2026 05:42
…iter does

json.dumps writes a bare Infinity, which is not JSON; the engine's writer spells
it as text. A NaN still leaves as null, as the worker casts it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@kz930

kz930 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Two commits since your review: the executor gets each finished port's declared schema, and a Python operator writes infinities as the engine does. Could you take another look?

@carloea2 carloea2 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.

Retested the updated input-schema and infinity handling on fresh upstream main. All 8 PyOpExecHarnessSpec tests and scalafmtCheckAll pass with the declared SDK, executor-schema and Arrow-type APIs supplied locally. No new harness finding. Code looks good; the prerequisite PRs and CI still need clearing. Keeping the draft unchanged.

@kz930
kz930 marked this pull request as ready for review November 9, 2026 07:13

@carloea2 carloea2 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.

This conflicts with current main in build.sbt. Please keep both the DAO test-fixture dependency and the WorkflowOperator test-class dependency when rebasing.

@kz930

kz930 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Merged main and kept both the DAO and WorkflowOperator test dependencies.

@carloea2 carloea2 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.

The rebase keeps both test dependencies. All 8 PyOpExecHarnessSpec tests and scalafmtCheckAll pass locally. CI is green.

@aglinxinyuan

Copy link
Copy Markdown
Contributor

Can you explain in high level why do we need this PR? What's the limitation of the original design that scala runs the python code?

@kz930

kz930 commented Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

A Python operator only runs inside a Python worker, and starting the engine's full worker stack for every operator in a test is too heavy. This PR runs the generated Python in a small driver that mimics the worker, so the visualization and ML operators can be verified too.

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

Labels

common dependencies Pull requests that update a dependency file feature platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run a Python operator the way the engine runs it

4 participants