Copilot commented on code in PR #8637: URL: https://github.com/apache/texera/pull/8637#discussion_r4328979222
########## frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.ts: ########## @@ -0,0 +1,158 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component, Input } from "@angular/core"; +import { AsyncPipe, NgFor, NgIf } from "@angular/common"; +import { BehaviorSubject, Observable, combineLatest } from "rxjs"; +import { map } from "rxjs/operators"; +import { NzAlertComponent } from "ng-zorro-antd/alert"; +import { NzSpinComponent } from "ng-zorro-antd/spin"; +import { NzTagComponent } from "ng-zorro-antd/tag"; +import { NzButtonComponent } from "ng-zorro-antd/button"; +import { NzWaveDirective } from "ng-zorro-antd/core/wave"; +import { NzIconDirective } from "ng-zorro-antd/icon"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service"; +import { WorkflowConsoleService } from "../../../service/workflow-console/workflow-console.service"; +import { + AiWorkflowFixerService, + FixConfidence, + FixState, + IDLE_STATE, + UNSUPPORTED_MESSAGE, +} from "./ai-workflow-fixer.service"; + +/** + * Result-panel frame that asks the LLM to fix the failed operator. + * + * The analysis is started by an explicit button: ResultPanelComponent rebuilds + * every frame on each re-render (clearResultPanel), so analysing on init would + * fire a model request per re-render. For the same reason all state lives in + * AiWorkflowFixerService and this component keeps none. + */ +@Component({ + selector: "texera-ai-fix-frame", + templateUrl: "./ai-fix-frame.component.html", + styleUrls: ["./ai-fix-frame.component.scss"], + imports: [ + NgIf, + NgFor, + AsyncPipe, + NzAlertComponent, + NzSpinComponent, + NzTagComponent, + NzButtonComponent, + NzWaveDirective, + NzIconDirective, + ], +}) +export class AiFixFrameComponent { + private readonly operatorIdSubject = new BehaviorSubject<string | undefined>(undefined); + + @Input() + set operatorId(value: string | undefined) { + this.operatorIdSubject.next(value); + } + + get operatorId(): string | undefined { + return this.operatorIdSubject.value; + } + + public readonly state$: Observable<FixState>; + public readonly unsupportedMessage = UNSUPPORTED_MESSAGE; + + constructor( + private aiWorkflowFixerService: AiWorkflowFixerService, + private executeWorkflowService: ExecuteWorkflowService, + private workflowActionService: WorkflowActionService, + private workflowCompilingService: WorkflowCompilingService, + private workflowConsoleService: WorkflowConsoleService + ) { + // The service is a root singleton holding one suggestion, but a failed pipeline can + // put an AI Fix tab on several operators at once. Showing another operator's + // suggestion here would let Apply patch an operator the user is not looking at, so a + // frame only ever renders state that names its own operator. + this.state$ = combineLatest([aiWorkflowFixerService.getState$(), this.operatorIdSubject]).pipe( + map(([state, operatorId]) => + state.operatorId === operatorId && state.workflowId === this.workflowActionService.getOpenWorkflowId() + ? state + : IDLE_STATE + ) + ); + } + + /** Collects the operator's error, code, schema and config, then asks for a fix. */ + public onAnalyze(): void { + const operatorId = this.operatorId; + if (!operatorId) { + return; + } + const errorMessage = this.rawErrorMessage(operatorId); + if (!errorMessage) { + return; + } + const properties = this.workflowActionService.getTexeraGraph().getOperator(operatorId).operatorProperties ?? {}; + void this.aiWorkflowFixerService.analyzeError( + operatorId, + errorMessage, + this.workflowCompilingService.getOperatorInputSchemaMap(operatorId), + properties.code as string | undefined, + properties + ); + } + + public onApply(): void { + this.aiWorkflowFixerService.applyFix(); + } Review Comment: `applyFix()` is async; calling it without `await`/`void` can violate common `no-floating-promises` linting and can also drop unhandled rejections. Consider either making `onApply` `async` and awaiting, or prefixing the call with `void` (as you already do in `onAnalyze`) so the intent is explicit. ########## frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts: ########## @@ -258,29 +258,36 @@ export class ExecuteWorkflowService { return []; } + /** + * Returns whether the run actually started, so a caller that reports on it does not have + * to repeat the refusal conditions. The refusals show their own toast either way; callers + * that only fire and forget can keep ignoring the result. + */ public executeWorkflowWithEmailNotification( executionName: string, emailNotificationEnabled: boolean, targetOperatorId?: string - ): void { + ): boolean { Review Comment: The docstring says “run actually started”, but the implementation returns `true` immediately after passing local refusal checks (and before any server acknowledgement). To avoid misleading callers/UI, please adjust the documentation (and ideally naming) to reflect “request submitted / not refused by client-side preconditions”, or change the implementation to return `true` only after a definitive “started/accepted” signal if such a signal exists in `sendExecutionRequest`. ########## frontend/src/app/workspace/component/result-panel/ai-fix/ai-fix-frame.component.scss: ########## @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +:host { + display: block; + width: 100%; + padding: 8px; +} + +.ai-fix-status { + align-items: center; + display: flex; + gap: 8px; + padding: 8px 0; +} + +.ai-fix-section-title { + font-weight: bold; + margin-top: 8px; +} + +.ai-fix-explanation { + margin: 4px 0 8px; + user-select: text; +} + +.ai-fix-field { + margin-bottom: 4px; + user-select: text; +} + +/* Same monospace treatment the other result-panel frames use for code. */ +.ai-fix-diff { + border: 1px solid #e8e8e8; + border-radius: 2px; + font-family: monaco, serif; Review Comment: `serif` is a poor fallback for code/diff rendering and can change alignment; `monospace` is the appropriate generic fallback for fixed-width display. Consider switching the fallback to `monospace` (or using an existing project-wide monospace stack, if one exists). ########## frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts: ########## @@ -258,29 +258,36 @@ export class ExecuteWorkflowService { return []; } + /** + * Returns whether the run actually started, so a caller that reports on it does not have + * to repeat the refusal conditions. The refusals show their own toast either way; callers + * that only fire and forget can keep ignoring the result. + */ public executeWorkflowWithEmailNotification( executionName: string, emailNotificationEnabled: boolean, targetOperatorId?: string - ): void { + ): boolean { const logicalPlan = ExecuteWorkflowService.getLogicalPlanRequest( this.workflowActionService.getTexeraGraph(), targetOperatorId ); const settings = this.workflowActionService.getWorkflowSettings(); if (this.refuseToRunOnUnavailableUnit()) { - return; + return false; } if (this.refuseToRunWithoutWarehouse()) { - return; + return false; } this.resetExecutionState(); this.workflowStatusService.resetStatus(); this.sendExecutionRequest(executionName, logicalPlan, settings, emailNotificationEnabled); + return true; } Review Comment: The docstring says “run actually started”, but the implementation returns `true` immediately after passing local refusal checks (and before any server acknowledgement). To avoid misleading callers/UI, please adjust the documentation (and ideally naming) to reflect “request submitted / not refused by client-side preconditions”, or change the implementation to return `true` only after a definitive “started/accepted” signal if such a signal exists in `sendExecutionRequest`. ########## frontend/src/app/workspace/component/result-panel/ai-fix/ai-workflow-fixer.service.ts: ########## @@ -0,0 +1,481 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Injectable } from "@angular/core"; +import { BehaviorSubject, Observable, firstValueFrom } from "rxjs"; +import { filter, timeout } from "rxjs/operators"; +import { createOpenAI } from "@ai-sdk/openai"; +import { generateText, type ModelMessage } from "ai"; +import { AppSettings } from "../../../../common/app-setting"; +import { AuthService } from "../../../../common/service/user/auth.service"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service"; +import { OperatorPortSchemaMap } from "../../../types/workflow-compiling.interface"; +import { ExecutionState } from "../../../types/execute-workflow.interface"; +import { buildFixPrompt, withoutSecrets } from "./ai-fix-prompt"; +import { OperatorMetadataService } from "../../../service/operator-metadata/operator-metadata.service"; + +export type FixErrorType = "missing_column" | "type_error" | "null_error" | "model_not_found" | "unsupported"; +export type FixConfidence = "high" | "medium" | "low"; + +export interface SuggestedFix { + /** How many places Apply will touch; the diff only ever shows one -/+ pair. */ + occurrences?: number; + type: "code_change" | "property_change"; + original: string; + suggested: string; + explanation: string; + // Rendered as the confidence badge; `fieldName` names the patched property + // (property_change only) so applyFix knows which field to write. + confidence: FixConfidence; + fieldName?: string; +} + +export interface FixState { + operatorId: string; + /** + * The workflow the analysis belongs to. The operator id alone does not identify it: two + * imports of the same JSON carry the same ids, so a reply that arrived after the user + * switched workflows could be applied to the wrong one. + */ + workflowId?: number; + errorType: FixErrorType; + suggestedFix?: SuggestedFix; + // `applied_without_run`: the fix was written but the deployment refused to start a + // run (a warehouse is required and none is selected), so the panel must not claim one. + // `apply_failed` keeps the suggestion on screen: the analysis was fine, the write was + // refused, and re-analysing would spend another model call to produce the same fix. + status: "idle" | "analyzing" | "ready" | "applying" | "applied" | "applied_without_run" | "error" | "apply_failed"; + /** Why the write was refused, shown as-is when the status is `apply_failed`. */ + applyError?: string; +} + +// Model id as LiteLLM exposes it (bin/single-node/litellm-config.yaml), not the provider's +// id. A deployment whose proxy publishes a different alias has to change this: there is no +// GuiConfig field for it today, and adding one is a backend change. +export const AI_FIXER_MODEL = "claude-haiku-4.5"; +export const AI_FIXER_TIMEOUT_MS = 120_000; +export const UNLOCK_TIMEOUT_MS = 15_000; + +// A run that reached one of these is over, so anything the panel still shows predates it. +const TERMINAL_STATES: ReadonlySet<ExecutionState> = new Set([ + ExecutionState.Completed, + ExecutionState.Failed, + ExecutionState.Killed, + ExecutionState.Terminated, +]); +export const UNSUPPORTED_MESSAGE = "This error type is not yet supported for automatic fixing."; + +export const IDLE_STATE: FixState = { operatorId: "", errorType: "unsupported", status: "idle" }; + +// Matched against the exception line only, never the frame lines above it. +const PATTERNS: ReadonlyArray<[RegExp, FixErrorType]> = [ + [/KeyError:\s*['"][^'"]+['"]/, "missing_column"], + [/TypeError:\s*unsupported operand/i, "type_error"], + [/ValueError:[^\n]*\bNaN\b|NullPointerException/i, "null_error"], + [/ModelNotFound|\bmodel[^\n]*\b404\b|\b404\b[^\n]*\bmodel/i, "model_not_found"], +]; + +/** + * Replaces every occurrence, keeping each one at the indentation it was found at. + * + * The snippet the model matches on usually starts mid-line, after the existing indentation, + * so a multi-line replacement put its first line in the right place and every line after it + * at column 0 -- an IndentationError the moment the fix was "applied". Continuation lines now + * take the indentation of the line the match was found on, and keep whatever relative + * indentation the suggestion itself had. + */ +export function replaceAllIndented(code: string, original: string, suggested: string): { code: string; count: number } { + const parts = code.split(original); + const count = parts.length - 1; + if (count === 0 || !suggested.includes("\n")) { + return { code: parts.join(suggested), count }; + } + + const [head, ...rest] = suggested.split("\n"); + let out = parts[0]; + for (const tail of parts.slice(1)) { + // Whitespace since the last newline is what this occurrence sits at. + const lineStart = out.lastIndexOf("\n") + 1; + const indent = (out.slice(lineStart).match(/^[ \t]*/) ?? [""])[0]; + out += [head, ...rest.map(line => (line.length > 0 ? indent + line : line))].join("\n") + tail; + } + return { code: out, count }; +} + +/** Writes the model's string back in the type the field already held. */ +export function coerceToFieldType(currentValue: unknown, suggested: string): unknown { + // Arrays and objects are out of scope: the model replies with a string, and writing it + // into a structured field replaced e.g. an outputColumns array with the text "[]". + // Validating a replacement against the operator's schema is a bigger change than the four + // error shapes this panel handles need, so it refuses instead of guessing. + if (currentValue !== null && typeof currentValue === "object") { + throw new Error("this setting holds structured data, which an automatic fix does not edit"); + } + if (typeof currentValue === "number") { + const parsed = Number(suggested); + if (Number.isNaN(parsed)) { + throw new Error(`${suggested} is not a number`); + } + return parsed; + } + if (typeof currentValue === "boolean") { + if (suggested !== "true" && suggested !== "false") { + throw new Error(`${suggested} is not a boolean`); + } + return suggested === "true"; + } + return suggested; +} + +/** + * The exception a traceback ends on. + * + * Matching the whole message classified on whatever appeared first, which is wrong twice + * over: the frame lines carry paths and line numbers that read like patterns -- a path + * containing "model" above a `line 404` entry looks like a model 404 -- and a chained + * traceback reports the handled exception before the one that actually stopped the worker. + * Python puts the real exception last. + */ +export function finalExceptionLine(errorMessage: string): string { + const lines = errorMessage + .split("\n") + .map(line => line.trim()) + .filter(line => line.length > 0); + // Frame lines are the `File "...", line N` entries and the source line under them; the + // exception is the last line that is neither that nor the traceback header. + for (let i = lines.length - 1; i >= 0; i--) { + if (!/^File\s+"/.test(lines[i]) && !/^Traceback \(/.test(lines[i])) { + return lines[i]; + } + } + return errorMessage; +} + +export function classifyError(errorMessage: string): FixErrorType { + return PATTERNS.find(([pattern]) => pattern.test(finalExceptionLine(errorMessage)))?.[1] ?? "unsupported"; +} + +/** + * Holds the AI-fix loop for one failed operator: classify -> ask the LLM -> apply -> re-run. + * + * The state lives here rather than in the frame component because + * ResultPanelComponent.clearResultPanel() destroys and rebuilds the frames on + * every re-render, which would otherwise discard an in-flight suggestion. + */ +@Injectable({ providedIn: "root" }) +export class AiWorkflowFixerService { + private readonly stateSubject = new BehaviorSubject<FixState>(IDLE_STATE); + + constructor( + private workflowActionService: WorkflowActionService, + private executeWorkflowService: ExecuteWorkflowService, + private operatorMetadataService: OperatorMetadataService + ) { + // Without this the panel kept whatever it last showed: after Apply the green + // "re-running..." message stayed up, so a run that failed again brought the tab back + // claiming a re-run that had already finished. Resetting when the run ends rather than + // when it starts keeps that message truthful for as long as it is running. + this.executeWorkflowService + .getExecutionStateStream() + .pipe( + filter(event => TERMINAL_STATES.has(event.current.state)), + // applyFix kills a live execution before it writes; that kill must not wipe the + // state the apply is in the middle of publishing. + filter(() => this.stateSubject.getValue().status !== "applying") + ) + .subscribe(() => this.reset()); + } + + public getState$(): Observable<FixState> { + return this.stateSubject.asObservable(); + } + + // Numbers the analyses so a slow reply cannot overwrite a newer one. A frame whose + // operator does not own the state renders idle, so its Analyze button stays live while + // another operator is mid-analysis -- two requests can be in flight at once. + private analysisSeq = 0; + + public async analyzeError( + operatorId: string, + errorMessage: string, + inputSchemas: OperatorPortSchemaMap | undefined, + code: string | undefined, + properties: Readonly<Record<string, unknown>> + ): Promise<void> { + const errorType = classifyError(errorMessage); + const workflowId = this.workflowActionService.getOpenWorkflowId(); + const base: FixState = { operatorId, workflowId, errorType, status: "analyzing" }; + + // Numbered before the early return as well: this still supersedes whatever is in flight, + // and without the bump a slower analysis for another operator would land on top of it. + // Numbered before the early return as well: this still supersedes whatever is in flight, + // and without the bump a slower analysis for another operator would land on top of it. + const seq = ++this.analysisSeq; Review Comment: The comment is duplicated verbatim, which makes the block look like an accidental copy/paste. Please remove the duplicate lines to keep the reasoning concise. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
