From 69546b4d3b5df0875b6a1f868476eac2ffc43228 Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Thu, 23 Jul 2026 16:19:13 -0500 Subject: [PATCH 1/3] bd-5t6wvu7m Phases 1+2: jupyter image outputs become real figures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes in text_execute.rs format_outputs: 1. MIME priority now follows Q1's displayDataMimeType for HTML targets: text/html -> image/svg+xml -> image/png -> image/jpeg -> text/plain LAST. Previously text/plain was checked first, so a matplotlib display bundle (image/png + '
' text fallback) always rendered as its text repr — the image branch was unreachable for typical figures. 2. The image branch writes real files instead of the '[Image output]' placeholder: new FigureWriter emits _files/figure-html/cell--output-. next to the source doc (Q1's naming; the layout JupyterEngine:: intermediate_files already declares and knitr already produces), references it as ![](...) inside the .cell-output-display div, and reports the _files dir via ExecuteResult:: with_supporting_files — so q2 render copies figures to the output dir (bd-o8pr) and the preview capture embeds them for browser replay (bd-qbhp2cvv). SVG payloads starting with ' --- .../src/engine/jupyter/text_execute.rs | 389 ++++++++++++++++-- 1 file changed, 363 insertions(+), 26 deletions(-) diff --git a/crates/quarto-core/src/engine/jupyter/text_execute.rs b/crates/quarto-core/src/engine/jupyter/text_execute.rs index e377e6334..015c50bfd 100644 --- a/crates/quarto-core/src/engine/jupyter/text_execute.rs +++ b/crates/quarto-core/src/engine/jupyter/text_execute.rs @@ -11,6 +11,8 @@ //! It parses QMD input, executes code blocks via the Jupyter daemon, and returns //! markdown with outputs inserted. +use std::path::{Path, PathBuf}; + use regex::Regex; use quarto_pandoc_types::config_value::{ConfigValue, InterpretationContext}; @@ -176,6 +178,9 @@ async fn execute_blocks_inner( // Build output by processing blocks in order let mut output = String::new(); let mut last_end = 0; + // Figure emission (bd-5t6wvu7m): image outputs are written under + // `_files/figure-html/` next to the source document. + let mut fig = FigureWriter::new(&ctx.source_path); for block in blocks { // Append content before this block @@ -236,7 +241,13 @@ async fn execute_blocks_inner( // Emit the whole cell — echoed (option-stripped) source plus // outputs — in the Quarto-canonical `::: {.cell}` shape. - output.push_str(&render_cell(&block.language, &cell.code, &exec_result)); + fig.begin_cell(); + output.push_str(&render_cell( + &block.language, + &cell.code, + &exec_result, + &mut fig, + )); last_end = block.end; } @@ -244,7 +255,14 @@ async fn execute_blocks_inner( // Append any remaining content after the last block output.push_str(&input[last_end..]); - Ok(ExecuteResult::new(output)) + // Report the `_files` dir when figures were written — + // dir-level, mirroring knitr. `q2 render` copies it to the output + // dir (bd-o8pr) and the preview capture embeds it (bd-qbhp2cvv). + let mut result = ExecuteResult::new(output); + if fig.wrote_any { + result = result.with_supporting_files(vec![fig.files_dir()]); + } + Ok(result) } /// The `execute` scope of the document's (already merged) metadata, @@ -384,11 +402,16 @@ fn ticks_for_code(text: &str) -> String { /// engine cell with exactly one wrapper Div, and the Bootstrap CSS /// targets `.cell .cell-output-* pre code`. A cell with no outputs /// still gets the wrapper (knitr wraps output-less chunks too). -fn render_cell(language: &str, code: &str, result: &KernelExecuteResult) -> String { +fn render_cell( + language: &str, + code: &str, + result: &KernelExecuteResult, + fig: &mut FigureWriter, +) -> String { format!( "::: {{.cell}}\n\n{}\n{}\n:::\n", echoed_source_fence(language, code), - format_outputs(result) + format_outputs(result, fig) ) } @@ -417,6 +440,110 @@ fn fenced_output_div(classes: &str, text: &str) -> String { format!("\n::: {{{classes}}}\n\n{ticks}\n{text}\n{ticks}\n\n:::\n") } +/// Writes figure files for one document run (bd-5t6wvu7m). +/// +/// Image outputs (`image/png`, `image/jpeg`, `image/svg+xml`) are +/// written to `/_files/figure-html/` — the same layout +/// Q1's jupyter engine (`mdImageOutput` + `figuresDir("html")`) and +/// q2's knitr engine produce, and the layout +/// [`JupyterEngine::intermediate_files`](super::JupyterEngine) already +/// declares. Files are named `cell--output-.` (Q1's +/// shape). When anything was written, the caller reports the +/// `_files` directory in `ExecuteResult::supporting_files` — +/// dir-level, mirroring knitr — which is what `q2 render` copies to +/// the output dir and what the preview capture transport +/// (bd-qbhp2cvv) embeds for browser replay. +struct FigureWriter { + /// Absolute directory of the source document. + doc_dir: PathBuf, + /// `_files` (single path component). + files_dir_name: String, + /// 1-based index of the cell currently being rendered. Set by the + /// block loop before each `render_cell`. + cell_index: usize, + /// 1-based index of the next output within the current cell. + /// Reset by `begin_cell`. + output_index: usize, + /// Whether any figure file was successfully written. + wrote_any: bool, +} + +impl FigureWriter { + fn new(source_path: &Path) -> Self { + let doc_dir = source_path.parent().unwrap_or(Path::new(".")).to_path_buf(); + let stem = source_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("document"); + Self { + doc_dir, + files_dir_name: format!("{stem}_files"), + cell_index: 0, + output_index: 0, + wrote_any: false, + } + } + + /// Start figure numbering for the next cell. + fn begin_cell(&mut self) { + self.cell_index += 1; + self.output_index = 0; + } + + /// The `_files` directory as an absolute path (for + /// supporting-files reporting). + fn files_dir(&self) -> PathBuf { + self.doc_dir.join(&self.files_dir_name) + } + + /// Write one image output. Returns the doc-relative, forward-slash + /// path to emit in markdown, or `None` when the payload is + /// malformed or the write fails (caller falls back — fail-soft, + /// matching the engine's other output paths). + fn write_image(&mut self, mime: &str, value: &serde_json::Value) -> Option { + let ext = match mime { + "image/png" => "png", + "image/jpeg" => "jpeg", + "image/svg+xml" => "svg", + _ => return None, + }; + let content = extract_text_content(value); + // Q1 rule (`mdImageOutput`): SVG payloads that are literal + // ` = if ext == "svg" && content.trim_start().starts_with(" bytes, + Err(e) => { + tracing::warn!(mime, error = %e, "jupyter figure: invalid base64; skipping"); + return None; + } + } + }; + + self.output_index += 1; + let file_name = format!( + "cell-{}-output-{}.{ext}", + self.cell_index, self.output_index + ); + let rel = format!("{}/figure-html/{file_name}", self.files_dir_name); + let abs = self.doc_dir.join(&self.files_dir_name).join("figure-html"); + if let Err(e) = std::fs::create_dir_all(&abs) { + tracing::warn!(dir = %abs.display(), error = %e, "jupyter figure: cannot create figures dir"); + return None; + } + if let Err(e) = std::fs::write(abs.join(&file_name), &bytes) { + tracing::warn!(file = %file_name, error = %e, "jupyter figure: write failed"); + return None; + } + self.wrote_any = true; + Some(rel) + } +} + /// Format kernel outputs as markdown. /// /// Every output becomes a `::: {.cell-output .cell-output-}` @@ -424,7 +551,16 @@ fn fenced_output_div(classes: &str, text: &str) -> String { /// (`outputTypeCssClass` in quarto-cli's `jupyter.ts`): streams are /// `-stdout` / `-stderr`, `execute_result` / `display_data` are /// `-display`, errors are `-error`. -fn format_outputs(result: &KernelExecuteResult) -> String { +/// +/// Rich-output MIME priority follows Q1's `displayDataMimeType` for +/// HTML targets, scoped to the types q2 handles: `text/html` → +/// `image/svg+xml` → `image/png` → `image/jpeg` → `text/plain` +/// **last** (a matplotlib bundle carries an image plus a text +/// fallback; the image must win). `text/markdown` and jupyter-widget +/// payloads remain unhandled (follow-up). +fn format_outputs(result: &KernelExecuteResult, fig: &mut FigureWriter) -> String { + const IMAGE_MIMES: [&str; 3] = ["image/svg+xml", "image/png", "image/jpeg"]; + let mut output = String::new(); for cell_output in &result.outputs { @@ -436,23 +572,34 @@ fn format_outputs(result: &KernelExecuteResult) -> String { )); } CellOutput::ExecuteResult { data, .. } | CellOutput::DisplayData { data, .. } => { - // Rich output - pick best format - if let Some(text) = data.get("text/plain") { - let s = extract_text_content(text); - output.push_str(&fenced_output_div( - ".cell-output .cell-output-display", - s.trim_end(), - )); - } else if let Some(html) = data.get("text/html") { + // Rich output — pick the best format (Q1 priority). + let image_entry = IMAGE_MIMES + .iter() + .find_map(|m| data.get(*m).map(|v| (*m, v))); + if let Some(html) = data.get("text/html") { let s = extract_text_content(html); let ticks = ticks_for_code(&s); output.push_str(&format!( "\n::: {{.cell-output .cell-output-display}}\n\n{ticks}{{=html}}\n{}\n{ticks}\n\n:::\n", s )); - } else if data.contains_key("image/png") || data.contains_key("image/svg+xml") { - // TODO(bd-5t6wvu7m): save the image to a supporting - // file and emit a real figure instead of a placeholder. + } else if let Some(rel_path) = + image_entry.and_then(|(mime, value)| fig.write_image(mime, value)) + { + output.push_str(&format!( + "\n::: {{.cell-output .cell-output-display}}\n\n![]({rel_path})\n\n:::\n", + )); + } else if let Some(text) = data.get("text/plain") { + let s = extract_text_content(text); + output.push_str(&fenced_output_div( + ".cell-output .cell-output-display", + s.trim_end(), + )); + } else if image_entry.is_some() { + // An image payload we failed to write (bad base64, + // I/O error) and no text fallback: keep the + // pre-bd-5t6wvu7m placeholder rather than dropping + // the output silently. output.push_str( "\n::: {.cell-output .cell-output-display}\n\n[Image output]\n\n:::\n", ); @@ -679,6 +826,21 @@ print("hello") data } + /// A `FigureWriter` rooted in a fresh temp dir, positioned at cell 1. + /// The temp dir is leaked for the process lifetime — tests that + /// assert on written files create their own named `TempDir` instead. + fn test_fig() -> FigureWriter { + let dir = tempfile::tempdir().unwrap(); + let mut fig = FigureWriter::new(&dir.path().join("doc.qmd")); + fig.begin_cell(); + std::mem::forget(dir); + fig + } + + /// Valid base64 of a 1x1 transparent PNG, split across two lines + /// (the nbformat multiline convention the decoder must handle). + const PNG_B64_MULTILINE: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk\nYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=="; + #[test] fn test_echoed_source_fence_emits_cell_code_class() { // `{python}` fence means "execute"; after execution the echoed @@ -711,7 +873,7 @@ print("hello") text: "Hello, World!\n".to_string(), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-stdout}\n\n```\nHello, World!\n```\n\n:::\n" ); } @@ -723,7 +885,7 @@ print("hello") text: "warning\n".to_string(), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-stderr}\n\n```\nwarning\n```\n\n:::\n" ); } @@ -738,7 +900,7 @@ print("hello") metadata: serde_json::json!({}), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-display}\n\n```\n5\n```\n\n:::\n" ); } @@ -752,7 +914,7 @@ print("hello") metadata: serde_json::json!({}), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-display}\n\n```{=html}\n5\n```\n\n:::\n" ); } @@ -765,7 +927,7 @@ print("hello") traceback: vec!["Traceback...".to_string()], }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-error}\n\n```\nNameError: name 'x' is not defined\nTraceback...\n```\n\n:::\n" ); } @@ -779,7 +941,7 @@ print("hello") text: "```\n".to_string(), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-stdout}\n\n````\n```\n````\n\n:::\n" ); } @@ -792,11 +954,186 @@ print("hello") metadata: serde_json::json!({}), }]); assert_eq!( - render_cell("python", "2 + 3\n", &result), + render_cell("python", "2 + 3\n", &result, &mut test_fig()), "::: {.cell}\n\n```{.python .cell-code}\n2 + 3\n```\n\n::: {.cell-output .cell-output-display}\n\n```\n5\n```\n\n:::\n\n:::\n" ); } + #[test] + fn test_display_bundle_with_png_and_text_prefers_image() { + // bd-5t6wvu7m: a matplotlib display bundle carries BOTH + // image/png and a text/plain fallback (`
`). The image must win — Q1's + // displayDataMimeType puts text/plain last. Before the fix, + // format_outputs checked text/plain first, so the figure + // rendered as its text repr. + let mut data = std::collections::HashMap::new(); + // 1x1 transparent PNG, valid base64. + data.insert( + "image/png".to_string(), + serde_json::json!( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk\nYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==" + ), + ); + data.insert( + "text/plain".to_string(), + serde_json::json!("
"), + ); + let result = ok_result(vec![CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + }]); + + let md = format_outputs(&result, &mut test_fig()); + assert!( + !md.contains("
_files/figure-html/cell--output-.png` next to + // the doc and emits `![](...)` inside the display div. + let tmp = tempfile::tempdir().unwrap(); + let mut fig = FigureWriter::new(&tmp.path().join("pyfig.qmd")); + fig.begin_cell(); + + let mut data = std::collections::HashMap::new(); + data.insert( + "image/png".to_string(), + serde_json::json!(PNG_B64_MULTILINE), + ); + data.insert( + "text/plain".to_string(), + serde_json::json!("
"), + ); + let result = ok_result(vec![CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + }]); + + let md = format_outputs(&result, &mut fig); + assert_eq!( + md, + "\n::: {.cell-output .cell-output-display}\n\n![](pyfig_files/figure-html/cell-1-output-1.png)\n\n:::\n" + ); + // Multiline base64 decoded and written to disk. + let bytes = std::fs::read( + tmp.path() + .join("pyfig_files/figure-html/cell-1-output-1.png"), + ) + .expect("figure file written"); + assert!( + bytes.starts_with(b"\x89PNG\r\n"), + "decoded bytes must be a PNG" + ); + assert!(fig.wrote_any); + assert_eq!(fig.files_dir(), tmp.path().join("pyfig_files")); + } + + #[test] + fn test_svg_markup_written_as_text_with_svg_extension() { + // Q1's mdImageOutput rule: literal `"), + ); + let result = ok_result(vec![CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + }]); + + let md = format_outputs(&result, &mut fig); + assert!( + md.contains("![](doc_files/figure-html/cell-1-output-1.svg)"), + "got:\n{md}" + ); + let text = + std::fs::read_to_string(tmp.path().join("doc_files/figure-html/cell-1-output-1.svg")) + .expect("svg written"); + assert!(text.starts_with("table")); + data.insert( + "image/png".to_string(), + serde_json::json!(PNG_B64_MULTILINE), + ); + data.insert("text/plain".to_string(), serde_json::json!("table")); + let result = ok_result(vec![CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + }]); + + let md = format_outputs(&result, &mut test_fig()); + assert!(md.contains("{=html}"), "html must win; got:\n{md}"); + assert!(!md.contains("![]("), "no image when html wins; got:\n{md}"); + } + + #[test] + fn test_two_outputs_in_one_cell_get_distinct_names() { + let tmp = tempfile::tempdir().unwrap(); + let mut fig = FigureWriter::new(&tmp.path().join("doc.qmd")); + fig.begin_cell(); + + let png_output = || { + let mut data = std::collections::HashMap::new(); + data.insert( + "image/png".to_string(), + serde_json::json!(PNG_B64_MULTILINE), + ); + CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + } + }; + let result = ok_result(vec![png_output(), png_output()]); + + let md = format_outputs(&result, &mut fig); + assert!(md.contains("cell-1-output-1.png"), "got:\n{md}"); + assert!(md.contains("cell-1-output-2.png"), "got:\n{md}"); + assert!( + tmp.path() + .join("doc_files/figure-html/cell-1-output-2.png") + .exists() + ); + } + + #[test] + fn test_invalid_base64_with_text_fallback_uses_text() { + // Fail-soft: an unwritable image with a text/plain sibling + // falls back to the text representation rather than a + // placeholder or a panic. + let mut data = std::collections::HashMap::new(); + data.insert( + "image/png".to_string(), + serde_json::json!("!!!not-base64!!!"), + ); + data.insert("text/plain".to_string(), serde_json::json!("fallback")); + let result = ok_result(vec![CellOutput::DisplayData { + data, + metadata: serde_json::json!({}), + }]); + + let md = format_outputs(&result, &mut test_fig()); + assert!(md.contains("fallback"), "got:\n{md}"); + assert!(!md.contains("[Image output]"), "got:\n{md}"); + } + #[test] fn test_format_outputs_image_placeholder_is_display_div() { // Until bd-5t6wvu7m lands, image outputs render a placeholder — @@ -808,7 +1145,7 @@ print("hello") metadata: serde_json::json!({}), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-display}\n\n[Image output]\n\n:::\n" ); } @@ -848,7 +1185,7 @@ print("hello") metadata: serde_json::json!({}), }]); assert_eq!( - format_outputs(&result), + format_outputs(&result, &mut test_fig()), "\n::: {.cell-output .cell-output-display}\n\n```\n42\n```\n\n:::\n" ); } @@ -861,7 +1198,7 @@ print("hello") // too. let result = ok_result(vec![]); assert_eq!( - render_cell("python", "x = 1\n", &result), + render_cell("python", "x = 1\n", &result, &mut test_fig()), "::: {.cell}\n\n```{.python .cell-code}\nx = 1\n```\n\n:::\n" ); } From 7527df2989c8d33dab7dae4611547ca27cc456e2 Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Thu, 23 Jul 2026 16:22:27 -0500 Subject: [PATCH 2/3] bd-5t6wvu7m Phase 3: figure-output parity case + knitr-shaped figure divs jupyter figure divs now carry ONLY cell-output-display (no generic cell-output), matching q2-knitr's vendored figure hook (hooks.R:627). Q1's own engines disagree here (Q1 jupyter adds .cell-output to every output; Q1 knitr does not for figures); q2 resolves the asymmetry toward knitr so the parity suite's strict semantic-class comparison holds with no figure-specific carve-out. The only generic .cell-output consumer (printStylesheet.ts page-break list) also matches img directly, so CSS behavior is unchanged. New parity_plot_output case in engine_output_parity.rs: knitr plot(1) vs jupyter matplotlib (trailing ; suppresses the execute_result so the cells are semantically equivalent) must produce the same block shape, exactly one image each under doc_files/figure-html/, and captures reporting non-empty supporting_files. Skips when knitr, jupyter, or matplotlib is unavailable; verified actually running (2.1s, real engines) and passing locally. Co-Authored-By: Claude Fable 5 --- .../src/engine/jupyter/text_execute.rs | 13 +- .../tests/integration/engine_output_parity.rs | 128 ++++++++++++++++++ 2 files changed, 139 insertions(+), 2 deletions(-) diff --git a/crates/quarto-core/src/engine/jupyter/text_execute.rs b/crates/quarto-core/src/engine/jupyter/text_execute.rs index 015c50bfd..6079e5db9 100644 --- a/crates/quarto-core/src/engine/jupyter/text_execute.rs +++ b/crates/quarto-core/src/engine/jupyter/text_execute.rs @@ -586,8 +586,17 @@ fn format_outputs(result: &KernelExecuteResult, fig: &mut FigureWriter) -> Strin } else if let Some(rel_path) = image_entry.and_then(|(mime, value)| fig.write_image(mime, value)) { + // Figure divs carry ONLY `cell-output-display` — no + // generic `cell-output` — matching q2-knitr's + // vendored figure hook (hooks.R:627) so the + // cross-engine parity contract holds. (Q1's jupyter + // adds `.cell-output` here and thus disagrees with + // Q1's own knitr; q2 resolves the asymmetry toward + // knitr. The only generic `.cell-output` consumer, + // the print stylesheet, also matches `img` + // directly.) output.push_str(&format!( - "\n::: {{.cell-output .cell-output-display}}\n\n![]({rel_path})\n\n:::\n", + "\n::: {{.cell-output-display}}\n\n![]({rel_path})\n\n:::\n", )); } else if let Some(text) = data.get("text/plain") { let s = extract_text_content(text); @@ -1017,7 +1026,7 @@ print("hello") let md = format_outputs(&result, &mut fig); assert_eq!( md, - "\n::: {.cell-output .cell-output-display}\n\n![](pyfig_files/figure-html/cell-1-output-1.png)\n\n:::\n" + "\n::: {.cell-output-display}\n\n![](pyfig_files/figure-html/cell-1-output-1.png)\n\n:::\n" ); // Multiline base64 decoded and written to disk. let bytes = std::fs::read( diff --git a/crates/quarto-core/tests/integration/engine_output_parity.rs b/crates/quarto-core/tests/integration/engine_output_parity.rs index 5669d0c67..40e9d901c 100644 --- a/crates/quarto-core/tests/integration/engine_output_parity.rs +++ b/crates/quarto-core/tests/integration/engine_output_parity.rs @@ -236,6 +236,134 @@ fn parity_source_only_cell() { assert_engine_parity("x <- 1", "x = 1"); } +/// Whether the `python3` on PATH can import matplotlib. Approximation: +/// the kernel the daemon launches resolves `python3` the same way in +/// practice, and a false negative just skips the test. Keeps the +/// figure-parity case from failing on machines that have jupyter but +/// not matplotlib. +fn matplotlib_available() -> bool { + std::process::Command::new("python3") + .args(["-c", "import matplotlib"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +/// Figure output parity (bd-5t6wvu7m): a plotting cell must produce +/// the same shape from both engines — a `.cell` wrapper with the +/// echoed source fence plus a figure display div containing an image +/// whose target lives under `_files/figure-html/`, and a +/// capture that reports non-empty `supporting_files` (the transport +/// contract bd-o8pr / bd-qbhp2cvv ride on). +/// +/// The jupyter cell ends with `;` to suppress the execute_result text +/// (`[]`) — matplotlib both returns a +/// value and displays the figure, while knitr's `plot()` only +/// displays; the semicolon makes the two cells semantically +/// equivalent (one figure, no textual value). +#[test] +fn parity_plot_output() { + if !both_engines_available() { + eprintln!("Skipping test: parity suite needs both knitr and jupyter installed"); + return; + } + if !matplotlib_available() { + eprintln!("Skipping test: figure parity needs matplotlib importable from python3"); + return; + } + + let knitr = record_one(&knitr_doc("plot(1)"), "knitr"); + let jupyter = record_one( + &jupyter_doc("import matplotlib.pyplot as plt\nplt.plot([1,2,3]);"), + "jupyter", + ); + + let k_ast = parse_capture_markdown(&knitr); + let j_ast = parse_capture_markdown(&jupyter); + assert_blocks_parity(&k_ast.blocks, &j_ast.blocks, "blocks"); + + for (engine, ast) in [("knitr", &k_ast), ("jupyter", &j_ast)] { + let images = collect_image_targets(&ast.blocks); + assert_eq!( + images.len(), + 1, + "{engine}: expected exactly one figure image; got {images:?}" + ); + assert!( + images[0].starts_with("doc_files/figure-html/"), + "{engine}: figure must live under doc_files/figure-html/; got {}", + images[0] + ); + } + + for (engine, capture) in [("knitr", &knitr), ("jupyter", &jupyter)] { + let supporting = capture + .result + .get("supporting_files") + .and_then(|v| v.as_array()) + .cloned() + .unwrap_or_default(); + assert!( + !supporting.is_empty(), + "{engine}: a figure-producing run must report supporting_files" + ); + } +} + +/// Run one engine over `content` and return its single capture. +fn record_one(content: &str, engine: &str) -> quarto_trace::EngineCapture { + let (_tmp, path, project, runtime) = fixture(content); + let captures = + pollster::block_on(record_capture(&path, &project, runtime, None)).expect("record_capture"); + assert_eq!(captures.len(), 1, "expected one capture for {engine}"); + assert_eq!(captures[0].engine_name, engine); + captures[0].clone() +} + +fn parse_capture_markdown(capture: &quarto_trace::EngineCapture) -> Pandoc { + let result_markdown = capture + .result + .get("markdown") + .and_then(|v| v.as_str()) + .unwrap_or(""); + let (pandoc, _, _) = pampa::readers::qmd::read( + result_markdown.as_bytes(), + false, + "capture-result.md", + &mut std::io::sink(), + false, + None, + ) + .expect("parse post-engine markdown"); + pandoc +} + +/// Collect every `Image` target in the block tree (top-level Divs and +/// Paragraphs are the only containers the cell shape produces). +fn collect_image_targets(blocks: &[Block]) -> Vec { + use quarto_pandoc_types::Inline; + fn walk_inlines(inlines: &[Inline], out: &mut Vec) { + for inline in inlines { + if let Inline::Image(img) = inline { + out.push(img.target.0.clone()); + } + } + } + fn walk(blocks: &[Block], out: &mut Vec) { + for block in blocks { + match block { + Block::Div(d) => walk(&d.content, out), + Block::Paragraph(p) => walk_inlines(&p.content, out), + Block::Plain(p) => walk_inlines(&p.content, out), + _ => {} + } + } + } + let mut out = Vec::new(); + walk(blocks, &mut out); + out +} + /// Two cells where the second depends on state from the first — pins /// both kernel-state persistence within a document (through the /// production text path) and the multi-cell output shape. From c42fcb0978a43437903fc39764a356deae8a3f42 Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Thu, 23 Jul 2026 16:28:30 -0500 Subject: [PATCH 3/3] bd-5t6wvu7m Phase 4: E2E record, plan doc, clippy fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the plan/E2E record (claude-notes/plans/2026-07-23-jupyter- figure-outputs.md): q2 render emits a real 556x413 PNG + , manifest records the Engine/jupyter resource; q2 preview capture embeds the PNG (bd-qbhp2cvv transport) and Chrome renders the matplotlib plot via blob URL — output inspected. Clippy: is_ok_and in the matplotlib probe. cargo xtask verify passes end to end. Co-Authored-By: Claude Fable 5 --- .../2026-07-23-jupyter-figure-outputs.md | 153 ++++++++++++++++++ .../tests/integration/engine_output_parity.rs | 3 +- 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 claude-notes/plans/2026-07-23-jupyter-figure-outputs.md diff --git a/claude-notes/plans/2026-07-23-jupyter-figure-outputs.md b/claude-notes/plans/2026-07-23-jupyter-figure-outputs.md new file mode 100644 index 000000000..6e0116e52 --- /dev/null +++ b/claude-notes/plans/2026-07-23-jupyter-figure-outputs.md @@ -0,0 +1,153 @@ +# Jupyter engine: emit real figures for image outputs + +**Braid strand:** bd-5t6wvu7m (pre-existing; carries the in-code TODO). +bd-rwz8kwia (filed from the bd-qbhp2cvv session) is a duplicate — closed +in favor of this one, evidence merged here. +**Status:** plan draft — awaiting review + +## Overview + +A minimal jupyter doc (python cell, `plt.plot([1,2,3])`) produces no +image in `q2 render` or `q2 preview` — the cell output is the text repr +(`[]` + `
`). +knitr produces real figures; jupyter must reach parity. Once fixed, the +bd-qbhp2cvv capture transport carries jupyter figures into the preview +with no further work. + +## Diagnosis (verified empirically, 2026-07-23) + +**The kernel already sends the PNG.** Probing a bare `python3` kernel +via `jupyter_client` with the matplotlib fixture yields: + +``` +[('execute_result', ['text/plain']), + ('display_data', ['image/png', 'text/plain'])] +``` + +No `MPLBACKEND` forcing or setup cell is needed for the basic case — +ipykernel's default `matplotlib_inline` integration ships the PNG. +q2 discards it in `format_outputs` +(`crates/quarto-core/src/engine/jupyter/text_execute.rs:427`), which has +two compounding bugs: + +1. **MIME priority is inverted.** `text/plain` is checked *first*, so + the display bundle (`image/png` + `text/plain` fallback) always + takes the text branch. That's why we see `
`. + Q1's `displayDataMimeType` + (`external-sources/quarto-cli/src/core/jupyter/display-data.ts:45`) + always puts `text/plain` **last**: for HTML targets the effective + order is widgets/javascript/`text/html` → `text/markdown` → + `image/svg+xml` → `image/png` → `image/jpeg` → … → `text/plain`. +2. **The image branch is a placeholder.** Even when reached, it emits + the literal `[Image output]` (the `TODO(bd-5t6wvu7m)` in the code) + instead of writing the bytes to a supporting file and emitting a + figure. + +Contrast with Q1's `mdImageOutput` +(`external-sources/quarto-cli/src/core/jupyter/jupyter.ts:1987`): base64- +decode (except literal `_files/figure-/.`, emit an image reference; +supporting files travel on the result. + +And with q2's knitr engine, which already does this via the vendored +hooks (`crates/quarto-core/src/engine/knitr/mod.rs:214` reports +`supporting_files`); `JupyterEngine::intermediate_files` already +declares `_files/` as its output dir, so downstream cleanup +expects exactly this layout. + +## Fix plan + +All in `crates/quarto-core/src/engine/jupyter/text_execute.rs` (plus the +parity suite): + +1. **MIME selection order** in `format_outputs`, per Q1's html-target + priority, scoped to the types q2 handles today: + `text/html` → `image/svg+xml` → `image/png` → `image/jpeg` → + `text/plain` (last). (`text/markdown` and widget payloads remain + out of scope; note as follow-up.) +2. **Real image emission.** Thread `ctx` (for `source_path`) and a + per-document figure counter into `render_cell`/`format_outputs`: + - target `_files/figure-html/cell--output-.` + next to the document (Q1's naming shape; `figure-html` matches + both Q1's `figuresDir("html")` and what knitr produces for our + HTML/preview pipeline); + - base64-decode PNG/JPEG (strip newlines first — nbformat multiline + convention); write SVG as text when it starts with `_files/figure-html/….png)` inside the + `.cell-output-display` div; + - collect the `_files` dir once into + `ExecuteResult::with_supporting_files` (mirroring knitr's + dir-level reporting, which bd-qbhp2cvv's `collect_capture_files` + walks recursively). +3. **Parity suite**: extend + `crates/quarto-core/tests/integration/engine_output_parity.rs` with + a plot case (knitr `plot(1)` vs jupyter matplotlib) asserting both + engines produce a `.cell-output-display` containing an image whose + target lives under `_files/figure-html/`, and that both + report non-empty `supporting_files`. (Requires R + jupyter — follow + the suite's existing skip conventions.) + +## Test plan (TDD) + +- [ ] Unit (no kernel): `format_outputs` on a hand-built + `KernelExecuteResult` whose display bundle has BOTH `image/png` + and `text/plain` → writes the PNG file, emits `![](…)`, no + `[Image output]`, no bare text repr. (Write first; fails today + by taking the text branch.) +- [ ] Unit: `image/svg+xml` written as text with `.svg` extension. +- [ ] Unit: `text/html` still beats images (DataFrame-style bundles). +- [ ] Unit: multiline base64 (nbformat array-of-lines) decodes. +- [ ] Integration: parity plot case (above). +- [ ] E2E: `q2 render pyfig.qmd` → ``, + file exists on disk, inspected. +- [ ] E2E: `q2 preview pyfig.qmd` → capture embeds the PNG + (bd-qbhp2cvv transport), browser shows blob-URL image. + +## Work items + +- [x] Phase 1: failing unit tests + MIME priority fix (landed with + Phase 2 — the signature change interlocks them) +- [x] Phase 2: image file emission + supporting_files (FigureWriter; + commit 69546b4d) +- [x] Phase 3: parity suite plot case (commit 7527df29). Shape decision + recorded there: jupyter figure divs converge on knitr's + `cell-output-display`-only class (Q1's engines disagree on this; + q2 resolves toward knitr). +- [x] Phase 4: E2E (render + preview) — record below + +## End-to-end verification record (2026-07-23) + +Exact invocations, observed output, output inspected — per the repo's +end-to-end policy. Fixture is the matplotlib doc from the Diagnosis +section, copied to a scratch dir. + +**`q2 render` (previously: text repr only, no image):** + +```bash +target/debug/q2 render pyfig.qmd +# output tree now contains: +# pyfig_files/figure-html/cell-1-output-1.png ← "PNG image data, 556 x 413" +# pyfig.html contains: +# +# .quarto/render-manifest.json records pyfig_files as +# {kind: Engine, engine: jupyter} — identical shape to knitr. +``` + +**`q2 preview` (previously: broken/no image):** + +```bash +target/debug/q2 preview pyfig.qmd --no-browser +# session capture (gunzip captures/*.bin): +# files: [{path: "pyfig_files/figure-html/cell-1-output-1.png", bytes(b64): 25512}] +# result.markdown contains ![](pyfig_files/figure-html/cell-1-output-1.png) +# supporting_files: [/pyfig_files] +# In Chrome (DevTools MCP): the rendered iframe contains +# {"src": "blob:http://127.0.0.1:58943/…", "loaded": true, "w": 556, "h": 413} +# and the screenshot shows the actual matplotlib line plot. This is the +# bd-qbhp2cvv transport carrying a jupyter figure end to end, as +# predicted — no further preview work was needed. +``` + +Note: the unsuppressed `[]` execute_result +line above the figure is correct notebook semantics (the cell has no +trailing `;`), matching Q1/Jupyter behavior — not a bug. diff --git a/crates/quarto-core/tests/integration/engine_output_parity.rs b/crates/quarto-core/tests/integration/engine_output_parity.rs index 40e9d901c..5c7d3bb89 100644 --- a/crates/quarto-core/tests/integration/engine_output_parity.rs +++ b/crates/quarto-core/tests/integration/engine_output_parity.rs @@ -245,8 +245,7 @@ fn matplotlib_available() -> bool { std::process::Command::new("python3") .args(["-c", "import matplotlib"]) .output() - .map(|o| o.status.success()) - .unwrap_or(false) + .is_ok_and(|o| o.status.success()) } /// Figure output parity (bd-5t6wvu7m): a plotting cell must produce