Skip to content

Make batch inference resumable and leak-free - #36

Open
RickyVishwakarma wants to merge 2 commits into
baidu:mainfrom
RickyVishwakarma:feat/resumable-batch-inference
Open

Make batch inference resumable and leak-free#36
RickyVishwakarma wants to merge 2 commits into
baidu:mainfrom
RickyVishwakarma:feat/resumable-batch-inference

Conversation

@RickyVishwakarma

Copy link
Copy Markdown

What

Hardens the SGLang batch runner (infer.py) for long-horizon documents.

  • Resume: skip pages whose output already exists; --overwrite forces a re-run.
  • Atomic writes: each page streams to a .part file, renamed only on completion —
    a present output file always means a fully parsed page, so resume is safe.
  • Fix temp-dir leak: rendered PDF pages now go to a temp dir that is cleaned up
    after the run; --keep_temp retains it.
  • Run manifest: writes <output_dir>/manifest.json with per-page status
    (ok/skipped/failed), tokens, decode time, and retry attempts.
  • Ctrl-C: cancels pending jobs, keeps finished pages, still writes the manifest.

Tests

Adds tests/test_infer.py (mocks the SGLang endpoint — runs CPU-only, no GPU/model)
and a GitHub Actions CI workflow, per CONTRIBUTING.md. All 11 tests pass locally.

Why

A long PDF that fails or is interrupted partway currently has to be re-run from page 1.
This makes batch runs idempotent and auditable.

Hardens the SGLang batch runner for long-horizon documents:

- Resume: skip pages whose output already exists; add --overwrite to force
- Atomic writes: stream each page to a .part file, rename on completion, so a
  present output file always means a fully parsed page (safe to resume on)
- Fix temp-dir leak: rendered PDF pages now go to a temp dir that is cleaned
  up after the run; add --keep_temp to retain it
- Emit <output_dir>/manifest.json with per-page status/tokens/timing/attempts
- Ctrl-C: cancel pending jobs, keep finished pages, still write the manifest

Adds tests/test_infer.py (mocks the SGLang endpoint, runs CPU-only without the
model) and a GitHub Actions CI workflow, per CONTRIBUTING.md.
@kushdab

kushdab commented Jun 26, 2026

Copy link
Copy Markdown

Well-designed PR — the atomic-write + resume + manifest combination is exactly what batch OCR pipelines need for production reliability. The implementation is clean throughout. A few observations:

What's particularly strong

  • .partos.replace() atomicity is the right pattern. A present .md file always means a fully parsed page, so resume is provably safe. remove_partial() on failure correctly cleans the .part sibling.
  • is_completed() checking both existence and non-empty catches the truncation edge case where a previous run created the file but wrote nothing.
  • executor.shutdown(wait=False, cancel_futures=True) on Ctrl-C cancels pending (not yet started) futures. Futures already in-flight run to completion (or raise) before shutdown(wait=True) returns — so the .part cleanup path in infer_one is always reached, and the manifest captures their final status. This is correct.
  • avg_decode is now filtered on status == "ok" rather than tokens > 0, which implicitly fixes the same averaging bug that PR Fix avg_decode calculation when decode_time is 0 for single tokens #13 targets — skipped requests contribute zero tokens but are excluded from the decode average by construction.

One functional gap: missing --image_mode validation for PDF mode

gundam uses crop-mode tiling (calling infer() per tile), which is designed for a single image. When --pdf is passed, infer_multi() is used under the hood — gundam silently produces wrong output there. PRs #26 and #21 both add this guard; it would be worth including here too:

# In parse_args(), after args = parser.parse_args():
if args.pdf and args.image_mode != "base":
    parser.error("--pdf requires --image_mode base (gundam is for single images only)")

Minor: manifest.json is a fixed name in output_dir

If two runs share an output_dir (e.g. resuming a PDF batch mid-way through, then running a second PDF into the same dir), the second run overwrites the first manifest. Could accept --manifest as an optional path that defaults to <output_dir>/manifest.json, or include the source filename in the manifest name (<prefix>_manifest.json). Low priority, but easy to add.

Overlap note

This PR covers the same ground as:

Of the tmpdir approaches across these three PRs, finally is the most reliable — it runs even if run() raises, which atexit also guarantees, but finally makes the scope explicit. The missing piece relative to PR #26 is the --image_mode validation noted above.

Overall this is ready to merge pending that one guard. ✅

gundam tiles a single image (crop_mode=True); multi-page/PDF inference only supports base, so reject the silently-wrong --pdf + gundam combo in parse_args() up front instead of producing wrong output.

Also fix the README batch example, which used --image_mode gundam for a PDF, and add ParseArgsValidationTest covering reject/accept cases.
@RickyVishwakarma

Copy link
Copy Markdown
Author

Added the --image_mode base guard for PDF mode in parse_args().

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a substantial and well-thought-out enhancement rather than just a bug fix. It makes long-running OCR jobs resumable, prevents partially written output files from being treated as completed work, adds interruption handling, introduces a manifest for tracking progress, and backs everything with comprehensive unit tests. The implementation also updates the documentation and CI, making the feature much easier to maintain.

@RickyVishwakarma

Copy link
Copy Markdown
Author

This is a substantial and well-thought-out enhancement rather than just a bug fix. It makes long-running OCR jobs resumable, prevents partially written output files from being treated as completed work, adds interruption handling, introduces a manifest for tracking progress, and backs everything with comprehensive unit tests. The implementation also updates the documentation and CI, making the feature much easier to maintain.

Not in progress — it's complete. Added the --image_mode base guard kushdab requested, with tests and a README fix. Ready to merge. Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants