Feat/marker pdf chunking - #297
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces MARKER_MIN_PROCESSES with MARKER_CHUNK_SIZE to split PDFs into page-range chunks processed in parallel; adds pypdfium2 page counting; refactors pool health to detect broken executors; adds VDB_ENABLE_INSERTION; updates docs, helm values, and config schemas accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MarkerPool as MarkerPool
participant PdfLib as pypdfium2
participant Executor as ProcessPoolExecutor
participant Worker as MarkerWorker
Client->>MarkerPool: process_pdf(file_path)
MarkerPool->>PdfLib: get_page_count(file_path)
PdfLib-->>MarkerPool: total_pages
MarkerPool->>MarkerPool: _create_chunks(total_pages, marker_chunk_size)
MarkerPool-->>MarkerPool: chunks = [range0, range1, ...]
par process chunks
MarkerPool->>Executor: submit(_process_chunk, file_path, page_range=rangeN)
Executor->>Worker: process_pdf(file_path, page_range=rangeN)
Worker-->>Executor: {markdown_chunk, images_chunk}
Executor-->>MarkerPool: chunk_result_N
end
MarkerPool->>MarkerPool: reassemble(chunk_results in order)
MarkerPool-->>Client: {full_markdown, merged_images}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
openrag/components/indexer/loaders/pdf_loaders/marker.py (1)
155-159: Avoid hard-depending onProcessPoolExecutor._broken.
_brokenis private stdlib state. If the target Python runtime changes or removes it, this health check will start raising instead of recreating the pool. At minimum guard it withgetattr(...); longer-term it would be safer to reinitialize from submit/result failures instead of probing internals.🛡️ Minimal defensive guard
- return self.executor is None or self.executor._broken + return self.executor is None or bool(getattr(self.executor, "_broken", False))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/loaders/pdf_loaders/marker.py` around lines 155 - 159, The is_pool_broken method currently accesses the private attribute executor._broken; change it to defensively check using getattr(self.executor, "_broken", False) (and still treat executor is None as broken) so it won't raise if _broken is absent, i.e., return self.executor is None or getattr(self.executor, "_broken", False); consider a follow-up to remove probing internals entirely by detecting failures on submit/result and reinitializing the pool then.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/marker/marker_page_chunking.md`:
- Around line 53-55: The markdown fenced block containing the expression
"available_gpu_mem >= max_spike * num_workers + marker_model_gpu_size +
other_gpu_processes" should be changed from an untyped fence to a typed one to
satisfy markdownlint (MD040); update the fence from ``` to ```text around that
expression so the block is explicitly marked as text (locate the fenced block
that holds the GPU capacity expression shown in the diff).
In `@docs/content/docs/documentation/env_vars.md`:
- Around line 42-50: The documentation currently hardcodes performance numbers
for MARKER_CHUNK_SIZE (e.g., "up to 10x faster" and "~1 GB per worker")—update
the MARKER_CHUNK_SIZE note to either (a) reference the specific benchmark and
its test conditions (link or cite the bench file and state worker count,
environment, and chunk size used) when keeping numbers, or (b) remove precise
numeric claims and replace them with qualitative guidance (e.g., "significantly
faster" and "reduces peak memory usage") plus a suggestion to consult
benchmarks/marker/marker_page_chunking.md for measured results; adjust the
paragraph mentioning MARKER_MAX_TASKS_PER_CHILD accordingly so it remains
generic guidance rather than a hard claim.
- Line 176: Update the docs row for `VDB_ENABLE_INSERTION`: change the default
from `false` to `true` to match the actual defaults in
`openrag/config/models.py` (where `vectordb.enable` is set) and
`conf/config.yaml`; also correct the typo "texting" to "testing" in the
description. Ensure the table cell for `VDB_ENABLE_INSERTION` reflects `true`
and the explanation reads "...Useful for testing."
In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 253-255: The current call to
asyncio.gather(*[self._process_chunk(...) for ...]) lets other chunk coroutines
keep running if one fails; change this to create explicit asyncio.Task objects
for each self._process_chunk call (e.g., tasks =
[asyncio.create_task(self._process_chunk(...)) for ...]), use
asyncio.wait(tasks, return_when=asyncio.FIRST_EXCEPTION) to detect the first
failure, cancel all pending tasks (for t in pending: t.cancel()), then await
asyncio.gather(*tasks, return_exceptions=False) or re-raise the caught exception
so the failure propagates and sibling tasks are not left running; update the
code around results = await asyncio.gather(...) to implement this pattern
referencing self._process_chunk, chunks, tasks, pending, and FIRST_EXCEPTION.
---
Nitpick comments:
In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 155-159: The is_pool_broken method currently accesses the private
attribute executor._broken; change it to defensively check using
getattr(self.executor, "_broken", False) (and still treat executor is None as
broken) so it won't raise if _broken is absent, i.e., return self.executor is
None or getattr(self.executor, "_broken", False); consider a follow-up to remove
probing internals entirely by detecting failures on submit/result and
reinitializing the pool then.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 418a8d6e-0506-498f-a231-ad0856e2c060
📒 Files selected for processing (13)
benchmarks/marker/marker_page_chunking.mdbenchmarks/workspace/README.mdbenchmarks/workspace/docker-compose.ymlbenchmarks/workspace/requirements.txtbenchmarks/workspace/results_workspace.mdbenchmarks/workspace/workspace.pycharts/openrag-stack/values.yamlconf/config.yamldocs/content/docs/documentation/deploy_ray_cluster.mddocs/content/docs/documentation/env_vars.mdopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/config/loader.pyopenrag/config/models.py
💤 Files with no reviewable changes (2)
- charts/openrag-stack/values.yaml
- docs/content/docs/documentation/deploy_ray_cluster.md
…kers Split large PDFs into configurable page chunks (MARKER_CHUNK_SIZE) and dispatch them to available workers concurrently, reducing memory spikes and enabling safer scaling of worker count. Also replace the unreliable process-counting health check with executor broken-state detection, and remove the now-unused MARKER_MIN_PROCESSES config.
Ahmath-Gadji
force-pushed
the
feat/marker_pdf_chunking
branch
from
April 8, 2026 16:11
22d96ec to
e3a56d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 217-234: The worker dequeued from self._queue in the
_process_chunk method can be lost if ensure_worker_pool_healthy raises, so
ensure the worker is always requeued: after obtaining worker = await
self._queue.get(), immediately wrap the subsequent logic in a try/finally (or
catch exceptions from ensure_worker_pool_healthy and re-put the worker before
re-raising) so that await self._queue.put(worker) in the finally block always
runs; keep the processing call to worker.process_pdf.remote and
call_ray_actor_with_timeout inside the try block and rethrow any errors after
requeuing.
- Line 7: The file marker.py imports pypdfium2 directly (import pypdfium2), but
pypdfium2 is only a transitive dependency; add pypdfium2 as a direct dependency
in pyproject.toml under [tool.poetry.dependencies] (or the project's dependency
section) with an appropriate version constraint, so the direct import in
marker.py is guaranteed to resolve and not break if marker-pdf drops it.
- Around line 211-215: The health-check calls in ensure_worker_pool_healthy are
calling the actor methods directly and can block; change both calls to use
call_ray_actor_with_timeout so they follow the same timeout/cancellation path as
process_pdf (see process_pdf usage). Specifically, replace awaiting
worker.is_pool_broken.remote() and worker.setup_mp.remote() with calls wrapped
by call_ray_actor_with_timeout(...) passing the same timeout constant/handler
used by process_pdf so the actor calls time out and free the queue slot if the
actor is wedged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0de7cd3-3264-4dfa-a146-9cfbfbed8caf
📒 Files selected for processing (13)
benchmarks/marker/marker_page_chunking.mdbenchmarks/workspace/README.mdbenchmarks/workspace/docker-compose.ymlbenchmarks/workspace/requirements.txtbenchmarks/workspace/results_workspace.mdbenchmarks/workspace/workspace.pycharts/openrag-stack/values.yamlconf/config.yamldocs/content/docs/documentation/deploy_ray_cluster.mddocs/content/docs/documentation/env_vars.mdopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/config/loader.pyopenrag/config/models.py
💤 Files with no reviewable changes (2)
- docs/content/docs/documentation/deploy_ray_cluster.md
- charts/openrag-stack/values.yaml
✅ Files skipped from review due to trivial changes (1)
- benchmarks/marker/marker_page_chunking.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/config/loader.py
EnjoyBacon7
force-pushed
the
feat/marker_pdf_chunking
branch
from
April 9, 2026 07:47
e3a56d2 to
3aebdd1
Compare
Ahmath-Gadji
force-pushed
the
feat/marker_pdf_chunking
branch
from
April 9, 2026 07:57
3aebdd1 to
c53d4b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/indexer/loaders/pdf_loaders/marker.py`:
- Around line 193-197: The _get_page_count function currently opens a
PdfDocument with pypdfium2.PdfDocument(file_path) and calls pdf.close()
manually, which can leak the file handle if len(pdf) raises; change it to use a
context manager (with pypdfium2.PdfDocument(file_path) as pdf:) so the document
is always closed on error and simply return len(pdf) from inside the with block
to ensure proper resource cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39f328bb-ca4c-43d3-9431-c7f78e711fea
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
benchmarks/marker/marker_page_chunking.mdbenchmarks/workspace/README.mdbenchmarks/workspace/docker-compose.ymlbenchmarks/workspace/requirements.txtbenchmarks/workspace/results_workspace.mdbenchmarks/workspace/workspace.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pypyproject.toml
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- benchmarks/marker/marker_page_chunking.md
EnjoyBacon7
force-pushed
the
feat/marker_pdf_chunking
branch
from
April 9, 2026 08:07
c53d4b2 to
f91f196
Compare
EnjoyBacon7
force-pushed
the
feat/marker_pdf_chunking
branch
from
April 9, 2026 08:12
f91f196 to
6989e64
Compare
This was referenced May 5, 2026
This was referenced Jun 8, 2026
This was referenced Jun 25, 2026
Closed
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
1 participant
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
MARKER_CHUNK_SIZE, default 10) and dispatched concurrently across all available Marker workers. This reduces per-worker GPU memory spikes (~2.5 GB vs 4+ GB unchunked) and enables safely scalingMARKER_MAX_PROCESSESwithout OOM risk.get_current_pool_size) withis_pool_broken(), which checks the executor's broken state instead of counting live subprocesses. The previous approach falsely triggered pool resets when workers were simply busy.VDB_ENABLE_INSERTIONenv var: Allows disabling vector database insertion while still processing documents. Useful for benchmarking and testing.MARKER_MIN_PROCESSES: No longer needed after the health check rework. Cleaned up from config, code, docs, and Helm chart.Benchmark results
Summary by CodeRabbit
New Features
Documentation
Configuration