[FEAT][FIX] Speed up PDF rendering and add report --pdf-dir - #90
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, low-risk, and include targeted unit tests covering the new path-resolution behavior and CLI parsing.
Pull request overview
This PR improves PDF reporting in oceanarray by (1) speeding up WeasyPrint PDF rendering via a print-CSS table layout change and (2) adding an opt-in report --pdf-dir DIR flag to centralize combined PDF outputs into a shared directory.
Changes:
- Set print CSS tables to
table-layout: fixed(withwidth: 100%) to avoid pathological intrinsic-width scans during PDF rendering. - Introduce
paths.resolve_pdf_path()and wire it intocmd_reportfor both dry-run preview and real PDF generation, plus add the--pdf-dirCLI flag. - Add unit tests for PDF path resolution and CLI parsing of
--pdf-dir.
File summaries
| File | Description |
|---|---|
tests/unit/test_paths.py |
Adds unit tests for resolve_pdf_path including default behavior, override behavior, and purity (no mkdir side effects). |
tests/unit/test_cli.py |
Extends CLI parser tests to cover report --pdf-dir parsing and default None behavior. |
oceanarray/reports/_pdf.py |
Updates print CSS to use fixed table layout for major WeasyPrint performance gains on wide/large cells. |
oceanarray/paths.py |
Adds resolve_pdf_path() as a single source of truth for combined PDF output location selection. |
oceanarray/cli.py |
Adds --pdf-dir to report, uses resolve_pdf_path() in dry-run and real PDF flow, and passes an explicit output_path to the PDF combiner. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two PDF-report changes. First, a print-CSS fix that cuts combined-PDF render time from ~50s to ~6s by giving tables a fixed layout. Second, a new
report --pdf-dir DIRflag that writes each mooring's combined PDF into one shared directory instead of beside its HTML, so PDFs from many moorings collect in one shareable place. The flag is opt-in and changes nothing when unset.What's changed
PDF render speedup (
reports/_pdf.py). The print stylesheet used the defaulttable-layout: auto, which makes WeasyPrint scan every cell to compute intrinsic column widths. A ~7.7 KB one-line seasenselibraw-opaqueprovenance JSON in an ADCP global-attributes table made that pass pathological — one page took 47s and the whole report ~50s. Settingtable-layout: fixedwith an explicitwidth: 100%skips the intrinsic-width scan (columns size from the first row, long values wrap), dropping the report to ~6s. Print-only, so on-screen tables are unaffected; cross-page links still resolve.report --pdf-dir DIRflag.paths.resolve_pdf_path(mooring, pdf_dir, report_html_dir): with--pdf-dirset, the combined PDF is written toDIR/{mooring}_report.pdf; unset, it stays beside the HTML atreport_html_dir/{mooring}_report.pdf. The resolver is pure — it creates no directory — so the dry-run preview stays side-effect-free.cmd_reportwires the resolver into both the dry-run preview and the real PDF branch, so the two can't drift. The real branchmkdirs the parent and passesoutput_pathtocombine_mooring_pdf.--pdf-dir DIRargument to thereportsubparser. It only affects PDF placement; building the PDF still needs--pdfor--all.rundoes not build PDFs, so it is untouched.resolve_pdf_path(default beside-HTML,--pdf-diroverride, and a purity check that no directory is created) plus two--pdf-dirCLI-parse cases.Breaking changes
None.
--pdf-diris opt-in; with it unset, PDF output is byte-for-byte where it was before. The speedup is print-CSS only and does not change output content.Notes
Follow-up (not in this PR): the raw-opaque provenance blob is ugly in a table regardless of layout — truncate it or wrap it in
<details>.