Copilot commented on code in PR #8910:
URL: https://github.com/apache/texera/pull/8910#discussion_r4288391840
##########
frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts:
##########
@@ -316,6 +331,74 @@ describe("NotebookMigrationLLM", () => {
});
});
+ describe("Python prompts", () => {
+ const workflowResponse = JSON.stringify({ code: { UDF1: "# UDF1" }, edges:
[], outputs: {} });
+ const documentation = [
+ TEXERA_OVERVIEW,
+ TUPLE_DOCUMENTATION,
+ TABLE_DOCUMENTATION,
+ OPERATOR_DOCUMENTATION,
+ EXAMPLE_OF_GOOD_CONVERSION,
+ VISUALIZER_DOCUMENTATION,
+ UDF_INPUT_PORT_DOCUMENTATION,
+ EXAMPLE_OF_MULTIPLE_UDF_CONVERSION,
+ ];
+
+ function contentsFor(role: string): string[] {
+ return (callModelSpy.mock.calls[0][0] as { role: string; content: string
}[])
+ .filter(message => message.role === role)
+ .map(message => message.content);
+ }
+
+ it("sends the notebook prelude and prompts unchanged", async () => {
+ mockResponses(workflowResponse, JSON.stringify({ UDF1: ["CELL1"] }));
+
+ await makeLLM().convertNotebookToWorkflow({ cells: [codeCell("CELL1",
"print(1)")] });
+
+ expect(contentsFor("system")).toEqual(documentation);
+ const [workflowPrompt, mappingPrompt] = contentsFor("user");
+ expect(workflowPrompt.startsWith(`${WORKFLOW_PROMPT}\n`)).toBe(true);
+ expect(mappingPrompt).toBe(MAPPING_PROMPT);
+ });
+
+ it("sends the script prelude and prompts unchanged", async () => {
+ mockResponses(workflowResponse, JSON.stringify({ UDF1: [[1, 1]] }));
+
+ await makeLLM().convertScriptToWorkflow("print(1)");
+
+ expect(contentsFor("system")).toEqual(
+ documentation.map(doc =>
+ doc === EXAMPLE_OF_MULTIPLE_UDF_CONVERSION ?
EXAMPLE_OF_MULTIPLE_UDF_CONVERSION_SCRIPT : doc
+ )
+ );
+ const [workflowPrompt, mappingPrompt] = contentsFor("user");
+
expect(workflowPrompt.startsWith(`${SCRIPT_WORKFLOW_PROMPT}\n`)).toBe(true);
+ expect(mappingPrompt).toBe(SCRIPT_MAPPING_PROMPT);
Review Comment:
contentsFor()` always inspects `callModelSpy.mock.calls[0]` (the first model
invocation). Since conversions call the model at least twice (workflow prompt
then mapping prompt), the first call’s message list typically won’t include the
mapping prompt—making `mappingPrompt` `undefined` and the assertions
incorrect/flaky. Consider either (a) updating `contentsFor` to accept a call
index and asserting workflow prompt from call 0 and mapping prompt from call 1,
or (b) using the existing conversation helper (seen elsewhere in this spec)
that already supports selecting which call to inspect.
##########
frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts:
##########
@@ -316,6 +331,74 @@ describe("NotebookMigrationLLM", () => {
});
});
+ describe("Python prompts", () => {
+ const workflowResponse = JSON.stringify({ code: { UDF1: "# UDF1" }, edges:
[], outputs: {} });
+ const documentation = [
+ TEXERA_OVERVIEW,
+ TUPLE_DOCUMENTATION,
+ TABLE_DOCUMENTATION,
+ OPERATOR_DOCUMENTATION,
+ EXAMPLE_OF_GOOD_CONVERSION,
+ VISUALIZER_DOCUMENTATION,
+ UDF_INPUT_PORT_DOCUMENTATION,
+ EXAMPLE_OF_MULTIPLE_UDF_CONVERSION,
+ ];
+
+ function contentsFor(role: string): string[] {
+ return (callModelSpy.mock.calls[0][0] as { role: string; content: string
}[])
+ .filter(message => message.role === role)
+ .map(message => message.content);
+ }
Review Comment:
contentsFor()` always inspects `callModelSpy.mock.calls[0]` (the first model
invocation). Since conversions call the model at least twice (workflow prompt
then mapping prompt), the first call’s message list typically won’t include the
mapping prompt—making `mappingPrompt` `undefined` and the assertions
incorrect/flaky. Consider either (a) updating `contentsFor` to accept a call
index and asserting workflow prompt from call 0 and mapping prompt from call 1,
or (b) using the existing conversation helper (seen elsewhere in this spec)
that already supports selecting which call to inspect.
##########
frontend/src/app/workspace/service/notebook-migration/migration-llm.ts:
##########
@@ -111,15 +111,66 @@ function toDerivedNotebook(cells: DerivedCell[]):
Notebook {
outputs: [],
execution_count: null,
})),
- metadata: {
- kernelspec: { display_name: "Python 3", language: "python", name:
"python3" },
- language_info: { name: "python" },
- },
+ metadata: structuredClone(metadata),
Review Comment:
structuredClone` is not available in all runtime environments/browsers
without a polyfill; if the frontend’s supported browser matrix includes older
Safari/Chromium versions, this will throw at runtime when deriving notebooks
from scripts/folders. If broad browser support is required, consider switching
to an existing project-standard deep-clone utility (or a guarded fallback) for
cloning notebook metadata.
##########
frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts:
##########
@@ -316,6 +331,74 @@ describe("NotebookMigrationLLM", () => {
});
});
+ describe("Python prompts", () => {
+ const workflowResponse = JSON.stringify({ code: { UDF1: "# UDF1" }, edges:
[], outputs: {} });
+ const documentation = [
+ TEXERA_OVERVIEW,
+ TUPLE_DOCUMENTATION,
+ TABLE_DOCUMENTATION,
+ OPERATOR_DOCUMENTATION,
+ EXAMPLE_OF_GOOD_CONVERSION,
+ VISUALIZER_DOCUMENTATION,
+ UDF_INPUT_PORT_DOCUMENTATION,
+ EXAMPLE_OF_MULTIPLE_UDF_CONVERSION,
+ ];
+
+ function contentsFor(role: string): string[] {
+ return (callModelSpy.mock.calls[0][0] as { role: string; content: string
}[])
+ .filter(message => message.role === role)
+ .map(message => message.content);
+ }
+
+ it("sends the notebook prelude and prompts unchanged", async () => {
+ mockResponses(workflowResponse, JSON.stringify({ UDF1: ["CELL1"] }));
+
+ await makeLLM().convertNotebookToWorkflow({ cells: [codeCell("CELL1",
"print(1)")] });
+
+ expect(contentsFor("system")).toEqual(documentation);
+ const [workflowPrompt, mappingPrompt] = contentsFor("user");
+ expect(workflowPrompt.startsWith(`${WORKFLOW_PROMPT}\n`)).toBe(true);
+ expect(mappingPrompt).toBe(MAPPING_PROMPT);
Review Comment:
contentsFor()` always inspects `callModelSpy.mock.calls[0]` (the first model
invocation). Since conversions call the model at least twice (workflow prompt
then mapping prompt), the first call’s message list typically won’t include the
mapping prompt—making `mappingPrompt` `undefined` and the assertions
incorrect/flaky. Consider either (a) updating `contentsFor` to accept a call
index and asserting workflow prompt from call 0 and mapping prompt from call 1,
or (b) using the existing conversation helper (seen elsewhere in this spec)
that already supports selecting which call to inspect.
--
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]
