mengw15 commented on code in PR #8767: URL: https://github.com/apache/texera/pull/8767#discussion_r4156723423
########## frontend/src/app/workspace/service/notebook-migration/folder-assembly.ts: ########## @@ -0,0 +1,359 @@ +/** + * 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 { ScriptSegmentation, splitScriptLines } from "./script-segmentation"; + +/** + * Assembles a folder of Python sources into the one numbered document the migration LLM sees. + * + * Converted as a single input, so the model reports line ranges as it does for a script and + * nothing downstream learns folders exist; a banner line before each file carries provenance. + * Lines split through the segmenter's own splitScriptLines, so assembly, numbering and slicing + * agree on what a line is. + */ + +// Never project code, matched per segment so a cache or vendored dependency is caught at any +// depth. Hidden segments are covered separately by the leading-dot rule. +const EXCLUDED_SEGMENTS = new Set(["__pycache__", "venv", "site-packages", "node_modules"]); + +// Build output at the root, but ordinary package names deeper in a tree (src/build/ can be real +// code). Dropping real code silently is worse than letting a build tree hit the caps. +const EXCLUDED_ROOT_DIRECTORIES = new Set(["build", "dist"]); + +// Non-Python files are named in the layout, never read. Capped because a folder can hold +// thousands of data files and the layout is orientation, not an inventory. +export const MAX_LISTED_OTHER_FILES = 20; + +/** + * Caps on one conversion, refused before any request is sent. Sized by the reply, not the prompt: + * the conversion asks for every line of code back and sets no output budget, so the model's output + * ceiling binds long before its context does. Raise once a larger folder has been measured. + */ +export const MAX_FOLDER_FILES = 100; +export const MAX_FOLDER_CHARACTERS = 60_000; Review Comment: This is the right thing to size the cap against, and it's the one number here I can't check from the code. "Sets no output budget, so the model's output ceiling binds" assumes the model gets to pick that ceiling, but nothing in the chain sends one and the model you ship requires it: - `sendPrompt` calls `callModelWithTimeout(this.messages)` with `maxOutputTokens` omitted, so the AI SDK leaves `max_tokens` out. The only caller that passes it is `verifyConnection`'s 10-token ping. - `LiteLLMProxyResource.proxyPost` forwards the body with `Entity.json(body)`, so nothing is added server-side. - Neither `bin/litellm-config.yaml` nor `bin/single-node/litellm-config.yaml` sets `max_tokens`, and single-node serves `claude-haiku-4.5` alone — an API that requires the parameter. So on the release deliverable the budget is whatever LiteLLM substitutes, not the model's own ceiling. 60,000 characters of Python is roughly 15-20k output tokens once it comes back as JSON-escaped strings, so a folder near the cap is the input most likely to meet that substituted value. Not asking you to lower the number — but the failure it produces is the one the pre-`6316fe0dc` version of this comment named: the user waits out the whole conversion and gets `Failed to parse LLM workflow response as JSON` from a truncated reply, with nothing saying it was truncated. If you've converted a folder near the cap end to end, saying so here settles it. Otherwise passing an explicit `maxOutputTokens` would at least turn an overrun into a length error instead of a parse error. ########## frontend/src/app/workspace/service/notebook-migration/folder-assembly.ts: ########## @@ -0,0 +1,359 @@ +/** + * 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 { ScriptSegmentation, splitScriptLines } from "./script-segmentation"; + +/** + * Assembles a folder of Python sources into the one numbered document the migration LLM sees. + * + * Converted as a single input, so the model reports line ranges as it does for a script and + * nothing downstream learns folders exist; a banner line before each file carries provenance. + * Lines split through the segmenter's own splitScriptLines, so assembly, numbering and slicing + * agree on what a line is. + */ + +// Never project code, matched per segment so a cache or vendored dependency is caught at any +// depth. Hidden segments are covered separately by the leading-dot rule. +const EXCLUDED_SEGMENTS = new Set(["__pycache__", "venv", "site-packages", "node_modules"]); + +// Build output at the root, but ordinary package names deeper in a tree (src/build/ can be real +// code). Dropping real code silently is worse than letting a build tree hit the caps. +const EXCLUDED_ROOT_DIRECTORIES = new Set(["build", "dist"]); + +// Non-Python files are named in the layout, never read. Capped because a folder can hold +// thousands of data files and the layout is orientation, not an inventory. +export const MAX_LISTED_OTHER_FILES = 20; + +/** + * Caps on one conversion, refused before any request is sent. Sized by the reply, not the prompt: + * the conversion asks for every line of code back and sets no output budget, so the model's output + * ceiling binds long before its context does. Raise once a larger folder has been measured. + */ +export const MAX_FOLDER_FILES = 100; +export const MAX_FOLDER_CHARACTERS = 60_000; + +// Pre-read guard for the character cap: UTF-8 never uses fewer bytes than characters, so a +// selection this far over cannot come in under it, and one huge file would otherwise hang the tab +// before the character cap, which needs the files read, could run. +export const MAX_FOLDER_BYTES = MAX_FOLDER_CHARACTERS * 4; Review Comment: The bound is right, but the reason given is the other inequality — and it survived `6316fe0dc` unchanged, so it's worth fixing while it's in view. What makes `4 * MAX_FOLDER_CHARACTERS` a safe pre-filter is that UTF-8 uses at *most* four bytes per character: `bytes <= 4 * chars`, so `bytes > 4 * CAP` does imply `chars > CAP`. "Never uses fewer bytes than characters" is `bytes >= chars`, which only gives `chars <= bytes` and rules nothing out. Worth being exact because the stated property would justify a tighter bound than it can support: someone reading it could reasonably set this to `MAX_FOLDER_CHARACTERS` and refuse a folder of CJK-commented code at a third of the real cap. ########## frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.ts: ########## @@ -30,20 +30,34 @@ import { NzButtonComponent } from "ng-zorro-antd/button"; import { NzIconDirective } from "ng-zorro-antd/icon"; import { NzTabsComponent, NzTabComponent, NzTabDirective } from "ng-zorro-antd/tabs"; import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; +import { folderRootName, pickedFilePath } from "../../service/notebook-migration/folder-assembly"; +import { NotificationService } from "../../../common/service/notification/notification.service"; // Passed in via nzData. requestImport resolves true to close the modal, false to keep it open -// with the user's selection intact (bad file or a retryable failure). +// with the user's selection intact (bad file or a retryable failure). A folder is handed over as +// the whole picked list, which is also how the opener tells a folder from a single file. export interface NotebookImportModalData { - requestImport: (file: NzUploadFile, model: string) => Promise<boolean>; + requestImport: (selection: NzUploadFile | NzUploadFile[], model: string) => Promise<boolean>; +} + +// Falls back to a count when the picker reported no path, so the row always says something Review Comment: `747a9dc9c` dropped the new constant between `describeFolderSelection`'s comment and the function, so lines 43-44 now read as the opening of the latch comment and `describeFolderSelection` has none. Moving those two lines down to just above the function restores both. ########## frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts: ########## @@ -696,4 +697,218 @@ describe("NotebookMigrationLLM", () => { expect(callModelSpy).not.toHaveBeenCalled(); }); }); + describe("convertFolderToWorkflow", () => { + // Two files behind banner lines: a.py on document lines 1-3, b.py on lines 4-5. + const folder = buildFolderDocument( + [ + { path: "a.py", source: "import os\nx = compute()" }, + { path: "b.py", source: "print(x)" }, + ], + { rootName: "proj", otherPaths: ["requirements.txt"] } + ); + + const workflowResponse = JSON.stringify({ + code: { UDF1: "# UDF1", UDF2: "# UDF2" }, + edges: [["UDF1", "UDF2"]], + outputs: { UDF1: ["value"], UDF2: ["result"] }, + }); + + // Same reason as the script suite: sendPrompt mutates the array it handed to callModel, so + // index by the conversion's first call and read the whole exchange. + function contentsFor(role: string, firstCallIndex = 0): string[] { + return (callModelSpy.mock.calls[firstCallIndex][0] as { role: string; content: string }[]) + .filter(message => message.role === role) + .map(message => message.content); + } + + it("sends one document whose line numbers run across both files", async () => { + const llm = makeLLM(); + mockResponses(workflowResponse, JSON.stringify({ UDF1: [[2, 3]] })); + + await llm.convertFolderToWorkflow(folder); + + const prompt = contentsFor("user").join("\n"); + expect(prompt).toContain("1| # ===== FILE: a.py ====="); + expect(prompt).toContain("2| import os"); + // b.py restarts no numbering: its first line is 4, not 1. + expect(prompt).toContain("4| # ===== FILE: b.py ====="); + expect(prompt).toContain("5| print(x)"); + }); + + it("sends the layout ahead of the code, and never numbers it", async () => { Review Comment: The name promises two properties but only the numbering half is asserted. Swapping `preamble` and `numberScriptLines(source)` in `requestConversion` leaves all 57 tests in this file green — I checked — even though the prompt then reads "Here is the code:" *after* the code. One `indexOf` comparison would lock the order the name claims. -- 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]
