Skip to content

Catch generic exception when export to pdf#8704

Merged
mscolnick merged 4 commits intomainfrom
sham/catch-generic-error
Mar 16, 2026
Merged

Catch generic exception when export to pdf#8704
mscolnick merged 4 commits intomainfrom
sham/catch-generic-error

Conversation

@Light2Dark
Copy link
Collaborator

@Light2Dark Light2Dark commented Mar 16, 2026

📝 Summary

Relates to #8700.

We previously only checked for OSError. It seems that this isn't sufficient to capture missing Pandoc. So instead, we catch a generic exception. Standard PDF tends to be quite strict anyway compared to WebPDF, so this is a good fallback

🔍 Description of Changes

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • Tests have been added for the changes made.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Pull request title is a good summary of the changes - it will be used in the release notes.

@vercel
Copy link

vercel bot commented Mar 16, 2026

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

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment Mar 16, 2026 4:53pm

Request Review

@Light2Dark Light2Dark added the bug Something isn't working label Mar 16, 2026
@Light2Dark Light2Dark requested a review from Copilot March 16, 2026 14:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the server-side PDF export flow to fall back to WebPDFExporter when standard nbconvert.PDFExporter fails due to non-OSError conversion issues (e.g., missing Pandoc), and adds a regression test for that fallback.

Changes:

  • Broaden standard PDF export failure handling from OSError to Exception to trigger webpdf fallback for more nbconvert conversion errors.
  • Add a test ensuring PandocMissing triggers fallback to WebPDFExporter.

Reviewed changes

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

File Description
marimo/_server/export/exporter.py Catch generic exceptions during standard PDF export and warn + fall back to webpdf.
tests/_server/export/test_exporter.py Add regression test for fallback behavior when PandocMissing is raised.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves Marimo’s PDF export reliability by expanding failure handling for “standard” nbconvert PDF generation (pandoc/TeX) and validating fallback to WebPDF.

Changes:

  • Catch additional nbconvert conversion exceptions (including pandoc-missing scenarios) and fall back to WebPDF.
  • Add test coverage for fallback behavior on conversion and unexpected errors.
  • Update PDF export tests’ dependency skip conditions (now including playwright).

Reviewed changes

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

File Description
marimo/_server/export/exporter.py Expands exception handling in export_as_pdf to fall back to WebPDF when standard PDF conversion fails.
tests/_server/export/test_exporter.py Adjusts skip conditions and adds new tests covering fallback behavior for additional exception types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


# Try standard PDF export first (requires pandoc + TeX)
# and fall back to webpdf if it fails
from nbconvert.utils.exceptions import ( # type: ignore[import-not-found]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should probably add nbconvert to our test-optional group

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, ill add in a followup

@Light2Dark Light2Dark marked this pull request as ready for review March 16, 2026 16:54
@mscolnick mscolnick merged commit 3e9642b into main Mar 16, 2026
41 of 45 checks passed
@mscolnick mscolnick deleted the sham/catch-generic-error branch March 16, 2026 18:00
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