Repository navigation
refactor(agent-service): model execution results as canonical summary types - #6009
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6009 +/- ##
============================================
+ Coverage 91.75% 96.74% +4.99%
============================================
Files 1281 436 -845
Lines 56777 28821 -27956
Branches 7454 3937 -3517
============================================
- Hits 52096 27884 -24212
+ Misses 2801 503 -2298
+ Partials 1880 434 -1446
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 392 | 0.239 | 24,977/40,305/40,305 us | 🔴 +34.4% / 🔴 +162.2% |
| 🔴 | bs=100 sw=10 sl=64 | 795 | 0.485 | 124,362/157,297/157,297 us | 🔴 +11.0% / 🔴 +47.1% |
| 🔴 | bs=1000 sw=10 sl=64 | 878 | 0.536 | 1,133,885/1,234,278/1,234,278 us | 🔴 +8.3% / 🔴 +21.0% |
Baseline details
Latest main 6fdfe19 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 392 tuples/sec | 402 tuples/sec | 776.68 tuples/sec | -2.5% | -49.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.245 MB/s | 0.474 MB/s | -2.4% | -49.6% |
| bs=10 sw=10 sl=64 | p50 | 24,977 us | 24,611 us | 12,599 us | +1.5% | +98.3% |
| bs=10 sw=10 sl=64 | p95 | 40,305 us | 29,994 us | 15,370 us | +34.4% | +162.2% |
| bs=10 sw=10 sl=64 | p99 | 40,305 us | 29,994 us | 18,723 us | +34.4% | +115.3% |
| bs=100 sw=10 sl=64 | throughput | 795 tuples/sec | 806 tuples/sec | 994.07 tuples/sec | -1.4% | -20.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.485 MB/s | 0.492 MB/s | 0.607 MB/s | -1.4% | -20.1% |
| bs=100 sw=10 sl=64 | p50 | 124,362 us | 122,406 us | 100,754 us | +1.6% | +23.4% |
| bs=100 sw=10 sl=64 | p95 | 157,297 us | 141,719 us | 106,898 us | +11.0% | +47.1% |
| bs=100 sw=10 sl=64 | p99 | 157,297 us | 141,719 us | 113,210 us | +11.0% | +38.9% |
| bs=1000 sw=10 sl=64 | throughput | 878 tuples/sec | 917 tuples/sec | 1,027 tuples/sec | -4.3% | -14.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.536 MB/s | 0.56 MB/s | 0.627 MB/s | -4.3% | -14.5% |
| bs=1000 sw=10 sl=64 | p50 | 1,133,885 us | 1,090,660 us | 979,814 us | +4.0% | +15.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,234,278 us | 1,139,977 us | 1,019,689 us | +8.3% | +21.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,234,278 us | 1,139,977 us | 1,049,906 us | +8.3% | +17.6% |
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,510.52,200,128000,392,0.239,24976.86,40305.48,40305.48
1,100,10,64,20,2514.86,2000,1280000,795,0.485,124362.14,157297.35,157297.35
2,1000,10,64,20,22791.94,20000,12800000,878,0.536,1133884.88,1234278.44,1234278.4489eb9e9 to
84022c9
Compare
43166b9 to
66d9785
Compare
8e8a9eb to
392f4de
Compare
3ef4ff8 to
8cae820
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the sync-execution wire contract across the Amber backend, agent-service, and frontend so operator execution results are represented as structured summaries (OperatorExecutionSummary / WorkflowExecutionSummary), sampled rows become { rowIndex, tuple }, and execution errors reuse the shared WorkflowFatalError shape already used by compilation errors.
Changes:
- Redesign sync-execution DTOs: replace flat per-operator result payloads with
OperatorExecutionSummary(state, errors, optional result summary, optional console messages) andWorkflowExecutionSummary. - Switch sampled output rows from embedded metadata fields (e.g.,
__row_index__) to an explicitSampleRow { rowIndex, tuple }shape and update formatting/mapping accordingly. - Align execution error reporting with the engine/compilation
WorkflowFatalErrorstructure across backend + agent-service + frontend consumers, with updated unit tests.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/workspace/service/agent/agent.service.ts | Updates frontend REST typing/storage to consume OperatorExecutionSummary (and new SampleRow/result mode/state enums). |
| frontend/src/app/workspace/service/agent/agent.service.spec.ts | Updates unit tests for the new operator-results REST response shape. |
| frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts | Switches operator result accessors to resultSummary.sampleTuples + resultSummary.resultMode. |
| frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.html | Wires new inputs (sampleTuples, resultMode) into the agent interaction UI. |
| frontend/src/app/workspace/component/agent/agent-interaction/agent-interaction.component.ts | Updates UI logic to render sample rows with explicit rowIndex and visualization via resultMode. |
| frontend/src/app/workspace/component/agent/agent-interaction/agent-interaction.component.html | Updates template to display a leading Row column and read values from SampleRow.tuple. |
| frontend/src/app/workspace/component/agent/agent-interaction/agent-interaction.component.scss | Removes column-statistics styling tied to removed statistics rendering. |
| frontend/src/app/workspace/component/agent/agent-interaction/agent-interaction.component.spec.ts | Adds coverage for the new table/ellipsis/visualization rendering behavior with SampleRow. |
| amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala | Refactors sync-execution DTOs and result sampling/truncation into helper methods; emits WorkflowFatalError consistently. |
| amber/src/test/scala/org/apache/texera/web/resource/SyncExecutionResourceSpec.scala | Adds extensive unit tests for extracted pure helpers (error classification, sampling/truncation, summary assembly). |
| agent-service/src/types/execution.ts | Redefines execution result/error interfaces (states, SampleRow, WorkflowFatalError, operator/workflow summaries). |
| agent-service/src/server.ts | Returns visible operator summaries verbatim (now OperatorExecutionSummary) from /operator-results. |
| agent-service/src/server.spec.ts | Updates server route tests to assert the new operator-results payload shape. |
| agent-service/src/api/compile-api.ts | Re-exports shared WorkflowFatalError from the centralized execution types module. |
| agent-service/src/agent/workflow-result-state.ts | Stores OperatorExecutionSummary in result state and updates accessor typing. |
| agent-service/src/agent/workflow-result-state.spec.ts | Updates tests for visibility/ancestor-walk semantics with new summary shape. |
| agent-service/src/agent/tools/workflow-execution-tools.ts | Updates execute-and-format tool to consume WorkflowExecutionSummary and derive warnings/errors from console/error summaries. |
| agent-service/src/agent/tools/workflow-execution-tools.spec.ts | Adds new unit tests for execution tool behavior under the redesigned model. |
| agent-service/src/agent/tools/tools-utility.ts | Centralizes “operator failed” detection + TSV formatting for SampleRow[]; derives warnings from console messages. |
| agent-service/src/agent/tools/tools-utility.spec.ts | Updates tests for operator error extraction and header handling. |
| agent-service/src/agent/tools/result-formatting.ts | Updates formatting of stored results to use SampleRow[] and new visualization/result mode semantics. |
| agent-service/src/agent/tools/result-formatting.spec.ts | Updates tests for new summary fields, warning derivation, visualization stripping, and row gap handling. |
| agent-service/src/agent/texera-agent.ts | Adjusts DAG-result formatting call signature after input-shape removal. |
| agent-service/src/agent/texera-agent.spec.ts | Adds basic coverage for formatted DAG results using the new summary shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
179fb14 to
aefa333
Compare
Yicong-Huang
left a comment
There was a problem hiding this comment.
This PR is tooo large to review. Please keep the size smaller!!! I left comments on key files.
- amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala
- agent-service/src/types/execution.ts
b97d2a9 to
8fd4b4b
Compare
Per review, reuse the engine Tuple instead of a bespoke SampleRow struct.
OperatorResultSummary.sampleTuples is now List[(Int, Tuple)] on the wire
(serialized as [index, {schema, fields}]).
Sampled rows are truncated (long cells clipped, binary replaced with
previews), so their values no longer match the operator's real column
types; each row is wrapped in a Tuple with a synthetic all-STRING schema
over its columns (toStringTuple). Sampling/size-estimation still runs on
the processed JSON node internally (SampledRow); the node->Tuple conversion
happens at the DTO boundary. A serialization test locks the
[index, {schema, fields}] wire contract.
agent-service and frontend mirror the Tuple/Schema/Attribute types and read
values via schema-order fields (tupleColumns / tupleToRecord).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename OperatorResultSummary.tuplesCount -> totalTuplesCount (the total row count before truncation) across Scala, agent-service, and frontend, and trim the interface comments to note only that sample tuples carry their original row index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the sync-execution backend contract unchanged and normalize its legacy response at the agent-service HTTP boundary. Update the frontend-facing fixtures to use the canonical tuple representation.
Rebase onto main brought in specs written against the pre-redesign execution model. Both sides had created texera-agent.spec.ts, workflow-execution-tools.spec.ts and agent-interaction.component.spec.ts; this restores main's coverage inside the branch's new-model versions: - texera-agent.spec.ts: main's step-tree/settings/lifecycle suite plus the branch's DAG result-formatting tests; cached results are seeded as OperatorExecutionSummary values. - workflow-execution-tools.spec.ts: port main's guard, request construction and rendering tests; input-port shapes are asserted as no longer rendered, warnings derive from WARNING-titled console messages, a non-array legacy result is rejected by the adapter. - agent-interaction.component.spec.ts: port main's agent-list, canSend and sendFeedback tests onto the sampleTuples/resultMode inputs; the removed column-statistics tests are dropped. - context-utils.spec.ts: build the compilation error with the shared WorkflowFatalError shape. - workflow-editor.component.spec.ts: seed operatorExecutionSummaries$ and assert visualization via the SetSnapshotMode result mode. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The legacy sync-execution adapter validated console messages against exactly today's four ConsoleMessageType values and required inputTuples, so an additive backend change (a new proto value, or ScalaPB's "UNRECOGNIZED") rejected the whole response and turned a successful run into an Error, although nothing reads msgType. Validate msgType as text and drop messages of an unrecognized type, and validate only the fields the adapter reads; zod strips inputTuples, inputPortShapes, displayedRows, truncated and warnings. resultMode stays strict because it decides visualization redaction.
The result Tuple's Schema carried its own Attribute type with a bare string attributeType, duplicating SchemaAttribute (agent-service types/workflow.ts, frontend workflow-compiling.interface.ts), which types attributeType as the engine's attribute-type union. Define Schema over the existing SchemaAttribute in both apps, drop the duplicate Attribute, and move the tuple comment onto the Tuple it describes.
The DAG context (formatOperatorResult) and the execute tool each ran the same redact -> headers -> TSV -> shape line -> warnings sequence, so any change to what the agent sees had to be written twice. Extract it into formatOperatorOutput in tools-utility.ts; the execute tool passes its character limit, which moves the TSV head/tail truncation into a helper. Rendered output is unchanged, and the tool still returns "(no result data)" before caching any result. sampleTuples is a typed array out of the validated adapter, so drop the leftover Array.isArray guards and the spec that cast a non-array in only to reach them.
executeOperatorAndFormat declared an onResultLegacy callback that it never invokes and no caller supplies.
getOperatorErrorText reads only errorMessages, so filter(Boolean) drops fatal errors with an empty message; it never sees console ERRORs.
GET /agents/:id/operator-results returned each operator's full
OperatorExecutionSummary, shipping console-message bodies (stack traces
included), state and errorMessages that its only consumer, the operator
popover, never reads; the frontend mirror already declares just
resultSummary. Return { resultSummary } per operator so the payload
matches what the frontend declares.
The executeOperator tool description still said its result includes the input tables' shapes, but the result now reports the output shape only; the agent runs every operator it adds or modifies, so upstream output shapes are already in its context.
The fetchOperatorResults TSDoc still said "operator result summaries" after the stream was renamed to operatorExecutionSummaries$.
apache#9252 added tests on main for jsonToTableFormat, formatInputOutputMetadata and the charLimit truncation of formatOperatorResult. This branch replaces those helpers with formatSampleTuplesAsTsv and formatOperatorOutput, so the TSV and truncation cases now run against them. The input-shape cases go with the input-shape line this branch removes.
formatSampleTuplesAsTsv and formatOperatorOutput live in tools-utility.ts, so their tests now sit in tools-utility.spec.ts beside the tests of the sibling helpers.
a98b158 to
ff15542
Compare
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 0 resolved · 0 open · 0 new (0 new = 0 newly introduced · 0 late catches)
Verification trace
This round is a rebase onto current main, so the review checked that the rebase moved no PR code: the sorted added lines of the PR diff are identical at the old base (8b0fb16) and the new base (ed53531) across all 27 changed files, and the 6 files whose content differs differ only by absorbed main lines. The four agent-service specs pass at the new head: 142 pass, 0 fail.
|
@Yicong-Huang can you take another look ? I want to merge this one soon |
|
@bobbai00 can we merge this? Do you need an additional reviewer? |
What changes were proposed in this PR?
Redefines the execution result and error types shared by
agent-serviceand the frontend.SyncExecutionResourceis unchanged:adaptLegacySyncExecutionResultconverts its legacy response in one place.OperatorExecutionSummaryreplacesOperatorInfo, keeping state, errors, result summary and console messages separate;WorkflowExecutionSummaryreplacesSyncExecutionResult.WorkflowExecutionStateis the outcome; a legacysuccess=false, state=CompletedbecomesFailed.WebOutputModeunion, includingSetDeltaMode.[rowIndex, Tuple]pairs, not records with a__row_index__marker.WorkflowFatalError.GET /agents/:id/operator-resultsreturns onlyresultSummary, the field the UI reads.executeOperatoroutput drops theInput operator(table shape)line.Any related issues, documentation, discussions?
Closes #5750
Part of #5747
Supersedes #5927
How was this PR tested?
agent-service:bun run typecheck,bun run format:check,bun test.frontend:yarn format:ci,yarn build:ci, and the agent-interaction, agent.service and workflow-editor specs.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8 (1M context), Claude Fable 5, Codex, Claude Code (Claude Opus 5.5)