fix: clean up PDF tmpdir and validate --image_mode in PDF mode

[iamadhitya1]

Summary

Fixes two bugs in the PDF processing path of infer.py, both reported in issue #17.

Bug 1 — tmpdir leak (pdf_to_images, line 54)

pdf_to_images() created a temp directory with mkdtemp(prefix="pdf_ocr_") but never removed it. After a normal run or a crash, all rendered page PNGs were left behind in /tmp/pdf_ocr_*/ indefinitely. Long batch runs or repeated re-runs accumulate these silently.

Fix: pdf_to_images() now returns (image_paths, tmp_dir) instead of just image_paths. run() stores the path and deletes it in a try/finally block so cleanup happens whether inference succeeds or fails.

Bug 2 — --image_mode gundam silently accepted in PDF mode (build_jobs, line 241)

The README (lines 87, 93) states that PDF/multi-page input must use --image_mode base. With --image_mode gundam and a PDF, the script cropped every page into 640px tiles and fed them to the model with no warning — producing worse output than the documented setting.

Fix: build_jobs() raises ValueError immediately when --pdf is combined with any image_mode other than base.

Changes

• infer.py: add import shutil; pdf_to_images returns tuple[list[str], str]; build_jobs returns tuple[list, str | None] and validates image_mode; run unpacks the tuple and cleans up in try/finally
• tests/test_infer.py: 8 unit tests covering both fixes (mock-based, no GPU or model required)

Test plan

• python -m pytest tests/test_infer.py -v → 8 passed
• flake8 infer.py tests/test_infer.py --max-line-length=120 → no errors

[mensahamoskwaku-source]

PDF clean

[rajpratham1]

This is a well-scoped bug fix that addresses two practical issues together: temporary directories created during PDF processing are now cleaned up reliably, and invalid --image_mode values are rejected early for PDF inputs. I also like that the implementation includes comprehensive regression tests covering success, failure, validation, and cleanup paths.

[kushdab]

Good fix, and the --image_mode validation for PDF mode is a useful addition that the other tmpdir PRs didn't include.

On the tmpdir cleanup approach

Returning a TemporaryDirectory object from pdf_to_images() and keeping it alive in the caller is clean and idiomatic. Two small notes:

1. Caller must hold the reference. If someone copies pdf_to_images into their own script and writes paths, _ = pdf_to_images(...), the _ is discarded and the finalizer can run at GC time — in CPython that's immediate, but worth a docstring warning:

def pdf_to_images(pdf_path: str, dpi: int = 300):
    # Docstring note: caller must keep the returned TemporaryDirectory alive for
    # the duration of inference; discarding it releases the rendered PNGs immediately.

2. No max_pages guard — PR infer.py: CLI flags for ngram params, --resume, --results_jsonl, --max_pages/images, mode validation, tempdir cleanup #21 adds max_pages: int | None = None to cap PDF batch sizes. If both PRs are reviewed together, worth considering whether to include that here too.

On the --image_mode validation

if args.pdf and args.image_mode != "base":
    parser.error("--pdf mode requires --image_mode base")

This is the right guard. The gundam crop-mode path calls infer() (single-image), not infer_multi(), so it simply doesn't work on PDF page sequences. Failing early with a clear error beats silently producing wrong output.

Overlap note

This PR overlaps with PR #34 (PDF tmpdir portion only) and is also a subset of PR #21. If Baidu decides to merge #21 (which covers all of #17 comprehensively), this PR and #34 would both be superseded. Still worth merging standalone if the maintainers prefer incremental patches.

[iamadhitya1]

Thanks for the detailed review @kushdab.

Two clarifications and the fixes are now pushed:

On the GC concern: our implementation uses tempfile.mkdtemp() which returns a plain string path — not a TemporaryDirectory object — so there's no finalizer and no GC timing risk. Cleanup is done explicitly via shutil.rmtree in run()'s try/finally block. That said, the docstring suggestion is still valid for anyone reading the function in isolation. Added a docstring clarifying that the caller owns cleanup.

On parser.error(): moved the --image_mode validation out of build_jobs() and into parse_args() using parser.error() as suggested. This gives users a proper argparse error message and exits cleanly from the CLI entry point.

Updated the tests accordingly — validation is now tested at the parse_args level via SystemExit.