Skip to content

Complete ipynb export support #1915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
agoose77 merged 5 commits into from
May 23, 2025
Merged

Complete ipynb export support #1915

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35d4e45
fix some tests and comment the failing ones to fix in upcoming commits
kp992 Mar 12, 2025
9820179
fix tests
kp992 Mar 14, 2025
7d8218a
fix failure in myst-cli
kp992 Mar 14, 2025
2747ecc
update the tests and keep the split lines logic
kp992 Apr 3, 2025
1975f85
Revert merging md blocks and update 2 sample test cases
kp992 Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/myst-cli/src/build/build.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('get export formats', () => {
ExportFormats.tex,
ExportFormats.xml,
ExportFormats.md,
ExportFormats.ipynb,
ExportFormats.meca,
ExportFormats.cff,
]);
Expand Down
38 changes: 28 additions & 10 deletions packages/myst-to-ipynb/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,43 @@ function sourceToStringList(src: string): string[] {
return lines;
}

function markdownString(file: VFile, md_cells: Block[]) {
const md = writeMd(file, { type: 'root', children: md_cells }).result as string;
return {
cell_type: 'markdown',
metadata: {},
source: sourceToStringList(md),
};
}

export function writeIpynb(file: VFile, node: Root, frontmatter?: PageFrontmatter) {
const cells = (node.children as Block[]).map((block: Block) => {
const cells = [];
const md_cells: Block[] = [];

for (const block of node.children as Block[]) {
if (block.type === 'block' && block.kind === 'notebook-code') {
if (md_cells.length != 0) {
cells.push(markdownString(file, md_cells));
md_cells.length = 0;
}
const code = select('code', block) as Code;
return {
cells.push({
cell_type: 'code',
execution_count: null,
metadata: {},
outputs: [],
source: sourceToStringList(code.value),
};
});
} else {
md_cells.push(block);
}
const md = writeMd(file, { type: 'root', children: block.children as any }).result as string;
return {
cell_type: 'markdown',
metadata: {},
source: sourceToStringList(md),
};
});
}

if (md_cells.length != 0) {
cells.push(markdownString(file, md_cells));
Copy link
Contributor

@agoose77 agoose77 Apr 2, 2025

Choose a reason for hiding this comment

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

@kp992 this is a "policy" decision to fuse Markdown cells. Is there a particular reason for doing this? With the base PR #1882, if you ingest an ipynb file and export an ipynb, then you'll end up with the same number of cells (perhaps the frontmatter might be moved, though).

If you have a use-case for fusing Markdown cells, I'd suggest adding that as an option!

Copy link
Author

@kp992 kp992 Apr 3, 2025

Choose a reason for hiding this comment

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

Thanks for pointing this and reviewing the PR. I was actually in a confusion splitting at \n may break the code where the string contains \n. But we do have an escape character added before that after parsing so that should not be any issue. I tried that case works fine with the splitting logic so I will put that logic back. I have also updated the tests according to that.

Copy link
Contributor

@agoose77 agoose77 Apr 16, 2025

Choose a reason for hiding this comment

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

I still think these changes need reverting. Consider the following Markdown notebook

---
kernelspec:
  name: python3
  display_name: Python 3
---

```{code-cell} python3
import sys
```

+++ { "kind": "notebook-content" }
Desc 1

+++ { "kind": "notebook-content" }

Desc 2

```{code-cell} python3
import os
```

It's the MD equivalent of having four cells:

  • code
  • md
  • md
  • code

In this PR, that gets flattened to

  • code
  • md
  • code

(and the myst-to-md also writes the block separator +++ inside the cell, which is not quite right).

Can you elaborate on why you made this particular change? Did the original PR fail to convert something properly?

md_cells.length = 0;
}

const ipynb = {
cells,
metadata: {
Expand Down
Loading