fix(docling): thread page_from/page_to into Docling Serve request - #17451
Open
Elyytscha wants to merge 2 commits into
Open
fix(docling): thread page_from/page_to into Docling Serve request#17451Elyytscha wants to merge 2 commits into
Elyytscha wants to merge 2 commits into
Conversation
by_docling accepted from_page/to_page and called DoclingParser.parse_pdf() without them, and DoclingParser never set options.page_range on the payload. Every task of a page-split PDF therefore converted the whole document and RAGFlow sliced the result locally, so the work was repeated once per task and each request was as slow and as memory-hungry as the entire file. The sibling backends already forward their range: by_deepdoc, by_mineru (infiniflow#16957) and by_mistral_ocr. Thread page_from/page_to through by_docling -> parse_pdf -> _parse_pdf_remote, and translate them once in _resolve_page_range: RAGFlow is 0-based with page_to exclusive, Docling is 1-based with both bounds inclusive. The helper returns None when the caller did not narrow the range, so an unsplit document sends the request it sent before, and for a degenerate range, which Docling rejects with end < start. page_range is set on all four payload variants the fallback chain can reach, and on the local DocumentConverter.convert() path. No page-number rebasing is needed, unlike MinerU: Docling converts only the requested pages but still numbers them from the start of the document, so page_from/page_to are deliberately left at their defaults and the existing crop/_make_line_tag/cropout_docling_table arithmetic is unchanged.
dosubot
Bot
added
size:M
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocling parsing now accepts RAGFlow page bounds, translates them to Docling’s page convention, and forwards them through local conversion and remote fallback payloads. The Docling application caller now supplies task page bounds, with tests covering ranged and full-document requests. ChangesDocling page-range handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deepdoc/parser/docling_parser.py (1)
140-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove compatibility-history wording from the new range documentation.
deepdoc/parser/docling_parser.py#L140-L142: describeNoneas the full-document/degenerate-range behavior without referring to earlier requests.deepdoc/parser/docling_parser.py#L454-L456: state that full-document requests omitpage_range, without “byte-for-byte” history.test/unit_test/deepdoc/parser/test_docling_parser_remote.py#L213-L215: describe the asserted omission behavior directly.As per coding guidelines, “do not add compatibility wording to comments or documentation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deepdoc/parser/docling_parser.py` around lines 140 - 142, Remove compatibility-history wording from the range documentation and test description: in deepdoc/parser/docling_parser.py lines 140-142, describe None as the full-document or degenerate-range behavior; in deepdoc/parser/docling_parser.py lines 454-456, state directly that full-document requests omit page_range; and in test/unit_test/deepdoc/parser/test_docling_parser_remote.py lines 213-215, describe the asserted omission behavior directly without referring to prior requests or byte-for-byte compatibility.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deepdoc/parser/docling_parser.py`:
- Line 655: Update the local-path image rendering call in __images__() to pass
page_range[0] - 1 and page_range[1] when a page range is selected, while
preserving default full-document bounds otherwise. Keep conv.convert() aligned
with the same range, ensure self.page_from reflects the narrowed image set, and
add a local-path test covering page-range rendering and alignment.
- Around line 134-148: Update _resolve_page_range to clamp page_from to the
valid nonnegative source range and page_to to MAXIMUM_PAGE_NUMBER before
converting to Docling’s 1-based inclusive bounds. Preserve the None behavior for
an unrestricted or empty range, and add assertions confirming the returned start
and end are valid 1-based bounds with the end not preceding the start.
---
Nitpick comments:
In `@deepdoc/parser/docling_parser.py`:
- Around line 140-142: Remove compatibility-history wording from the range
documentation and test description: in deepdoc/parser/docling_parser.py lines
140-142, describe None as the full-document or degenerate-range behavior; in
deepdoc/parser/docling_parser.py lines 454-456, state directly that
full-document requests omit page_range; and in
test/unit_test/deepdoc/parser/test_docling_parser_remote.py lines 213-215,
describe the asserted omission behavior directly without referring to prior
requests or byte-for-byte compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 315ab300-bbcd-4ace-9d03-14f9387ec8a3
📒 Files selected for processing (3)
deepdoc/parser/docling_parser.pyrag/app/naive.pytest/unit_test/deepdoc/parser/test_docling_parser_remote.py
Follow-up to review feedback on the page-range change. _resolve_page_range trusted its inputs, so a negative page_from produced a 0-based start, which Docling rejects outright. Clamp both bounds before translating, since parse_pdf is public and its callers are not required to pre-clamp. The local path narrowed conv.convert() but still rasterised the whole document in __images__, so a page-split task kept the full-document CPU and memory cost and self.page_from did not describe the images it had rendered. Render the same window the conversion covers. That makes page_from non-zero for the first time, which _make_line_tag was not written for: it indexed page_images with Docling's absolute page number, while crop() adds page_from back when it turns a tag into a position. Emit window-relative page numbers instead, matching what cropout_docling_table already indexes by, so a tag and the page it names stay aligned.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17451 +/- ##
==========================================
- Coverage 94.56% 90.65% -3.91%
==========================================
Files 10 10
Lines 717 717
Branches 118 118
==========================================
- Hits 678 650 -28
- Misses 25 39 +14
- Partials 14 28 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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
rag/app/naive.py::by_doclingacceptsfrom_page/to_pageand then callsDoclingParser.parse_pdf()without them, andDoclingParsernever setsoptions.page_rangeon the docling-serve payload. So when RAGFlow splits a PDFinto
task_page_sizetasks, every task converts the whole document andRAGFlow slices the result locally. The
Page(145~157)prefix in the progress logis RAGFlow's own task label, not what Docling was asked to convert.
Every sibling backend already forwards its range —
by_deepdoc(
naive.py:119),by_mineru(naive.py:175-176, since #16957) andby_mistral_ocr(naive.py:394-395). Docling is the one that drops it. This isthe Docling half of the same bug fixed for MinerU in #16957 (issues #16586,
#16971).
Repro steps
DOCLING_SERVER_URL.layout_recognize = "Docling".task_page_size(default 12) pages, so RAGFlowcreates more than one task.
/v1/convert/sourcerequest, or the Docling Serve logs.Every task posts the whole document with no
page_range, and every task returnsthe whole document's content.
Expected
Each task converts only its own pages. Docling Serve already supports this:
ConvertDocumentsOptions.page_range(docling.datamodel.service.options),documented as "Only convert a range of pages. The page number starts at 1."
RAGFlow's
from_page/to_pageare 0-based withto_pageexclusive (Pythonslice stop); Docling's
page_rangeis 1-based with both bounds inclusive.No page-number rebasing is needed, unlike MinerU: Docling converts only the
requested pages but still numbers them from the start of the document
(
docling/pipeline/base_pipeline.pybuildsPage(page_no=i + 1)from thefull-document index), so positions and citations stay correct.
DoclingParserwas evidently written expecting a ranged response — it already keeps
page_from/page_toand offsets result page numbers by them(
docling_parser.py:212,286); only the wiring was missing.Fix
page_from/page_tothroughby_docling→parse_pdf→_parse_pdf_remoteinrag/app/naive.pyanddeepdoc/parser/docling_parser.py, clampingto_pagetoMAXIMUM_PAGE_NUMBERthe wayby_minerudoes.DoclingParser._resolve_page_range, which returnsNonewhen the caller did not narrow the range, so an unsplit document sendsbyte-for-byte the request it sent before. It also returns
Nonefor adegenerate range: Docling's
_validate_page_rangerejectsend < start, whichwould otherwise 422 every payload variant and turn a no-op task into a hard
RuntimeError.options.page_rangeon all four payload variants the fallback chain canreach (chunked/standard × v1/v1alpha).
page_rangeon the localDocumentConverter.convert()path too.self.page_from/self.page_toare deliberately left alone: the local path stillrenders the whole document in
__images__, and Docling returns absolute pagenumbers, so
_make_line_tag,cropandcropout_docling_tablekeep the pagearithmetic they have today.
Out of scope, mentioned for whoever hits them next:
by_opendataloaderandby_tcadphave the same gap,DoclingParser.__init__'s hardcodedrequest_timeout: int = 600has no env override, and the local path stillrenders every page of the document per task.
This overlaps the Docling part of #16857, which also fixes OpenDataLoader wiring
and MinerU and has been open since 13 July; its MinerU half now conflicts with
the merged #16957. This PR is the Docling-only slice, rebased onto the
page_from/page_toconvention that #16957 established, so it can landindependently. Happy to close this in favour of that PR if it is revived.
Tests
Three regression tests in
test/unit_test/deepdoc/parser/test_docling_parser_remote.py, mirroring the ones#16957 added for MinerU:
_resolve_page_range, including the un-narrowed anddegenerate cases;
"page_range": [145, 157]on every payloadthe fallback chain sends (both chunked attempts and the standard fallback);
page_rangeat all.11 passed (8 existing + 3 new).
ruff format --checkclean on the three touchedfiles.
Verified in production on a 2383-page / 22.6 MB PDF against Docling Serve v1.21.0
with a vLLM-backed
vlmpipeline: each task drops from 2383 pages / ~1432s —which always hit RAGFlow's hardcoded 600s client timeout, so the ingest could
never finish — to 13 pages / ~10s, and Docling Serve stops being OOMKilled at
16 GiB.
Fixes #17450