Copilot commented on code in PR #5262: URL: https://github.com/apache/texera/pull/5262#discussion_r3480241571
########## frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts: ########## @@ -0,0 +1,212 @@ +/** + * 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 { AppSettings } from "../../../common/app-setting"; +import { Notebook, NotebookMigrationLLM } from "./migration-llm"; +import { HttpClient, HttpHeaders } from "@angular/common/http"; +import { NotificationService } from "src/app/common/service/notification/notification.service"; +import { GuiConfigService } from "../../../common/service/gui-config.service"; +import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; +import { catchError, firstValueFrom, map, Observable, of } from "rxjs"; + +interface LiteLLMModel { + id: string; + object: string; + created: number; + owned_by: string; +} + +interface LiteLLMModelsResponse { + data: LiteLLMModel[]; + object: string; +} + +interface MappingContent { + cell_to_operator: { [key: string]: any }; + operator_to_cell: { [key: string]: any }; +} + +interface StoreNotebookResponse { + success: boolean; + message: string; +} + +@Injectable({ + providedIn: "root", +}) +export class NotebookMigrationService { + private mapping: { [key: string]: MappingContent } = {}; + + constructor( + private http: HttpClient, + private notificationService: NotificationService, + private config: GuiConfigService, + private workflowUtilService: WorkflowUtilService + ) {} + + private get enabled(): boolean { + return this.config.env.pythonNotebookMigrationEnabled; + } + + public getAvailableModels(): Observable<{ name: string }[]> { + if (!this.enabled) return of([]); + return this.http.get<LiteLLMModelsResponse>(`${AppSettings.getApiEndpoint()}/models`).pipe( + map(response => + response.data.map(model => ({ + name: model.id, + })) + ), + catchError((err: unknown) => { + console.error("Failed to fetch models", err); + return of([]); + }) + ); + } + + public async sendToAIGenerateWorkflow(notebookContent: Notebook, modelType: string) { + if (!this.enabled) throw new Error("Notebook migration feature is disabled"); + const migrationLLM = new NotebookMigrationLLM(this.config, this.workflowUtilService); + // initialize() defaults to the user's Texera JWT via AuthService.getAccessToken(). + migrationLLM.initialize(modelType); + + const isValid = await migrationLLM.verifyConnection(); + if (!isValid) { + throw new Error("Unable to authenticate with or reach the LLM backend"); + } + + try { + const result = await migrationLLM.convertNotebookToWorkflow(notebookContent); + const parsedResult = JSON.parse(result); + const workflowContent = parsedResult.workflowJSON; + const mappingContent = parsedResult.workflowNotebookMapping; + return { workflowContent, mappingContent }; + } catch (error) { + console.error("Error converting notebook:", error); + throw error; + } finally { + migrationLLM.close(); + } + } Review Comment: `sendToAIGenerateWorkflow` doesn’t match the PR description/issue scope: it has no `apiKey` (or access-token override) parameter, and `verifyConnection()` runs outside the `try/finally`, so `close()` is skipped on verification failure/throw even though the intended lifecycle is initialize → verify → convert → close (in finally). Wrapping verification inside the `try` and adding an optional key/token parameter keeps the lifecycle consistent and allows callers to pass a user-provided key when needed. ########## frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts: ########## @@ -0,0 +1,212 @@ +/** + * 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 { AppSettings } from "../../../common/app-setting"; +import { Notebook, NotebookMigrationLLM } from "./migration-llm"; +import { HttpClient, HttpHeaders } from "@angular/common/http"; +import { NotificationService } from "src/app/common/service/notification/notification.service"; +import { GuiConfigService } from "../../../common/service/gui-config.service"; +import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; +import { catchError, firstValueFrom, map, Observable, of } from "rxjs"; + +interface LiteLLMModel { + id: string; + object: string; + created: number; + owned_by: string; +} + +interface LiteLLMModelsResponse { + data: LiteLLMModel[]; + object: string; +} + +interface MappingContent { + cell_to_operator: { [key: string]: any }; + operator_to_cell: { [key: string]: any }; +} Review Comment: `MappingContent` is currently typed with `{ [key: string]: any }`, which loses the stronger (and already-used) mapping shape. The underlying LLM code produces `Record<string, string[]>` mappings (see `CombinedMapping` in `migration-llm.ts`), so tightening this type improves safety for cache and persistence callers. ########## frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts: ########## @@ -0,0 +1,328 @@ +/** + * 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 { TestBed } from "@angular/core/testing"; +import { NotebookMigrationService } from "./notebook-migration.service"; +import { HttpClient } from "@angular/common/http"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +import { NotificationService } from "src/app/common/service/notification/notification.service"; +import { GuiConfigService } from "src/app/common/service/gui-config.service"; +import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; +import { firstValueFrom, throwError } from "rxjs"; + +// NotebookMigrationLLM is constructed inside sendToAIGenerateWorkflow. Mock the +// whole module so the LLM lifecycle is driven by `mockLLM` instead of real +// network / AI-SDK calls. vi.hoisted lets the mock instance be referenced from +// the (hoisted) vi.mock factory. +const { mockLLM } = vi.hoisted(() => ({ + mockLLM: { + initialize: vi.fn(), + verifyConnection: vi.fn(), + convertNotebookToWorkflow: vi.fn(), + close: vi.fn(), + }, +})); +vi.mock("./migration-llm", () => ({ + NotebookMigrationLLM: vi.fn(() => mockLLM), +})); + +describe("NotebookMigrationService", () => { + let service: NotebookMigrationService; + let httpMock: HttpTestingController; + let mockNotificationService: { success: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn> }; + // Mutable so individual describe blocks can flip the flag mid-spec by + // reassigning `mockGuiConfigService.env.pythonNotebookMigrationEnabled`. + // The service stores a reference to this object, so mutations are observed + // on the next read of `this.enabled`. + let mockGuiConfigService: { env: { pythonNotebookMigrationEnabled: boolean } }; + + beforeEach(() => { + mockNotificationService = { + success: vi.fn(), + error: vi.fn(), + }; + mockGuiConfigService = { env: { pythonNotebookMigrationEnabled: true } }; + + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [ + NotebookMigrationService, + { provide: NotificationService, useValue: mockNotificationService }, + { provide: GuiConfigService, useValue: mockGuiConfigService }, + // Stub so the real WorkflowUtilService (and its OperatorMetadataService, + // which fires GET /api/resources/operator-metadata on construction) is + // never built. The service only passes it to NotebookMigrationLLM, which + // no test exercises. + { provide: WorkflowUtilService, useValue: {} }, + ], + }); + + service = TestBed.inject(NotebookMigrationService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + vi.restoreAllMocks(); + }); + + // getAvailableModels + it("should fetch and map available models", async () => { + const mockResponse = { + data: [ + { id: "gpt-4", object: "", created: 0, owned_by: "" }, + { id: "gpt-3.5", object: "", created: 0, owned_by: "" }, + ], + object: "", + }; + + const promise = firstValueFrom(service.getAvailableModels()); + + const req = httpMock.expectOne(req => req.url.includes("/models")); + expect(req.request.method).toBe("GET"); + req.flush(mockResponse); + + const models = await promise; + expect(models.length).toBe(2); + expect(models[0].name).toBe("gpt-4"); + }); + + it("should return empty array on getAvailableModels error", async () => { + const promise = firstValueFrom(service.getAvailableModels()); + + const req = httpMock.expectOne(req => req.url.includes("/models")); + req.error(new ErrorEvent("Network error")); + + expect(await promise).toEqual([]); + }); + + // sendNotebookToJupyter + it("should send notebook successfully and return 1", async () => { + const mockNotebook: any = { cells: [] }; + + const promise = service.sendNotebookToJupyter(mockNotebook); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); + + expect(req.request.method).toBe("POST"); + + req.flush({ success: true }); + + const result = await promise; + + expect(result).toBe(1); + expect(mockNotificationService.success).toHaveBeenCalled(); + }); + + it("should handle error when sending notebook and return 0", async () => { + const mockNotebook: any = { cells: [] }; + + const promise = service.sendNotebookToJupyter(mockNotebook); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); + + req.error(new ErrorEvent("Server error")); + + const result = await promise; + + expect(result).toBe(0); + expect(mockNotificationService.error).toHaveBeenCalled(); + }); + + it("includes the Error message in the failure toast when an Error is thrown", async () => { + // HttpTestingController's req.error yields an HttpErrorResponse (not an Error + // instance), so spy on http.post directly to exercise the `error instanceof + // Error` branch. No request reaches the testing backend, so verify() stays happy. + vi.spyOn(TestBed.inject(HttpClient), "post").mockReturnValue(throwError(() => new Error("network down"))); + + const result = await service.sendNotebookToJupyter({ cells: [] } as any); + + expect(result).toBe(0); + expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); + }); + + // jupyter URL methods (HttpClient so the JwtModule interceptor attaches the auth token) + it("should return Jupyter URL when the request succeeds", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + expect(req.request.method).toBe("GET"); + req.flush({ success: true, url: "http://jupyter"; }); + + expect(await promise).toBe("http://jupyter";); + }); + + it("should return null when the Jupyter URL request fails", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + req.flush({ success: false }, { status: 500, statusText: "Server Error" }); + + expect(await promise).toBeNull(); + }); + + it("should return null when the Jupyter URL response is 200 but unsuccessful", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + req.flush({ success: false }); + + expect(await promise).toBeNull(); + }); + + it("should return iframe URL when the request succeeds", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + expect(req.request.method).toBe("GET"); + req.flush({ success: true, url: "http://iframe"; }); + + expect(await promise).toBe("http://iframe";); + }); + + it("should return null when the iframe URL request fails", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + req.flush({ success: false }, { status: 500, statusText: "Server Error" }); + + expect(await promise).toBeNull(); + }); + + it("should return null when the iframe URL response is 200 but unsuccessful", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + req.flush({ success: false }); + + expect(await promise).toBeNull(); + }); + + // mapping logic + it("should set and get mapping", () => { + const mockMapping: any = { + cell_to_operator: { a: 1 }, + operator_to_cell: { b: 2 }, + }; + + service.setMapping("test", mockMapping); + + expect(service.hasMapping("test")).toBe(true); + expect(service.getMapping("test")).toEqual(mockMapping); + }); + + it("should delete mapping", () => { + service.setMapping("test", { cell_to_operator: {}, operator_to_cell: {} }); + + service.deleteMapping("test"); + + expect(service.hasMapping("test")).toBe(false); + }); + + // storeNotebookAndMapping + it("should call storeNotebookAndMapping API", () => { + service.storeNotebookAndMapping(1, 1, {}, {}).subscribe(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); + + expect(req.request.method).toBe("POST"); + }); Review Comment: This test leaves the POST request unflushed, so `httpMock.verify()` in `afterEach` will fail due to an outstanding request. Flush a mock response (and optionally await it) to ensure the request completes. -- 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]
