Merge the Python Notebook Migration Tool in a Single PR

[zyratlo]

Summary

A new feature currently in the review phase before being merged is the Python Notebook Migration Tool. The related pages are Issue #4301 and PR #4486 . As of the time of this writing, the PR has 37 files changed and 3,342 additions. We have decided to currently merge this large addition to the code base in a single PR for multiple reasons:

• The code is not able to be sliced vertically to merge in multiple PR's
  • While portions of the code can be merged across multiple PR's, since we want the tool to still be functional in each PR we cannot divide it while maintaining functionality
• The code can be merged safely in one PR since it does not interfere or modify with any other parts of the code base.
  • Existing files which are modified are only changed to add functionality for this tool, and ensures any other pre-existing code is unchanged
  • A full breakdown of the new code can be found below

Architecture

As shown in the architecture diagrams below, a large portion of the new code is contained within new files. Portions of the front-end and existing services (such as the LiteLLM proxy) are reused while maintaining their original purposes are unchanged.

Breakdown of Code Changes

More detailed breakdown can be found here

SQL

• sql/updates/23.sql
sql/texera_ddl.sql
  • Add notebook and workflow_notebook_mapping tables to database

Modified Existing Files

Config files

• build.sbt
  • Add NotebookMigrationService
• common/config/src/main/resources/storage.conf
common/config/src/main/scala/org/apache/texera/amber/config/StorageConfig.scala
  • Added path for Jupyter server
• frontend/proxy.config.json
  • Add notebook-migration proxy
• frontend/src/app/app.module.ts
  • Add Jupyter components/services

Front-end

• frontend/src/app/workspace/component/menu/menu.component.ts
  • Added new button for upload popup and calling necessary services for the tool
• frontend/src/app/workspace/component/workflow-editor/mini-map/workspace.component.ts
  • Added spinner and stopwatch for LLM waiting phase

New Files

Front-end

• frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts
  • Logic to handle the Jupyter iframe (opening, closing, find the iframe URL, etc.)
• frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts
  • Handles communication between Jupyter iframe and the workflow
• frontend/src/app/workspace/service/notebook-migration/migration-llm.ts
  • Handles all communication with LiteLLM proxy
• frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts
  • Handles communication with back-end and mapping logic

Back-end

• notebook-migration-service/
  • All back-end code is in this directory and no code outside of it has been modified

[chenlica]

Thanks, @zyratlo .

@mengw15 : please share your thoughts about whether this PR could be further split vertically.

[kunwp1]

I haven't looked at the codes in detail but based on the demo videos, I think we should break it up to make this easier to review. How about this?

• PR 1: Uploading and rendering the notebook (plus the window controls).
• PR 2: The LLM notebook-to-workflow conversion logic.
• PR 3: The cell-to-operator mapping.

[zyratlo]

The main concern about splitting it up how you suggested and splitting up by feature in general is that the tool will only be partly functional with each PR until every PR is merged. We desire a way to break it up while maintaining the complete functionality of the tool at any point in each PR, which is why currently it is being merged in a single PR.

[kunwp1]

If you're concerned about disrupting the notebook-to-workflow functionality, we could introduce a configuration flag. We can set the default to false until all split PRs have been merged.

[kunwp1]

Also, since this PR introduces a new UI that hasn't been announced yet, the risk to users is minimal. Most won't even realize the feature exists until you officially notify them.

[chenlica]

A general question is whether we can have a PR after which we have "partial functionality."

@Yicong-Huang @aglinxinyuan @zuozhiw and others : What do you think?

[Yicong-Huang]

of course, Rome is never built in a day, by merging into main branch, does not mean a feature has to work end to end. it is the releasing time that matters: features to be released are better to be functional in a desired state.

The normal development loop is:

1. A few PRs to create a framework, with unit tests, mostly with a feature flag that hides the entire thing from user.
2. A few PRs to add some partial features, with unit tests
3. A few PRs to fix bugs as we develop, with unit tests
4. A few PRs to connect the new feature with other services, or hook up with frontend button etc, with integration tests.
5. A few more PRs to fix bug, add docs, refactor, polish. so far only alpha test: no users see it.
6. We decide if the feature is ready to be beta-tested, and flip the feature flag so users can see them. optionally we can flip deployment by deployment, so partial users can use the features and collect feedbacks before we include more users.
7. We include it in a release, enabling it by default, or tell users how to enable it. that's when it's done.
8. More post release bug fixes PRs ....

So. my answer to this question is, when there is no time pressure, we MUST follow the above procedure to gradually review and merge small PRs. You can divide the feature vertically, or horizontally, whatever way makes sense. Or even create PRs file by file, as long as you have tests to verify the unit correctness.

[mengw15]

I agree with @aglinxinyuan here.

However, if we're specifically aiming for vertical splitting (where each PR cuts through the full stack — DB, backend, frontend — and delivers something the user can actually interact with), the question gets a bit different. In the user-facing case, I do wonder whether "partial functionality" is acceptable or not.

[Yicong-Huang]

TL;DR, merge PRs with partial functionality, and don't let users see it till fully done.

[chenlica]

@mengw15 Can we draw a conclusion for this particular project?

[zuozhiw]

Agree with Yicong's point, best practices is merge PRs gradually that are easy to review, I recognize the overhead to the author, but small PRs are the best way. If a PR breaks the main branch primary functionality, like running a workflow, then it's better to merge things together. But this PR doesn't seems to be the case and this feature is rather isolated so it's ok to have it in partial state.

[mengw15]

Thanks @kunwp1 for the input. I think it's a great suggestion.

In general I agree a large PR isn't ideal, and smaller PRs are better when feasible. To address @zyratlo's concern about keeping the tool fully functional at each step, I wonder if we could keep @kunwp1's three slices but reorder them:

PR1: Uploading + LLM notebook-to-workflow conversion — the core function. After this PR, a user can upload a notebook and get a converted workflow end-to-end.

PR2: Rendering the notebook (plus window controls) — adds the Jupyter iframe panel as a UX enhancement on top of the working tool.

PR3: Cell-to-operator mapping — adds traceability between cells and operators.

Under this ordering, the main functionality lands in PR1, and PR2/PR3 are additive features. The tool stays functional after every PR, so the slices are more independent.

@zyratlo — could you confirm whether this reordering is feasible given the actual code dependencies? If it isn't, I'm not against shipping as a single PR, since the code is well-isolated, the risk is low.

[zyratlo]

This ordering is possible, I will need to do some omitting in certain files to remove code not related to the feature added in that PR but in terms of dependencies it should be able to be split in this way.

[mengw15]

@chenlica Based on the discussion, here's my proposed conclusion:

We don't go with a single PR. Instead, we split into multiple PRs, with a feature flag to hide the feature until everything is merged.

The thread converged on a few points that together resolve the original concern:

1. Smaller, incremental PRs are preferred over one big single PR (per @Yicong-Huang's development loop).

2. Partial functionality at the merge level is fine, as long as users don't see incomplete state.

3. we can utilize a feature flag to achieve this(default false until all PRs are merged)

[mengw15]

Yes

[Yicong-Huang]

not necessarily.

For instance: if you vertically you have 2 sub-features, and each sub feature have 3 horizontal layers, db, service, UI. you could also split 6 PRs, or even more. Not necessarily vertically end to end.

[chenlica]

So it can be a combination of vertical splits and horizontal splits, like a Parquet file?

[Yicong-Huang]

yes. as small as possible. smaller PRs are easier to review.

[chenlica]

OK. @mengw15 @zyratlo Let's go with this plan.

[aglinxinyuan]

You can check my PR as an example: #4442.
The original PR included about 70 files, and each was required for the feature to work. I break it into 10 PRs.

[zyratlo]

Thanks, I will take a look for reference