Skip to content

fix: preserve invalid cells in topological IPYNB export - #10451

Merged
mscolnick merged 2 commits into
marimo-team:mainfrom
peter-gy:ptr/fix-ipynb-topological-export
Aug 4, 2026
Merged

fix: preserve invalid cells in topological IPYNB export#10451
mscolnick merged 2 commits into
marimo-team:mainfrom
peter-gy:ptr/fix-ipynb-topological-export

Conversation

@peter-gy

@peter-gy peter-gy commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Topological IPYNB export could silently drop cells present in the current notebook document but absent from the dependency graph. This occurs during live editing when a cell contains transient invalid code such as x =.

The converter now starts from document order and applies topological sorting only when the graph contains exactly the same cell IDs. When coverage differs, it keeps document order and preserves every cell.

Copilot AI review requested due to automatic review settings August 4, 2026 05:24
@peter-gy peter-gy added the bug Something isn't working label Aug 4, 2026
@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview Aug 4, 2026 6:17am

Request Review

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 43.92kB (0.17%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
marimo-esm 25.56MB 43.92kB (0.17%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: marimo-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/dist-*.js -32 bytes 137 bytes -18.93%
assets/dist-*.js 154 bytes 256 bytes 150.98% ⚠️
assets/dist-*.js -155 bytes 104 bytes -59.85%
assets/dist-*.js 73 bytes 177 bytes 70.19% ⚠️
assets/dist-*.js 5 bytes 169 bytes 3.05%
assets/dist-*.js 79 bytes 183 bytes 75.96% ⚠️
assets/dist-*.js -79 bytes 104 bytes -43.17%
assets/dist-*.js -17 bytes 259 bytes -6.16%
assets/dist-*.js -266 bytes 137 bytes -66.0%
assets/dist-*.js 198 bytes 335 bytes 144.53% ⚠️
assets/dist-*.js -17 bytes 160 bytes -9.6%
assets/dist-*.js 283 bytes 387 bytes 272.12% ⚠️
assets/dist-*.js 7 bytes 176 bytes 4.14%
assets/dist-*.js 139 bytes 276 bytes 101.46% ⚠️
assets/dist-*.js -218 bytes 169 bytes -56.33%
assets/dist-*.js -233 bytes 102 bytes -69.55%
assets/dist-*.js -72 bytes 104 bytes -40.91%
assets/dist-*.js 147 bytes 403 bytes 57.42% ⚠️
assets/dist-*.js 4 bytes 164 bytes 2.5%
assets/add-*.js 674 bytes 55.23kB 1.24%
assets/file-*.js 869 bytes 54.37kB 1.62%
assets/useNotebookActions-*.js 31 bytes 60.99kB 0.05%
assets/components-*.js 42.35kB 54.12kB 359.7% ⚠️
assets/__vite-*.js 5 bytes 98 bytes 5.38% ⚠️
assets/__vite-*.js -5 bytes 93 bytes -5.1%

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a topological IPYNB export edge case where cells present in the live document but missing from the compiled dependency graph (e.g., during transient invalid edits) could be silently dropped. The exporter now only applies topological ordering when the dependency graph and document cover the exact same set of cell IDs; otherwise it preserves document order and exports every cell.

Changes:

  • Default to document-order export and only apply topological sorting when the graph’s cell IDs exactly match the document’s cell IDs.
  • Add a regression test ensuring an invalid (graph-missing) cell is preserved in topological export mode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
marimo/_convert/ipynb/from_ir.py Prevents dropping document cells by gating topological sorting on full graph/document ID coverage.
tests/_export/test_export_ipynb.py Adds a regression test for preserving invalid/missing-from-graph cells during topological export.

Comment thread tests/_export/test_export_ipynb.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Architecture diagram
sequenceDiagram
    participant User as User/Editor
    participant Export as convert_from_ir_to_ipynb()
    participant CM as CellManager
    participant Graph as Dependency Graph
    participant Sorter as dataflow.topological_sort()

    Note over User,Sorter: IPYNB Export with Topological Sort

    User->>Export: Request export with sort_mode="topological"
    Export->>CM: cell_data()
    CM-->>Export: List of cell_data (all cells in document order)

    Export->>Graph: Access app.graph
    Graph-->>Export: Graph object (only compiled cells)

    Export->>Export: Build cell_ids set from cell_data_list
    Export->>Graph: Get graph.cells keys

    alt cell_ids == set(graph.cells) (all cells are compilable)
        Export->>Sorter: topological_sort(graph, graph.cells.keys())
        Sorter-->>Export: Sorted cell IDs
        Export->>CM: cell_data_at(cid) for each sorted ID
        CM-->>Export: Sorted cell_data_list
    else cell_ids != set(graph.cells) (some cells have invalid code)
        Note over Export: Keep original document order
        Export->>Export: Use unchanged cell_data_list
    end

    Export->>Export: Build notebook from cell_data_list
    Export-->>User: IPYNB with all cells preserved

    Note over Export: Key: Invalid cells (e.g. "x =") are kept<br/>in document order when graph coverage differs
Loading

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/_export/test_export_ipynb.py Outdated
@mscolnick
mscolnick merged commit ab3c9fe into marimo-team:main Aug 4, 2026
39 of 40 checks passed
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.17-dev10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants