feat: add loop operators - #5700
Conversation
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
… switch Mirrors the same change on state-handshake-redesign (the upstream PR candidate, apache#4560 review feedback). Functionally equivalent to the prior design that removed all three per-task finallys; this design keeps them and drops the run-loop's end-of-body _switch_context() instead, which keeps process_state / process_internal_marker / process_tuple unchanged from origin/main.
Add cases for multi-jump sequences, unknown-target rejection, jump before any pull, jump after schedule exhaustion, and forward jump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reject any `Schedule` constructed with gaps or non-zero starting level keys. The schedule generator already produces contiguous-from-0 keys, so this only tightens the contract for direct callers and tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
# Conflicts: # amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/WorkflowExecutionCoordinator.scala # amber/src/test/scala/org/apache/texera/amber/engine/architecture/scheduling/WorkflowExecutionCoordinatorSpec.scala
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
The __post_init__ that wrapped a non-State frame argument in `State(...)`
was only triggered by two test_network_receiver.py call sites that
passed plain dict literals. Pass `State({...})` explicitly at those call
sites and remove the silent coerce -- callers already know the contract.
Resolve 3 conflicts from main's Controller->Coordinator/Manager rename: - main_loop.py: adopt coordinator_stub()/coordinator_interface for the loop jump + completion (main renamed controller_stub()); keep one stub handle, reused for _jump_to_loop_start and worker_execution_completed. - RegionExecutionManager.scala: keep the loopStartStateUris ctor param (last, defaulted) on top of main's rename; use the Manager default refs. - WorkflowExecutionManager.scala: keep the loopStartStateUris derivation def; take main's Manager naming in the coordinateRegionExecutors scaladoc. The JumpToOperatorRegion RPC merged onto the renamed CoordinatorService.
|
ok, as a related context, we are opening PRs to improve nested data types deserialization in pyarrow. please take this into considerations. |
|
Thanks @aglinxinyuan . Do we want to embed this design decision in this PR or do we want to create a separate discussion? |
I think we can keep the design here because it's an implementation choice. |
you can keep it on PR description, PR comments will be collapsed and no one will read them. PR description can become squashed commit message once merged, so it will be retained. |
Agreed. I believe this design decision should be more visible on GitHub, not embedded deep in the comments. |
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Reviewed against the full #4206 history. This is much improved — I checked each prior concern against the actual code (not just the replies), and almost all are genuinely fixed: worker-id parsing, the pickle-to-Arrow switch, codegen escaping, the name-mangled access, reserved-key isolation, and the scheduler flag coupling. The URI-in-State design question also got resolved: State is 3 columns and the write URI is shipped to workers at setup, so no address is stored in the data.
On size: the diff is large (~2.3k lines), but about 70% of that is tests — the source is only ~690 lines, and the separable pieces already went out as #5900 / #5706 / #5707. What's left is one coherent feature, so I don't think it needs a further split.
Approving. The comments below are non-blocking: two design decisions worth making explicitly (the per-iteration re-scheduling and worker recreation, and the silent mode coercion), one concrete test bug (the specId collision), a codegen-test gap, and a few cleanups.
test_process_state_can_emit_multiple_states duplicated test_process_state_can_emit_consecutive_states -- same executor, monkeypatches, inputs, and assertions, differing only in type annotations. Keep the latter. Per @Xiao-zhen-Liu's review on apache#5700.
MainLoop.complete()'s Loop End condition() error handler re-implemented the logger.exception -> set_exception_info -> error-console-message sequence that DataProcessor._executor_session already does. Extract it to a shared Context.report_exception(err) and call it from both, so they stay in sync. Drops the now-unused sys / loguru / create_error_console_message imports from the two callers. Behavior-preserving. Per @Xiao-zhen-Liu's review on apache#5700.
specId=5 collided with MultiRegionWorkflowIntegrationSpec; both run in the amber-integration job against the shared test DB, and setUpWorkflowExecutionData/cleanup key the seeded rows only on specId, so the two suites would share rows. 1-5 are taken, so use 6. Per @Xiao-zhen-Liu's review on apache#5700.
…exprs
The existing stub subclasses skip the base64 + decode_python_template layer
that the real generated ProcessLoop{Start,End}Operator goes through; that
layer was only exercised end-to-end in the slow @IntegrationTEST job.
Add TestGeneratedCodeShape: build the generated source exactly as
LoopStart/LoopEndOpDesc.generatePythonCode emits it (user expressions arrive
as self.decode_python_template('<base64>')) and exec it with a quote and a
newline in the expressions, then run open()/process_table and
process_state()/condition(). Catches codegen<->runtime drift (e.g. a quote
breaking the generated source) fast and hermetically. Per @Xiao-zhen-Liu's
review on apache#5700.
…sion Two points from review: - The reserved name was centralized in _RESERVED_STATE_KEYS but the literal "table" was repeated at the inject/serialize/read sites. Add _TABLE_KEY = "table", derive _RESERVED_STATE_KEYS from it, and reference it everywhere. - A user loop variable named `table` collided with the runtime's input table and was silently dropped by _strip_reserved. Raise instead (via _reserved_name_error): LoopStart.produce_state_on_finish flags a `table` in self.state; LoopEnd.run_update flags an `update` that rebinds the seeded table. Adds TestReservedNameCollision. Per @Xiao-zhen-Liu's review on apache#5700.
complete() guarded the LoopEnd condition() call but not the back-edge state write in _jump_to_loop_start: a put_one/close failure after the jump DCM would propagate on the main loop thread and kill it via run()'s @logger.catch(reraise=True). Extend the guard to cover the jump call, so a failed write (or a missing loop-back URI) is reported via Context.report_exception + EXCEPTION_PAUSE and completion is skipped, the same as a bad condition. Adds a mirrored test driving a real put_one failure. Per @Xiao-zhen-Liu's review on apache#5700.
effectiveExecutionMode silently coerced a user-selected PIPELINED mode to MATERIALIZED when any operator (e.g. a loop) requires it. Log a warning at the createRegionDAG call site naming the requiring operators, so the override is visible. The companion helper stays pure (its unit tests call it directly). Per @Xiao-zhen-Liu's review on apache#5700.
Fixes the amber/amber-integration/Bench CI failures: main added WorkflowWorkerSpec.scala, whose InitializeExecutorRequest construction predates the loopStartStateUris proto field (no constructor defaults), and CI builds the PR merged with current main. Merge main and pass loopStartStateUris = Map.empty at that site, matching the other call sites.
Comment/test hygiene from a final review sweep; no mechanism changes:
- Fix a doubled @pytest.mark.timeout(2) left by an earlier test deletion,
and add the missing timeout mark on the write-error sibling test.
- Rename stale Controller vocabulary in the jump-test helpers
(_stub_coordinator / _Coordinator) and three RegionExecutionCoordinator
references in the descriptor specs to the post-rename Manager names.
- Replace vacuous never-leak assertions on names that were never envelope
keys ("LoopStartId"/"LoopStartStateURI") with the real key
("loop_start_id").
- run_update: use namespace.get() for the reserved-table check so a user
`update` that deletes `table` raises the friendly reserved-name error
instead of a bare KeyError; new test pins it. Collision tests now pin
which variable was flagged.
- Refresh stale prose: Context.report_exception covers write failures too;
descriptor-spec comments no longer claim `output` is reserved or mention
unpickling; test_loop_operators docstrings match the raise-on-collision
behavior and list the newer test classes.
- Mode-override warn now carries the file's WID/EID log prefix and lists
operators deterministically (sorted).
…rors
Two correctness fixes from a pre-merge review of the loop operators.
1. Loop expressions with generator expressions / comprehensions / lambdas
NameError'd on their own loop variables. eval_output / run_update /
eval_condition / the generated open() ran user code with
`eval/exec(expr, {}, namespace)` -- an empty globals plus a locals-only
mapping. A nested genexp/lambda scope resolves free names against globals,
so `all(x > threshold for x in items)` raised NameError on `threshold`.
Pass the namespace as globals instead; strip the `__builtins__` that exec
injects so it never reaches the persisted loop state. Initialization now
runs through a guarded `LoopStartOperator.run_initialization` helper,
matching the eval_output / run_update / eval_condition pattern (generated
open() is now a one-line delegate).
2. State serialization (state.to_tuple -> to_json) for a LoopStart's produced
state runs on the main loop thread inside _emit_and_save_state, outside
DataProcessor's guarded executor session. A non-JSON-serializable loop
variable (e.g. a numpy array) raised a TypeError that propagated through
run()'s @logger.catch(reraise=True), killing the thread and hanging the
workflow with no operator-facing error. Report it like a UDF error
(exception manager + ERROR console message + EXCEPTION_PAUSE) instead, and
have _process_end_channel hold the region (skip port_completed / complete)
when such an error is reported, so it surfaces rather than silently
completing with single-iteration results.
Tests: TestLoopExpressionScoping (genexp/comprehension/lambda across
eval_output/run_update/eval_condition/run_initialization, plus no
`__builtins__` leak) and two main-loop tests (state-emit error is reported
not fatal; end channel holds the region on a reported error).
Repoints LoopIntegrationSpec (added on main by apache#5700) from the removed org.apache.texera.workflow.LogicalLink to the shared compiler model — the one main-side reference to the symbols this branch deletes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What changes were proposed in this PR?
Adds two operators — Loop Start and Loop End — that let users write a for-loop over the rows of a pandas table inside a visual workflow. The user supplies four small Python snippets:
initializationi = 0outputtable.iloc[i]— the row passed into the loop body each iterationupdatei += 1conditioni < len(table)— keep looping while this is trueOperators between Loop Start and Loop End are the loop body and run once per iteration. While
conditionis true the runtime starts another iteration; when it turns false, downstream operators run on the accumulated output.How an iteration works
The Loop End → Loop Start arrow is not a graph edge — the region DAG stays acyclic. When an iteration ends, Loop End (1) sends a
jump_to_operator_regionDCM asking the coordinator to schedule the Loop Start region again, and (2) writes the next iteration's state into the iceberg state channel Loop Start reads from (the same cross-region channel from #4490, reused here as the back-edge). Details in the code comments onMainLoop._jump_to_loop_startand_process_state_frame.The write address is the same every iteration, so it is config, not per-iteration data: the scheduler resolves each Loop Start's input-port state URI and ships it to workers at setup (
InitializeExecutorRequest.loopStartStateUris); a Loop End selects its entry by theloop_start_idstamped on the state it consumes. This is why the State format is only 3 columns (content/loop_counter/loop_start_id, from #5900) — see the proto field doc andWorkflowExecutionManager.loopStartStateUrisfor the rationale. Nested loops work with no extra machinery: aloop_counteron the state (+1 through an inner Loop Start, −1 through an inner Loop End, consumed at 0) routes each state to its owning loop — seeMainLoop._process_state_frame.State serialization
The loop state = the user's variables + the input
table. Variables ride the Statecontentcolumn as JSON (rawbytesbase64-encoded); thetableis serialized as an Apache Arrow IPC stream, not pickle — the state is persisted to iceberg and read back by another worker, sopickle.loadswould be a remote-code-execution surface. Arrow's nested-type slowness doesn't apply here (Texera tables are flat) and the channel is low-volume, so safety wins. Seetable_to_ipc_bytesincore/models/table.py.File map
loop/{LoopOpDesc,LoopStartOpDesc,LoopEndOpDesc}.scalaisLoopStart, Loop End sets output-portreuseStoragecore/models/operator.pyLoopStartOperator/LoopEndOperatorthe generated code extends; guarded expression evalPhysicalOp.scalaisLoopStart(loop-back resolution) +requiresMaterializedExecution(forces whole-plan MATERIALIZED) markersWorkflowExecutionManager.scala,controlcommands.proto,RegionExecutionManager.scala,initialize_executor_handler.py,context.py{Loop Start id → input-port state URI}from the final schedule and ship it to workers at setupMainLoop(_process_state_frame,_jump_to_loop_start)loop_counterbookkeeping, nested pass-through, and the jump-DCM + state-write on Loop End completionOutputPort.reuseStorage+RegionExecutionManager;OutputManager.reset_output_storageLoopStart.png,LoopEnd.pngAny related issues, documentation, discussions?
Closes #4442. Builds on #4490 (cross-region state materialization) and #5085 (
DocumentFactory.documentExists).Prerequisites split out of this PR per @Xiao-zhen-Liu's request, all merged: #5706 (worker-id helper), #5707 (state test harness), #5900 (the 3-column State format, dormant on main until this PR activates it).
How was this PR tested?
test_main_loop.py(loop-counter routing, nested pass-through, the full_jump_to_loop_startcontract incl. DCM-before-write ordering and the fail-loud missing-URI case, andcomplete()'s error reporting for condition / loop-back-write failures);test_loop_operators.py(operator base classes, guarded eval, the generated-code shape via a base64 round-trip, reserved-tablecollision raising, a multi-iteration loop to completion);test_output_manager.py(reset_output_storagecontract);test_initialize_executor_handler.py(proto field →Context);LoopStartOpDescSpec/LoopEndOpDescSpec(codegen, ports, JSON round-trip, flag pins).@IntegrationTest, CI) —LoopIntegrationSpec: single loop (3 iterations) and nested 3×3 loop (9 inner iterations), asserted via materialized iceberg row counts.scalafmtCheckAll+scalafixAll --checkand Pythonruff formatclean; full Python unit suite passes.Manual workflows
Input for both is a 3-row table from
TextInput("1\n2\n3"); each condition isi < len(table).TextInput → LoopStart → LoopEndTextInput → OuterStart → InnerStart → InnerEnd → OuterEndBasic Loop:

Nested Loop:

Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude (Opus 4.7, Opus 4.8) in compliance with ASF.