fix(ocr): stop task="parse" rejecting ordinary multi-page PDFs - #5308
Conversation
A 31-page A4 text PDF was rejected at every zoomin from 1 to 6, and the 400 advised lowering `zoomin`, which made the computed budget larger. Two independent causes: * Every page was budgeted for the 9x retry, which deepdoc only performs when the whole document yields no OCR boxes at all. An A4 page renders to 4.5 MP at zoomin 3 but was charged 45 MP, putting the whole-document ceiling at ~22 A4 pages. * The retry ladder is not monotonic in zoomin. deepdoc-lib tests `zoomin < 9` before multiplying, so 2 and 6 both escalate to 18x while 3 stops at 9x. "Lower `zoomin`" was therefore unsound advice. The whole-document budget is now enforced at the requested scale, with a separate, looser ceiling bounding what the escalated re-render would cost if the retry does fire. The per-page ceiling stays at the worst-case scale, so a single outsized MediaBox is still rejected. That keeps the memory-safety intent: an escalated document is held to ~12 GB of page images instead of the 32-130 GB the requested-scale budget alone would have allowed. Rejections now name a zoomin that actually fits, searching the whole permitted range rather than only downwards, and fall back to advising a split when no zoom works. Both whole-document ceilings are configurable via XINFERENCE_MAX_PDF_PARSE_TOTAL_PIXELS and XINFERENCE_MAX_PDF_PARSE_RETRY_TOTAL_PIXELS. The 31-page document now parses at the default zoomin 3, and the rejections at 2, 5 and 6 all point at zoomin 4. Ref xorbitsai#5307
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PDF parsing size limit logic to prevent false-positive rejections of standard documents. It splits the whole-document budget into a requested-scale ceiling and a looser retry-scale ceiling, allows configuring these ceilings via environment variables, and improves error messages by suggesting a specific fitting zoom factor. The review feedback correctly identifies a critical runtime bug where formatting integers with the :g format specifier in f-strings will raise a ValueError and crash the application.
End-to-end verification on GPU hardwareVerified on an RTX 3090 Ti box with real DeepDoc inference, not just the validation layer. Both runs used the same 31-page A4 text PDF (595.28 x 841.89 pt), the same DeepDoc model from ModelScope, and Before (unpatched, at
|
zoomin |
reported total | worst case |
|---|---|---|
| 1 | 1,037,233,622 | 9x |
| 2 | 1,082,382,462 | 18x |
| 3 | 1,037,233,622 | 9x |
| 4 | 1,042,280,369 | 12x |
| 5 | 1,002,251,168 | 15x |
| 6 | 1,082,382,462 | 18x |
Every message ended in lower `zoomin` or split the document, while zoomin=2 was budgeted at 18x — four times zoomin=3.
(The totals differ from the issue's by ~0.02% only because this document's MediaBox is 595.28 pt rather than an exact 595.)
After (this PR, same box, same document)
zoomin |
result |
|---|---|
| 1 | parsed, 1054 elements, 42.0s |
| 2 | 400 → retry with `zoomin` 4 |
| 3 (default) | parsed, 1054 elements, 32.1s |
| 4 | parsed, 1054 elements, 40.7s |
| 5 | 400 → retry with `zoomin` 4 |
| 6 | 400 → retry with `zoomin` 4 |
Element counts are identical across all three accepted zooms, so the budget change does not alter parse output.
The advice is actionable
Followed the message mechanically — request at zoomin=6, take the value it names, retry:
zoomin=6 -> 400 "...exceeding the retry limit of 3000000000; retry with `zoomin` 4"
zoomin=4 -> OK task=parse elements=1054
types: {'text': 1023, 'title': 31}
first text: "Chapter 1: Evaluation of Document Parsing"
1023 text + 31 title matches the document's structure (one heading per page), and the text matches the source. The recommended zoom is higher than the request — advice the old "lower zoomin" message could not have produced, and which lowering to 5 would not have found either (15x, also rejected).
Unrelated environment note
Launching DeepDoc on a fresh venv failed with ImportError: cannot import name 'is_offline_mode' from 'huggingface_hub' — the venv resolves huggingface_hub==0.36.2 while the launching environment has 1.26.1, and the venv is rebuilt back to 0.36.2 on each restart. Pinning the venv to 1.26.1 fixes it. This is unrelated to this PR (it reproduces on unpatched e45894915 too) and looks like the same class of issue as the deepdoc-lib hub pin from #5230; worth tracking separately.
Review found the retry budget rests on a condition deepdoc-lib 0.2.2 cannot reach. `__ocr` appends to `self.boxes` on every page -- `append([])` when a page yields nothing, `append(bxs)` otherwise -- so after the OCR pass `len(self.boxes)` equals the page count. The `len(self.boxes) == 0` guard the 9x re-render sits behind is therefore only true when there were no pages to render at all, i.e. the load failed, in which case there is nothing to re-render. Budgeting the whole document for that pass rejected a 74-page A4 PDF whose real render is ~334 M pixels. Removing it also resolves the peak undercount raised alongside it: with no aggregate retry budget there is no figure to undercount. The per-page ceiling is still checked at the worst-case scale. That is cheap insurance against a single outsized MediaBox and does not depend on this reasoning holding for every deepdoc-lib version -- and it is what keeps the budget non-monotonic in zoomin, so the recommendation search that motivated this PR is still load-bearing. Also drops the `:g` format specifiers on integer zoom values; they are meaningless for ints and would render large values in scientific notation. The 31-page A4 document from xorbitsai#5307 now parses at every zoomin from 1 to 6.
Updated following review — the retry budget is goneThe review established that the 9x re-render is unreachable, which invalidated the premise the retry budget rested on.
This also retroactively explains an observation I had recorded but not chased: across six end-to-end runs on real hardware, a 9x re-render never occurred. What changed since the first review
The per-page ceiling still runs at the worst-case scale. It is cheap, it guards a single outsized MediaBox, and it does not depend on the reachability argument holding for every Also dropped the UpstreamThe predicate still looks wrong upstream: Verificationpre-commit (black, ruff, isort, mypy, codespell) passes on all changed files. The GPU end-to-end results posted earlier were produced with the retry budget still in place; they remain valid as a before/after for the original bug, and the change since only widens what is accepted, with the accepted set now a superset of what was verified there. |
The docstrings still described the 9x re-render as something that fires when a page yields no boxes, and justified the per-page ceiling by a retry 'that may still fire'. Both predate the review finding that the re-render is unreachable. The per-page ceiling is kept as insurance against that reasoning changing, not because the retry is expected.
The document-wide budget sits at the requested scale on the strength of the 9x retry being unreachable in deepdoc-lib 0.2.2, but the dependency is `~=0.2.2` and accepts later 0.2.x, so that could change. What bounds the damage if it does is not that reasoning but the per-page ceiling, which is still enforced at the worst-case scale, times the page ceiling: no page may peak above 200 MP even after escalating and at most 200 pages are accepted, so an escalated document cannot exceed 40 G px. Searching every page geometry and zoom both budgets admit, the true maximum is 39.96 G px, from 200 pages of 200x11100 pt at zoomin 1. Pinned as a test so raising either limit has to face what it does to the escalated worst case.
The previous reasoning was that the per-page ceiling times the page ceiling already bounds the escalated case at 40 Gpx. That is a mathematical bound, not a memory-safety one: at ~4 bytes per pixel it permits ~160 GB of page images, and the concrete case of 200 A4 pages escalating 3 -> 9 is 9.0 Gpx (~36 GB) -- inside both existing limits and still enough to OOM an ordinary worker. Dropping the aggregate ceiling was a regression against the ~4 GB it used to provide. Restores it at 6 Gpx (~24 GB), summed from the per-page peaks so the render being replaced is counted alongside its replacement. The reason the earlier 3 Gpx value looked unusable was a mistake on my part: it does not reject the reported document. The 31-page A4 PDF peaks at 1.4 Gpx even at 9x, so it cleared that ceiling all along -- what rejected it was budgeting every page at the worst-case scale, which is what this PR removed. At 6 Gpx it now passes at every zoomin from 1 to 6, a 100-page A4 document still parses at the default zoom, and the 36 GB case is rejected. Tunable via XINFERENCE_MAX_PDF_PARSE_RETRY_TOTAL_PIXELS.
End-to-end re-verified on GPU with the retry ceiling in placeRe-ran on the RTX 3090 Ti against real DeepDoc inference, with all four commits applied ( The reported document — 31 pages
Every zoomin now works, where the previous revision of this PR only admitted 1, 3 and 4. Element counts are identical across all of them. The ceiling does what it is there for
The 200-page case is the ~36 GB scenario raised in review, now rejected. The 100-page case confirms the ceiling is not so tight that it costs ordinary long documents the default zoom. Followed the advice mechanically once more: 100 pages at One thing worth recording, unrelated to this PRRunning five or six large parses back-to-back against a single model instance eventually fails with CleanupThe test instance, repo copy, test home and PDFs were removed; GPU memory and the four pre-existing services are back to their prior state. |
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
One non-blocking documentation consistency issue remains after the final retry-ceiling change.
Restoring the aggregate retry ceiling left several comments describing the state between the two revisions: the constant block claimed there was no document-wide retry ceiling immediately above the one that defines it, and both `_parse_budget_error` and `validate_pdf_for_parse` documented two budgets where there are now three. Two test comments also still described 200-page A4 documents as required to be admitted, which their own assertions had already stopped claiming. All of them now describe the shipped policy: per-page at the worst-case scale, whole-document at the requested scale, and a looser whole-document ceiling on the escalated peak. No behaviour change.
3 participants
What
Fixes the size-budget defect in
task="parse"reported in #5307: an ordinary 31-page A4 text PDF (393 KB) was rejected at everyzoominfrom 1 to 6, and the 400 advised loweringzoomin, which makes the computed budget larger.Why it happened
Two independent causes, both confirmed against
deepdoc-lib0.2.2 rather than assumed:1. Every page was budgeted for a conditional retry.
deepdoc/parser/pdf_parser.pyre-renders atzoomin * 3only whenlen(self.boxes) == 0.boxesaccumulates across every page, so the escalation fires only if not one page in the whole document produced a single box — a scanned or broken PDF, not a text document. An A4 page renders to 4.5 MP atzoomin=3but was charged 45 MP, putting the whole-document ceiling at ~22 A4 pages where actual usage allows ~221.2. The retry ladder is not monotonic in
zoomin. The guard tests the pre-multiplication value:so
2 → 6 → 18and6 → 18, while3 → 9. "Lowerzoomin" was therefore not sound advice. The server'sworst_case_parse_zoommodels this faithfully — the arithmetic was never the bug.What changed
MAX_PDF_PARSE_RETRY_TOTAL_PIXELS, 3 G px). Without it, budgeting only at the requested scale would have left a 32–130 GB hole when the retry does fire — so this is not simply raising the ceiling. The escalated render replaces the first one (the retry re-enters__images__, rebindingself.page_images), which is why the two ceilings differ rather than summing.zoominthat actually fits, searching the whole permitted range rather than only downwards — with a non-monotonic ladder, a higher zoom can be the one that fits. When nothing fits, it advises splitting instead.XINFERENCE_MAX_PDF_PARSE_TOTAL_PIXELSandXINFERENCE_MAX_PDF_PARSE_RETRY_TOTAL_PIXELS.Memory safety
The intent is preserved rather than traded away. An accepted document is bounded at the requested scale (~4 GB of page images at 1 G px), and if the retry fires the escalated document is bounded at ~12 GB — uniformly, regardless of
zoomin, versus 32–130 GB under requested-scale budgeting alone. The binding constraint on long documents is now the retry ceiling at ~73 A4 pages, which is honest: a 200-page A4 document really would need ~32 GB if it escalated.Before / after on the reported document
31-page A4, 595x842 pt:
zoominzoomin4"zoomin4"zoomin4"Every rejection now names zoomin 4 — which genuinely works, and is higher than the old advice would have sent the caller.
Upstream
The non-monotonic ladder belongs in
deepdoc-lib, not here. Clamping it tomin(zoomin * 3, 9)would cap the real allocation and make the scale monotonic, at which point the retry ceiling here could be tightened. I have deliberately not clamped it server-side: that would model an escalation bound the parser does not actually honour. Worth filing upstream separately; this PR handles the server-side half.Tests
Added to
xinference/api/tests/test_ocr_pdf.py, including the two cases that would have caught this: a realistic 31-page A4 document being accepted at the default zoom, and the property that a rejection always carries a true way forward (parametrized over 1/5/31/100/200 pages — either a named zoom that itself validates, or advice to split). Also covers the upward-search gap, the retry-budget invariant, and env-var overrides.pre-commit run --files ...passes on all changed files (black, ruff, isort, mypy, codespell).Docs updated in
doc/source/models/model_abilities/image.rst, including an explicit warning that loweringzoomincan make the budget larger.Ref #5307