refactor(phase-5): core domain logic parsers - #354
Closed
Ahmath-Gadji wants to merge 13 commits into
Closed
Conversation
…ct conts ValidationError now accepts status_code/code overrides so callers can preserve 400/415 semantics instead of being forced to 422. decode_bytes adds UTF-8-first-then-chardet decoding (chardet alone misclassifies short ASCII-heavy UTF-8 as Latin-1). conts.py becomes the canonical home for PARTITION_PREFIX, IMAGE_PLACEHOLDER, and FILE_READ_CHUNK_SIZE; the legacy top-level consts.py will shim to it in a later commit.
ImageBlock gains a documented parser->caption contract (markdown_ref metadata key), a source_url field for HTTP image refs, and an image_url property that returns a data: URI when bytes are present or the source URL otherwise. Bytes default to b"" so HTTP-only blocks are constructible without dummy payloads. Document gains an async context manager `as_temporary_file()` that materializes raw_bytes to disk in a worker thread and unlinks on exit, removing the NamedTemporaryFile dance from every parser that wraps a sync library requiring a path.
…ntextualizer text_preprocessor re-exports decode_bytes from core/utils/text so parser callsites have a near import. image_preprocessor consolidates PIL mode normalization, PNG encoding, and markdown-image-ref helpers (extract_data_uri_image_blocks, HTTP_IMAGE_PATTERN) used across markdown, docx, pptx, and pdf parsers. validators centralizes file-id and file-format validation against the LLM ABC. ChunkContextualizer (moved out of components/indexer) takes an LLM port and emits one chunk description per call.
📝 WalkthroughWalkthroughRestructures indexing: moves parsers and image/text preprocessors into a framework-free core, introduces parser facades (client/pooled), implements loader→parser adapter shims for legacy discovery, adds Ray worker pools and service clients, extends Document/ImageBlock models, updates validators/exceptions, and adds tests and decision log entries for Phase 5D/5E. ChangesIndexing domain & Loader→Parser migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader as Legacy Loader Shim
participant Parser as Core Parser
participant Service as Worker/Client (Pool or External)
Client->>Loader: aload_document(file)
Loader->>Parser: build CoreDocument(raw_bytes, metadata)
alt Core-native parser
Loader->>Parser: parse(document)
Parser-->>Loader: ProcessedDocument (TextBlocks, ImageBlocks)
else Service-backed parser
Loader->>Service: submit(temp_path) with timeout/retry
Service-->>Loader: ProcessedDocument (markdown + images)
else Client-backed parser
Loader->>Service: upload & API call
Service-->>Loader: response (transcript/OCR)
Loader->>Parser: validate/normalize response
Parser-->>Loader: ProcessedDocument
end
Loader->>Client: LangChain Document (assembled from ProcessedDocument)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
EnjoyBacon7
force-pushed
the
refactor/phase-5-core-domain-logic-parsers
branch
from
May 5, 2026 09:44
62fbdc0 to
0d3ca51
Compare
…ocx, doc, pptx, eml, pymupdf) document_parser.py adds two empty type-marker subclasses (BasePooledParser, BaseClientParser) so consumers can constrain a parser by *how* it gets its work done — pool-backed vs client-backed — without coupling to a concrete services-side type. Native parsers (no Ray, no external clients): - text/html/markdown emit one TextBlock; image_preprocessor handles data-URI and HTTP image refs with the markdown_ref contract. - image emits a single ImageBlock with caption=None (caption stage fills). - docx (MarkItDown + python-docx fallback) and pptx (python-pptx) emit Markdown plus one ImageBlock per embedded image; docx zip-extracts embedded media and rewrites MarkItDown's truncated placeholders to unique synthetic refs. - doc converts to docx via spire.doc and delegates to DocxParser. - eml parses MIME parts: text via decode_bytes, attachments dispatched to a sub-parser map, image attachments emit ImageBlocks. - pdf/pymupdf is the lightweight CPU PDF backend; markdown mode uses pymupdf4llm with embed_images=True to surface embedded images as ImageBlocks. PPTX and PyMuPDF emit one TextBlock per slide/page. The pdf/ subpackage exports only PyMuPDFParser at this point; the Marker (Ray-pooled) and OpenAI-VLM (client-backed) facades land in the next commits.
EnjoyBacon7
force-pushed
the
refactor/phase-5-core-domain-logic-parsers
branch
from
May 5, 2026 09:53
0d3ca51 to
7bcbebd
Compare
…rence impl) Core OpenAIPdfParser is a thin facade taking an injected BaseClientParser; the real work is in services/inference/parsers/. BaseOpenAIPdfClient wraps an OpenAI-compatible Chat Completions client, renders each page to a PIL image, and asks the VLM for layout-preserving Markdown plus image captions in a single call (captioning is intrinsic to VLM-PDF parsers — they don't go through the generic parser->caption stage). DotsOCRPdfClient subclasses with model-specific prompt and parameter tweaks. The Docling and DoclingV2 backends are deferred — see decision log.
Core MarkerParser and LocalWhisperParser are thin facades each taking an injected BasePooledParser; the heavy lifting is in services/workers/. services/workers/ray_utils centralizes Ray-actor concurrency: - call_ray_actor_with_timeout / @with_timeout: await an ObjectRef with proper cancel-on-timeout, asyncio cancellation, and TaskCancelledError / RayTaskError translation. - retry_with_backoff / @with_retry: exponential backoff + jitter, CancelledError never retried. - Decorator descriptions accept str.format-style templates substituted with the wrapped call's bound arguments. MarkerWorker (per-actor) wraps the Marker PdfConverter inside a ProcessPoolExecutor so pdftext can spawn its own children. MarkerPool distributes across actors. MarkerLoader splits the post-processed markdown into one TextBlock per page (no in-band [PAGE_N] markers — see decision log #20) and emits one ImageBlock per Marker image with the markdown_ref carried in metadata. WhisperWorker / WhisperPool / WhisperLoader follows the same shape for audio and video, with retry-with-backoff around per-chunk transcription. All parsers in this commit materialize Document.raw_bytes via Document.as_temporary_file() before handing a path to the underlying sync library.
Utilities whose canonical home now lives under core/ or services/ get
thin re-export shims at their old import paths so existing callers
don't break:
- components/ray_utils.py -> services.workers.ray_utils
- components/indexer/utils/text_sanitizer.py -> core.utils.text
- consts.py -> core.utils.conts
- utils/exceptions/{base,embeddings,vectordb,__init__}.py
-> core.utils.exceptions
- routers/utils.py validate_file_id / validate_metadata /
validate_file_format now delegate to core.indexing.validators
This is a partial pass — the loader->parser shims wait until phases
5A/5B/5C land their consumers (chunker, retriever, prompts).
Records the decisions made during the parser-layer rebuild that aren't derivable from the code: - ray_utils split + decorator forms (#6-13) - ray_utils canonical home moves to services/workers/ (#14) - captioning split: stripped from generic parsers, kept inside VLM-PDF parsers (#17) - ImageBlock.metadata['markdown_ref'] as the parser->caption contract (#18) - ImageBlock.source_url + image_url property for HTTP image refs (#19) - Docling/DoclingV2 deferred (#16) - Paginated parsers emit list[TextBlock] with page_number; in-band [PAGE_N] markers are gone — chunker must read TextBlock.page_number, not scan the text (#20)
EnjoyBacon7
force-pushed
the
refactor/phase-5-core-domain-logic-parsers
branch
from
May 6, 2026 07:31
7bcbebd to
75551aa
Compare
Ahmath-Gadji
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
REFACTORING_DECISION_LOG.md (1)
75-353: ⚡ Quick winPhase 5D decision numbering is inconsistent.
The Phase 5D entries skip
#9and#15, and#16is placed out of order between#20and#21. Current sequence:1…8, 10, 11…14, 17…20, 16, 21…24. This makes future cross-references brittle (Phase 1 already cross-references decision numbers, e.g., the "see Phase 1 decision#1follow-up" pointer in#3) and confuses readers who expect monotonic numbering.Either renumber to be contiguous (
1…22) or re-order#16next to the other parser-scope decisions (#6,#7) and document the gaps if any decisions were intentionally dropped during drafting.🤖 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 `@REFACTORING_DECISION_LOG.md` around lines 75 - 353, The Phase 5D decision list numbering is non-sequential (current sequence 1…8, 10, 11…14, 17…20, 16, 21…24) which breaks cross-references; update the "Phase 5D — Indexing domain logic + parsers (2026-04-30)" section in REFACTORING_DECISION_LOG.md to use contiguous, monotonic numbering (e.g., 1…22) or move the misordered entry labeled "16. Docling and DoclingV2 PDF backends deferred" next to related parser items (near items 6–8) and then renumber all subsequent entries so every decision number is unique and sequential; ensure all internal references (like "see Phase 1 decision `#1` follow-up") are updated to the new numbers.openrag/core/indexing/parsers/pdf/marker.py (1)
23-28: 💤 Low valueDrop the redundant
pool is Noneclause and the unfinished dev comment.
isinstance(None, BasePooledParser)isFalse, sonot isinstance(pool, BasePooledParser)already covers theNonecase — theor pool is Nonebranch is unreachable. The inline comment also reads as a half-finished note. Aligning with the cleaner pattern used byClientAudioParser.__init__(audio/client_based.py L22-23) keeps the validation consistent across facades.♻️ Proposed cleanup
def __init__(self, pool: BasePooledParser) -> None: - # check pool is a BasePooledParser? and not empty - if not isinstance(pool, BasePooledParser) or pool is None: + if not isinstance(pool, BasePooledParser): raise ValueError("MarkerParser requires a BasePooledParser instance as pool") - self._pool = pool🤖 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 `@openrag/core/indexing/parsers/pdf/marker.py` around lines 23 - 28, Remove the unfinished dev comment and the redundant "or pool is None" check in MarkerParser.__init__; simply validate the pool with isinstance(pool, BasePooledParser) and raise the same ValueError if it fails (mirroring the pattern used in ClientAudioParser.__init__), then assign self._pool = pool.
🤖 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 `@openrag/core/indexing/contextualize.py`:
- Around line 44-56: The constructor currently stores a semaphore but
contextualize still creates one asyncio.Task per chunk up-front causing
unbounded memory usage; change contextualize to create at most
`_semaphore`-bounded concurrent LLM.chat invocations by acquiring `_semaphore`
before scheduling each chat and releasing it when that chat completes (or
implement a fixed pool of worker coroutines that pull chunk prompts from an
async iterator/queue), and ensure chunk prompt strings are produced lazily so
they aren’t all retained in memory; adjust code around the contextualize
function (and related code at the noted block 111-123) to use
`_semaphore.acquire()`/release or a worker queue pattern instead of creating all
tasks at once while still calling `LLM.chat()` with `_system_prompt`.
In `@openrag/core/indexing/parsers/image_parser.py`:
- Around line 46-55: The parse method of ImageParser calls the CPU-bound helpers
_normalize_to_png(document) and _below_min_pixels(png_bytes) directly on the
event loop; change both invocations to run in a thread by awaiting
asyncio.to_thread(...) so the heavy work is offloaded (e.g., png_bytes = await
asyncio.to_thread(self._normalize_to_png, document) and below = await
asyncio.to_thread(self._below_min_pixels, png_bytes)), preserving the existing
error/return handling and logger messages and keeping the rest of parse
unchanged.
In `@openrag/core/indexing/parsers/pdf/__init__.py`:
- Around line 8-12: The module eagerly imports ClientPdfParser, MarkerParser,
and PyMuPDFParser which contradicts the docstring's promise of lazy-loading and
causes import failures when optional backend deps are missing; change to lazy
exports by removing the top-level imports and implementing module-level
__getattr__ (and optionally __dir__) to import and return ClientPdfParser,
MarkerParser, and PyMuPDFParser on first access, keep __all__ listing those
names so tools see the public API, and ensure ImportError is raised only when
the specific backend is accessed.
In `@openrag/core/indexing/parsers/pptx_parser.py`:
- Around line 55-56: The page_count is being derived from the filtered slides
list (slides) so trailing empty slides are lost; update the parser to compute
and return the true slide count from the original data before filtering: capture
the total slide count produced by _convert (e.g., save len(raw_slides) or
total_slides prior to dropping empty markdown) and set page_count to that value
instead of slides[-1][0]; ensure TextBlock creation still uses the filtered
slides but page_count uses the preserved total (also apply the same change where
page_count is computed in the later block around the 123-126 logic).
In `@openrag/core/indexing/text_preprocessor.py`:
- Line 9: Replace the cross-package relative import in text_preprocessor.py with
an absolute import from the project root: change the "from ..utils.text import
clean_markdown_table_spacing, decode_bytes, sanitize_extracted_text,
sanitize_text" to use the absolute module path (e.g., import from
openrag.utils.text) so the symbols clean_markdown_table_spacing, decode_bytes,
sanitize_extracted_text, and sanitize_text are imported via the absolute package
name to comply with repository import rules.
In `@openrag/core/models/document.py`:
- Around line 152-182: The as_temporary_file context manager in Document keeps a
NamedTemporaryFile open across the yield which breaks on Windows; change the
_open/create logic in Document.as_temporary_file to create the temp file with
delete=False, write and close it before yielding (so the file can be reopened by
sync callers), and in the finally block explicitly remove the file via
os.unlink(Path(tf.name)) (perform both close/unlink on a thread via
asyncio.to_thread if needed); adjust references to tf/_open in that method
accordingly and add the required import for os.
In `@openrag/core/utils/text.py`:
- Around line 23-25: The decode_bytes function currently calls
raw.decode(encoding, errors="replace") without handling invalid codec names,
which can raise LookupError; update decode_bytes (the branch that checks if
encoding is truthy) to catch LookupError around the raw.decode call and, on
LookupError, fall through to the existing fallback decoding logic (i.e., perform
the same fallback attempts used when encoding is falsy) so invalid encoding
names do not crash the function.
In `@openrag/services/inference/parsers/dotsocr.py`:
- Around line 23-26: The file uses a relative/non-root import "from
core.models.document import Document, ImageBlock, ProcessedDocument, TextBlock"
which violates the package root import rule; update that line to import from the
package root (use "from openrag.core.models.document import Document,
ImageBlock, ProcessedDocument, TextBlock") so the module resolves when only
openrag is on PYTHONPATH, and apply the same change to the other parser files
mentioned (openai_audio.py and _base_openai_parser.py) where
core.models.document is imported.
In `@openrag/services/inference/parsers/openai_audio.py`:
- Around line 41-43: The _DEFAULT_DIRECT_UPLOAD_SUFFIXES tuple is missing
".wav", causing .wav files to be needlessly re-encoded and leading to wav_path
== input_path in _prepare_upload so sound.export can overwrite the original temp
file; add ".wav" to _DEFAULT_DIRECT_UPLOAD_SUFFIXES and/or in _prepare_upload
guard the wav_path == input_path case by writing the exported WAV to a sibling
temporary filename (or use tempfile.NamedTemporaryFile) before calling
sound.export and ensure unlink/as_temporary_file cleanup operates on that
separate temp file (update references to wav_path/input_path and
sound.export/as_temporary_file accordingly).
In `@openrag/services/workers/parsers/marker_workers.py`:
- Around line 421-454: The splitter currently drops empty page segments; in
_split_pages change the logic so empty pages are preserved: use
cls._PAGE_MARKER_RE and, instead of "if text: pairs.append((page, text))",
always append pairs.append((page, text)) so pages with empty text are kept (this
preserves page numbering and page_count); keep the existing tail handling but
only append the tail as (last_page + 1, tail) when tail is non-empty (leave
PAGE_SEP handling and _PAGE_MARKER_RE usage unchanged).
In `@openrag/services/workers/parsers/whisper_workers.py`:
- Line 17: The parse() method currently awaits
self.whisper_actor.transcribe.remote() directly (and elsewhere around the
135-140 region), which bypasses Ray's timeout/cancellation handling; replace
these direct remote awaits with calls to
call_ray_actor_with_timeout(self.whisper_actor, "transcribe", args...) (or the
existing helper call_ray_actor_with_timeout function) so the remote task is
invoked via the timeout/cancellation wrapper and will be cancelled if parse() is
cancelled or the temp file context exits; update both the primary transcribe
invocation in parse() and the additional transcribe calls at lines ~135-140 to
use call_ray_actor_with_timeout instead of awaiting transcribe.remote()
directly.
In `@openrag/services/workers/ray_utils.py`:
- Around line 159-172: The retry_with_backoff() function is rethrowing
asyncio.CancelledError but still retries ray.exceptions.TaskCancelledError;
update its exception handling to also catch and immediately re-raise
TaskCancelledError (ray.exceptions.TaskCancelledError) so it doesn't fall into
the generic Exception retry block—import or fully qualify TaskCancelledError,
add a dedicated except ray.exceptions.TaskCancelledError: raise (parallel to the
asyncio.CancelledError handler), and ensure the generic Exception handler
continues to handle other errors and logging/retry logic as before.
---
Nitpick comments:
In `@openrag/core/indexing/parsers/pdf/marker.py`:
- Around line 23-28: Remove the unfinished dev comment and the redundant "or
pool is None" check in MarkerParser.__init__; simply validate the pool with
isinstance(pool, BasePooledParser) and raise the same ValueError if it fails
(mirroring the pattern used in ClientAudioParser.__init__), then assign
self._pool = pool.
In `@REFACTORING_DECISION_LOG.md`:
- Around line 75-353: The Phase 5D decision list numbering is non-sequential
(current sequence 1…8, 10, 11…14, 17…20, 16, 21…24) which breaks
cross-references; update the "Phase 5D — Indexing domain logic + parsers
(2026-04-30)" section in REFACTORING_DECISION_LOG.md to use contiguous,
monotonic numbering (e.g., 1…22) or move the misordered entry labeled "16.
Docling and DoclingV2 PDF backends deferred" next to related parser items (near
items 6–8) and then renumber all subsequent entries so every decision number is
unique and sequential; ensure all internal references (like "see Phase 1
decision `#1` follow-up") are updated to the new numbers.
🪄 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: 04dce63c-f256-4f2d-b48a-365e83aa8dc6
📒 Files selected for processing (41)
REFACTORING_DECISION_LOG.mdopenrag/components/indexer/utils/text_sanitizer.pyopenrag/components/ray_utils.pyopenrag/consts.pyopenrag/core/indexing/contextualize.pyopenrag/core/indexing/image_preprocessor.pyopenrag/core/indexing/parsers/audio/__init__.pyopenrag/core/indexing/parsers/audio/client_based.pyopenrag/core/indexing/parsers/audio/local_whisper.pyopenrag/core/indexing/parsers/doc_parser.pyopenrag/core/indexing/parsers/document_parser.pyopenrag/core/indexing/parsers/docx_parser.pyopenrag/core/indexing/parsers/eml_parser.pyopenrag/core/indexing/parsers/html_parser.pyopenrag/core/indexing/parsers/image_parser.pyopenrag/core/indexing/parsers/markdown_parser.pyopenrag/core/indexing/parsers/pdf/__init__.pyopenrag/core/indexing/parsers/pdf/client_based.pyopenrag/core/indexing/parsers/pdf/marker.pyopenrag/core/indexing/parsers/pdf/pymupdf.pyopenrag/core/indexing/parsers/pptx_parser.pyopenrag/core/indexing/parsers/text_parser.pyopenrag/core/indexing/text_preprocessor.pyopenrag/core/indexing/validators.pyopenrag/core/models/document.pyopenrag/core/utils/conts.pyopenrag/core/utils/exceptions.pyopenrag/core/utils/text.pyopenrag/routers/utils.pyopenrag/services/inference/parsers/__init__.pyopenrag/services/inference/parsers/_base_openai_parser.pyopenrag/services/inference/parsers/dotsocr.pyopenrag/services/inference/parsers/openai_audio.pyopenrag/services/workers/parsers/__init__.pyopenrag/services/workers/parsers/marker_workers.pyopenrag/services/workers/parsers/whisper_workers.pyopenrag/services/workers/ray_utils.pyopenrag/utils/exceptions/__init__.pyopenrag/utils/exceptions/base.pyopenrag/utils/exceptions/embeddings.pyopenrag/utils/exceptions/vectordb.py
Comment on lines
+23
to
+26
| from core.models.document import Document, ImageBlock, ProcessedDocument, TextBlock | ||
| from pydantic import BaseModel, RootModel, ValidationError | ||
|
|
||
| from ._base_openai_parser import BaseOpenAIPdfClient |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the imports around lines 23-26
head -30 openrag/services/inference/parsers/dotsocr.pyRepository: linagora/openrag
Length of output: 968
🏁 Script executed:
# Search for where the Document, ImageBlock, ProcessedDocument, TextBlock classes are actually located
fd -t f "document.py" | head -20Repository: linagora/openrag
Length of output: 92
🏁 Script executed:
# Check how other parser files import from document models
rg "from.*document import" openrag/services/inference/parsers/ -A 0Repository: linagora/openrag
Length of output: 431
🏁 Script executed:
# Check if 'core' directory exists at root level or under openrag
fd -t d "^core$" openrag/ | head -10Repository: linagora/openrag
Length of output: 74
🏁 Script executed:
# Verify the actual import structure by checking what's in the openrag directory
ls -la openrag/ | grep -E "^d"Repository: linagora/openrag
Length of output: 860
Use absolute imports from the openrag package root.
The import from core.models.document ... violates the coding guideline for openrag/**/*.py files, which requires absolute imports from the openrag/ root. This pattern breaks in execution contexts where only the openrag package is available on PYTHONPATH. The correct import is from openrag.core.models.document ....
🔧 Import fix
-from core.models.document import Document, ImageBlock, ProcessedDocument, TextBlock
+from openrag.core.models.document import Document, ImageBlock, ProcessedDocument, TextBlockNote: This same import issue exists in other parser files (openai_audio.py, _base_openai_parser.py) and should be corrected consistently.
🤖 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 `@openrag/services/inference/parsers/dotsocr.py` around lines 23 - 26, The file
uses a relative/non-root import "from core.models.document import Document,
ImageBlock, ProcessedDocument, TextBlock" which violates the package root import
rule; update that line to import from the package root (use "from
openrag.core.models.document import Document, ImageBlock, ProcessedDocument,
TextBlock") so the module resolves when only openrag is on PYTHONPATH, and apply
the same change to the other parser files mentioned (openai_audio.py and
_base_openai_parser.py) where core.models.document is imported.
6 tasks
…parsers Adapted from the legacy components/indexer/loaders test suite to the new core.indexing.parsers / services.inference.parsers split. - test_image_preprocessor: PIL mode normalization, PNG encoding, data-URI decoding, ImageBlock extraction - test_validators: file_id / metadata / file format checks - test_docx_parser: zip image extraction (mirrors legacy TestGetImagesFromZip) + the new parser->caption ref-injection contract - test_doc_parser: spire.doc shimmed via sys.modules; success path, GetText fallback, total failure, missing-spire, empty bytes - test_openai_audio: pydub shimmed for Python 3.13 compat; AsyncOpenAI swapped out post-construction so no network is touched; covers _prepare_upload, parse() happy/empty/error paths, language detector forwarding + failure swallow
…r_registry Wires every concrete DocumentParser into parser_registry so config-driven factories can resolve them by name. Names are backend-flavored (pymupdf, marker, pdf_client, local_whisper, audio_client) so multiple impls of the same content type can coexist.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
openrag/core/indexing/parsers/pptx_parser.py (2)
57-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
page_countunderreports decks ending in empty slides.Because
_convert()drops slides with empty markdown,page_count = slides[-1][0]reflects the last non-empty slide rather than the true slide count. This breaks page-based consumers when a deck ends with blank slides. Return the total slide count from_convertand use it directly here.🤖 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 `@openrag/core/indexing/parsers/pptx_parser.py` around lines 57 - 58, The page_count is computed from the last non-empty entry in slides so decks ending with blank slides underreport; update the converter to return the true total slide count from _convert (e.g., return both slides and total_slide_count), then in pptx_parser use that returned total to set page_count instead of slides[-1][0]; adjust any call sites of _convert to accept the new return value and ensure TextBlock generation still uses the filtered slides list while page_count uses the returned total.
69-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
_convertreturns""forslideson failure, violating its declaredlist[tuple[int, str]]return type.Lines 75 and 81 return
("", [])but the signature istuple[list[tuple[int, str]], list[ImageBlock]]. It currently happens to work because the empty string is iterable and falsy, but it's a typing lie that will surface as a real bug the moment a caller does anything beyond the two operations on lines 57–58 (e.g.len(slides), slicing,slides[0][0]). Return[], []for consistency with the success path.🔧 Proposed fix
try: import pptx from PIL import Image except ImportError: logger.warning("python-pptx or PIL not available; cannot parse PPTX") - return "", [] + return [], [] try: presentation = pptx.Presentation(path) except Exception as exc: logger.warning("Failed to open PPTX: %s", exc) - return "", [] + return [], []🤖 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 `@openrag/core/indexing/parsers/pptx_parser.py` around lines 69 - 82, The _convert function currently returns ("", []) on failure which violates its declared return type tuple[list[tuple[int, str]], list[ImageBlock]]; update the two failure return sites in _convert (the ImportError branch and the presentation-open exception branch) to return ([], []) instead of ("", []) so callers always receive a list for slides and preserve type consistency with the success path.
🧹 Nitpick comments (8)
openrag/core/indexing/parsers/pptx_parser.py (2)
191-196: 💤 Low valueDead branch in
_chart_to_markdownValueErrorhandler.Both arms of the
if "unsupported plot type" in str(exc):check return the same"\n\n[unsupported chart]\n\n"string, so the conditional is unreachable-by-effect. Either drop theifentirely or differentiate the messages if the distinction is intended for logging.🔧 Proposed fix
- except ValueError as exc: - if "unsupported plot type" in str(exc): - return "\n\n[unsupported chart]\n\n" - return "\n\n[unsupported chart]\n\n" - except Exception: + except Exception: return "\n\n[unsupported chart]\n\n"🤖 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 `@openrag/core/indexing/parsers/pptx_parser.py` around lines 191 - 196, The ValueError handler in _chart_to_markdown contains a redundant conditional—both the if branch checking "unsupported plot type" in str(exc) and the else return the same "\n\n[unsupported chart]\n\n"; remove the unnecessary conditional and simplify the handler to a single return, or if you intended different behavior, change one branch to a distinct message or add logging that includes the exception (reference: _chart_to_markdown handling of ValueError).
86-117: 💤 Low valueGuard
titleagainstNoneand prefer identity comparison for the title shape.
slide.shapes.titlereturnsNonewhen a slide has no title placeholder.shape == titlethen compares each shape againstNonevia python-pptx's__eq__, which is at best wasteful and at worst surprising. Usetitle is not None and shape is titleto make the intent (and the guard) explicit.🔧 Proposed fix
elif getattr(shape, "has_text_frame", False): - if shape == title: + if title is not None and shape is title: md += "# " + shape.text.lstrip() + "\n" else: md += shape.text + "\n"🤖 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 `@openrag/core/indexing/parsers/pptx_parser.py` around lines 86 - 117, Guard against slide title being None and use identity comparison: when iterating slides in the loop that reads slide.shapes.title, change the check from `shape == title` to `title is not None and shape is title` (or equivalent) so you first ensure `title` exists and then compare by identity; update the branch that appends heading text (the block that currently does `if shape == title: md += "# " + shape.text.lstrip() + "\n"`) to use this guarded identity check to avoid relying on python-pptx’s __eq__ behavior.openrag/core/indexing/parsers/test_docx_parser.py (1)
24-30: 💤 Low valueTemp files are never cleaned up.
_fake_docxand the in-testNamedTemporaryFile(..., delete=False)calls (lines 26, 74, 81) leave.docxfiles behind in the system temp dir for every test run. Use the pytesttmp_pathfixture, which is auto-cleaned, to keep CI tidy.♻️ Suggested change
-def _fake_docx(media_files: dict[str, bytes]) -> Path: +def _fake_docx(tmp_path: Path, media_files: dict[str, bytes]) -> Path: """Build a minimal .docx zip with given ``word/media/<name>`` entries.""" - tmp = tempfile.NamedTemporaryFile(suffix=".docx", delete=False) - with zipfile.ZipFile(tmp, "w") as zf: + out = tmp_path / "fake.docx" + with zipfile.ZipFile(out, "w") as zf: for name, data in media_files.items(): zf.writestr(f"word/media/{name}", data) - return Path(tmp.name) + return outThe corresponding test methods would then accept
tmp_pathand pass it through (and the bareNamedTemporaryFilecalls intest_no_media_returns_empty/test_invalid_zip_returns_emptyshould also be migrated).🤖 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 `@openrag/core/indexing/parsers/test_docx_parser.py` around lines 24 - 30, The tests create temporary .docx files with _fake_docx and direct tempfile.NamedTemporaryFile(delete=False) calls which leak files; update _fake_docx to accept a pytest tmp_path (or Path) and write the zip there instead of using NamedTemporaryFile, and change the tests test_no_media_returns_empty and test_invalid_zip_returns_empty (and other callers) to accept and pass tmp_path into _fake_docx so pytest will auto-clean the files; ensure you replace delete=False usages and return a Path within tmp_path.openrag/core/indexing/parsers/docx_parser.py (2)
36-39: ⚖️ Poor tradeoffPrefer absolute imports across packages.
from ...models.document import …and similar deep relative imports cross package boundaries (parsers→core→models). Absolute imports rooted atopenrag/make refactors and IDE navigation more predictable.♻️ Suggested change
-from ...models.document import Document, DocumentType, ImageBlock, ProcessedDocument, TextBlock -from ..image_preprocessor import ensure_png_compatible_mode, pil_to_png_bytes -from .document_parser import DocumentParser -from .registry import parser_registry +from openrag.core.models.document import ( + Document, + DocumentType, + ImageBlock, + ProcessedDocument, + TextBlock, +) +from openrag.core.indexing.image_preprocessor import ( + ensure_png_compatible_mode, + pil_to_png_bytes, +) +from openrag.core.indexing.parsers.document_parser import DocumentParser +from openrag.core.indexing.parsers.registry import parser_registryThe same pattern applies to the other new parser modules in this PR.
As per coding guidelines: "Use absolute imports from the
openrag/root directory instead of relative imports across packages; relative imports are only acceptable within the same package".🤖 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 `@openrag/core/indexing/parsers/docx_parser.py` around lines 36 - 39, The file uses deep relative imports (e.g., "from ...models.document import Document, DocumentType, ImageBlock, ProcessedDocument, TextBlock", "from ..image_preprocessor import ensure_png_compatible_mode, pil_to_png_bytes", "from .document_parser import DocumentParser", "from .registry import parser_registry"); replace these with absolute imports rooted at the openrag package (e.g., import Document, DocumentType, ImageBlock, ProcessedDocument, TextBlock from openrag.models.document; ensure_png_compatible_mode and pil_to_png_bytes from openrag.core.indexing.image_preprocessor or the correct absolute module; DocumentParser and parser_registry from their absolute openrag paths) so IDEs and refactors resolve symbols consistently across packages.
119-159: 💤 Low valueEdge case: gaps in DOCX media numbering produce mis-aligned
Noneslots.
_extract_embedded_imagesreturns a list of lengthmax_order, where missing indices in the zip becomeNone. Office typically writesimage1.{ext}…imageN.{ext}contiguously, so this works in practice — but if a document has non-contiguous numbering (e.g.,image1.png,image3.pngafter edits), the syntheticNoneat index 2 will collapse the corresponding placeholder and shift all subsequent images by one, even though MarkItDown only emits two placeholders.Not blocking — consider compacting the result to only present indices, then sorting by
order_num:♻️ Optional compact form
- if not ordered: - return [] - max_order = max(ordered) - return [ordered.get(i + 1) for i in range(max_order)] + return [ordered[k] for k in sorted(ordered)]🤖 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 `@openrag/core/indexing/parsers/docx_parser.py` around lines 119 - 159, _extract_embedded_images currently builds an "ordered" dict and returns a list of length max_order filling gaps with None, which misaligns placeholders when DOCX image numbering is non-contiguous; change the logic to collect (order_num, bytes_or_None) pairs as you read media, then sort those pairs by order_num and return a compact list of just the values (preserving None for unsupported formats but only for images actually found) instead of generating a list up to max_order; update references inside the function (media loop, ordered -> list of tuples, and the final return) to implement this sorted-compact behavior while keeping existing exception handling and PIL conversion steps.openrag/core/indexing/parsers/audio/client_based.py (1)
28-29: ⚡ Quick winInconsistent
supported_types()between client-backed facades.
ClientPdfParser.supported_types()delegates to the wrapped client (return self._client.supported_types()), butClientAudioParserhardcodes[AUDIO, VIDEO]. If the injectedBaseClientParseronly supports one of the two (e.g., a Whisper client that doesn't transcribe video, or a video-only ASR client), this facade will lie about its capabilities and break parser routing.Consider delegating to the client for symmetry with
ClientPdfParserand to keep the facade truthful:♻️ Suggested change
def supported_types(self) -> list[str]: - return [DocumentType.AUDIO.value, DocumentType.VIDEO.value] + return self._client.supported_types()If the intent is that all audio clients must support both types, encode it as a postcondition check in
__init__rather than masking the client's actual contract.🤖 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 `@openrag/core/indexing/parsers/audio/client_based.py` around lines 28 - 29, ClientAudioParser.supported_types currently returns a hardcoded [DocumentType.AUDIO, DocumentType.VIDEO] which can lie about the wrapped client's capabilities; change ClientAudioParser.supported_types to delegate to the injected client's supported_types (return self._client.supported_types()) to match ClientPdfParser, or alternatively enforce the dual-type requirement as a postcondition in ClientAudioParser.__init__ by checking that all required types are present in self._client.supported_types() and raising an assertion/error if not.openrag/core/indexing/parsers/doc_parser.py (1)
27-27: 💤 Low valueModule-import-time
os.environmutation is a global side effect.Setting
DOTNET_SYSTEM_GLOBALIZATION_INVARIANTat module-import time means any import ofopenrag.core.indexing.parsers.doc_parser— including indirect imports through the parser registry, autodoc, IDE indexers, or test collection — mutates the process's env, even when no.docfile is ever parsed. Whilesetdefaultmakes it idempotent and non-clobbering, it still couples a domain module to a runtime DOTNET concern.Prefer scoping it to where Spire is actually invoked, or to the application startup/composition root:
♻️ Suggested change
-os.environ.setdefault("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1") - - `@parser_registry.register`("doc") class DocParser(DocumentParser): ... `@staticmethod` def _convert(path: str) -> tuple[bytes | None, str | None]: ... try: + os.environ.setdefault("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1") from spire.doc import Document as SpireDocument from spire.doc import FileFormat except ImportError:🤖 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 `@openrag/core/indexing/parsers/doc_parser.py` at line 27, Remove the module-level os.environ.setdefault("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1") side-effect from openrag.core.indexing.parsers.doc_parser.py and instead set this environment variable only at the point where the .doc/.Spire processing actually runs (for example inside the function/method that invokes Spire—e.g., the DocParser.parse method or the helper that constructs/calls the Spire client). Ensure the change is idempotent (only set if not present) and limited in scope to the startup of Spire use so importing the module no longer mutates process environment globally.openrag/core/indexing/parsers/pdf/pymupdf.py (1)
20-26: 💤 Low valueModule-level hard imports of optional backend libraries.
pymupdfandpymupdf4llmare imported at module top-level (lines 20–21), so importing this module will raiseImportErrorin environments without these libraries. The module's docstring declares the design intent: "Each backend lives in its own module so its heavy dependencies... are only imported when the backend is actually instantiated," yetpymupdf.pyviolates this by importing at the module level, whilemarker.pyandclient_based.py(in the same directory) correctly avoid hard imports.Defer imports into the functions where they are used:
♻️ Suggested change
-import pymupdf -import pymupdf4llm - from ....models.document import Document, DocumentType, ImageBlock, ProcessedDocument, TextBlock from ...image_preprocessor import extract_data_uri_image_blocks from ..document_parser import DocumentParser from ..registry import parser_registry ParseMode = Literal["markdown", "text"] def _extract_text(raw: bytes) -> tuple[list[str], list[ImageBlock]]: """Return one stripped plain-text string per page; no images.""" + import pymupdf with pymupdf.open(stream=raw, filetype="pdf") as doc: return [page.get_text().strip() for page in doc], [] def _extract_markdown(raw: bytes) -> tuple[list[str], list[ImageBlock]]: ... + import pymupdf + import pymupdf4llm with pymupdf.open(stream=raw, filetype="pdf") as doc:🤖 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 `@openrag/core/indexing/parsers/pdf/pymupdf.py` around lines 20 - 26, The module currently performs hard imports of optional backends (pymupdf and pymupdf4llm) at top-level; move those imports into the specific functions or methods that actually use them (e.g., inside the PDF parser class methods that call pymupdf/pymupdf4llm) and wrap them in a try/except ImportError to raise a clear, contextual error when the backend is instantiated; this ensures import-time of openrag.core.indexing.parsers.pdf.pymupdf does not fail in environments without those optional libraries while still providing a helpful message if the backend is chosen at runtime.
🤖 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 `@openrag/core/indexing/parsers/audio/local_whisper.py`:
- Around line 16-18: The import statements at the top of local_whisper.py use
relative imports across packages; replace them with absolute imports from the
package root so they follow repository rules, e.g., import Document,
DocumentType, ProcessedDocument, BasePooledParser, DocumentParser and
parser_registry using their full module paths (referencing the symbols Document,
DocumentType, ProcessedDocument, BasePooledParser, DocumentParser,
parser_registry) instead of the relative forms so the module imports resolve
from the openrag root.
In `@openrag/core/indexing/parsers/html_parser.py`:
- Around line 13-14: Replace the cross-package relative imports in
html_parser.py with absolute imports from the openrag root: change the import of
Document, DocumentType, ProcessedDocument, TextBlock (currently from
...models.document) to import them via their absolute module path, and import
decode_bytes (currently from ..text_preprocessor) via its absolute module path;
update the import statements where Document, DocumentType, ProcessedDocument,
TextBlock and decode_bytes are referenced so they use the new absolute names
consistently.
- Around line 54-58: The relative imports in html_parser.py cross package
boundaries and must be changed to absolute imports: replace the relative import
that brings in Document, DocumentType, ProcessedDocument, TextBlock with an
absolute import of openrag.models.document and replace the relative import of
decode_bytes with an absolute import from openrag.core.text_preprocessor; leave
the existing _to_markdown(html) implementation (which correctly uses
html_to_markdown.convert) unchanged.
In `@openrag/core/indexing/parsers/test_doc_parser.py`:
- Around line 109-113: Update test_missing_spire_returns_empty to force the
import failure for the Spire branch by inserting None entries into sys.modules
for the Spire import names before calling DocParser().parse; e.g., use the
pytest monkeypatch fixture to set sys.modules['spire']=None and
sys.modules['spire.doc']=None (or otherwise set and later restore) so that the
import inside DocParser._convert raises ImportError deterministically, then call
await DocParser().parse(_doc_document()) and assert the same empty results.
In `@openrag/core/indexing/parsers/text_parser.py`:
- Around line 15-16: Replace the cross-package relative imports with absolute
imports from the project root: import Document, DocumentType, ProcessedDocument,
TextBlock from openrag.models.document (replace "from ...models.document import
...") and import decode_bytes from openrag.core.indexing.text_preprocessor
(replace "from ..text_preprocessor import decode_bytes"); update the import
lines in text_parser.py accordingly so they use these absolute module paths and
preserve the same symbol names.
---
Duplicate comments:
In `@openrag/core/indexing/parsers/pptx_parser.py`:
- Around line 57-58: The page_count is computed from the last non-empty entry in
slides so decks ending with blank slides underreport; update the converter to
return the true total slide count from _convert (e.g., return both slides and
total_slide_count), then in pptx_parser use that returned total to set
page_count instead of slides[-1][0]; adjust any call sites of _convert to accept
the new return value and ensure TextBlock generation still uses the filtered
slides list while page_count uses the returned total.
- Around line 69-82: The _convert function currently returns ("", []) on failure
which violates its declared return type tuple[list[tuple[int, str]],
list[ImageBlock]]; update the two failure return sites in _convert (the
ImportError branch and the presentation-open exception branch) to return ([],
[]) instead of ("", []) so callers always receive a list for slides and preserve
type consistency with the success path.
---
Nitpick comments:
In `@openrag/core/indexing/parsers/audio/client_based.py`:
- Around line 28-29: ClientAudioParser.supported_types currently returns a
hardcoded [DocumentType.AUDIO, DocumentType.VIDEO] which can lie about the
wrapped client's capabilities; change ClientAudioParser.supported_types to
delegate to the injected client's supported_types (return
self._client.supported_types()) to match ClientPdfParser, or alternatively
enforce the dual-type requirement as a postcondition in
ClientAudioParser.__init__ by checking that all required types are present in
self._client.supported_types() and raising an assertion/error if not.
In `@openrag/core/indexing/parsers/doc_parser.py`:
- Line 27: Remove the module-level
os.environ.setdefault("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1") side-effect
from openrag.core.indexing.parsers.doc_parser.py and instead set this
environment variable only at the point where the .doc/.Spire processing actually
runs (for example inside the function/method that invokes Spire—e.g., the
DocParser.parse method or the helper that constructs/calls the Spire client).
Ensure the change is idempotent (only set if not present) and limited in scope
to the startup of Spire use so importing the module no longer mutates process
environment globally.
In `@openrag/core/indexing/parsers/docx_parser.py`:
- Around line 36-39: The file uses deep relative imports (e.g., "from
...models.document import Document, DocumentType, ImageBlock, ProcessedDocument,
TextBlock", "from ..image_preprocessor import ensure_png_compatible_mode,
pil_to_png_bytes", "from .document_parser import DocumentParser", "from
.registry import parser_registry"); replace these with absolute imports rooted
at the openrag package (e.g., import Document, DocumentType, ImageBlock,
ProcessedDocument, TextBlock from openrag.models.document;
ensure_png_compatible_mode and pil_to_png_bytes from
openrag.core.indexing.image_preprocessor or the correct absolute module;
DocumentParser and parser_registry from their absolute openrag paths) so IDEs
and refactors resolve symbols consistently across packages.
- Around line 119-159: _extract_embedded_images currently builds an "ordered"
dict and returns a list of length max_order filling gaps with None, which
misaligns placeholders when DOCX image numbering is non-contiguous; change the
logic to collect (order_num, bytes_or_None) pairs as you read media, then sort
those pairs by order_num and return a compact list of just the values
(preserving None for unsupported formats but only for images actually found)
instead of generating a list up to max_order; update references inside the
function (media loop, ordered -> list of tuples, and the final return) to
implement this sorted-compact behavior while keeping existing exception handling
and PIL conversion steps.
In `@openrag/core/indexing/parsers/pdf/pymupdf.py`:
- Around line 20-26: The module currently performs hard imports of optional
backends (pymupdf and pymupdf4llm) at top-level; move those imports into the
specific functions or methods that actually use them (e.g., inside the PDF
parser class methods that call pymupdf/pymupdf4llm) and wrap them in a
try/except ImportError to raise a clear, contextual error when the backend is
instantiated; this ensures import-time of
openrag.core.indexing.parsers.pdf.pymupdf does not fail in environments without
those optional libraries while still providing a helpful message if the backend
is chosen at runtime.
In `@openrag/core/indexing/parsers/pptx_parser.py`:
- Around line 191-196: The ValueError handler in _chart_to_markdown contains a
redundant conditional—both the if branch checking "unsupported plot type" in
str(exc) and the else return the same "\n\n[unsupported chart]\n\n"; remove the
unnecessary conditional and simplify the handler to a single return, or if you
intended different behavior, change one branch to a distinct message or add
logging that includes the exception (reference: _chart_to_markdown handling of
ValueError).
- Around line 86-117: Guard against slide title being None and use identity
comparison: when iterating slides in the loop that reads slide.shapes.title,
change the check from `shape == title` to `title is not None and shape is title`
(or equivalent) so you first ensure `title` exists and then compare by identity;
update the branch that appends heading text (the block that currently does `if
shape == title: md += "# " + shape.text.lstrip() + "\n"`) to use this guarded
identity check to avoid relying on python-pptx’s __eq__ behavior.
In `@openrag/core/indexing/parsers/test_docx_parser.py`:
- Around line 24-30: The tests create temporary .docx files with _fake_docx and
direct tempfile.NamedTemporaryFile(delete=False) calls which leak files; update
_fake_docx to accept a pytest tmp_path (or Path) and write the zip there instead
of using NamedTemporaryFile, and change the tests test_no_media_returns_empty
and test_invalid_zip_returns_empty (and other callers) to accept and pass
tmp_path into _fake_docx so pytest will auto-clean the files; ensure you replace
delete=False usages and return a Path within tmp_path.
🪄 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: 34156e55-d571-4eb8-9900-27a8f7e23856
📒 Files selected for processing (18)
openrag/core/indexing/parsers/audio/client_based.pyopenrag/core/indexing/parsers/audio/local_whisper.pyopenrag/core/indexing/parsers/doc_parser.pyopenrag/core/indexing/parsers/docx_parser.pyopenrag/core/indexing/parsers/eml_parser.pyopenrag/core/indexing/parsers/html_parser.pyopenrag/core/indexing/parsers/image_parser.pyopenrag/core/indexing/parsers/markdown_parser.pyopenrag/core/indexing/parsers/pdf/client_based.pyopenrag/core/indexing/parsers/pdf/marker.pyopenrag/core/indexing/parsers/pdf/pymupdf.pyopenrag/core/indexing/parsers/pptx_parser.pyopenrag/core/indexing/parsers/test_doc_parser.pyopenrag/core/indexing/parsers/test_docx_parser.pyopenrag/core/indexing/parsers/text_parser.pyopenrag/core/indexing/test_image_preprocessor.pyopenrag/core/indexing/test_validators.pyopenrag/services/inference/parsers/test_openai_audio.py
🚧 Files skipped from review as they are similar to previous changes (3)
- openrag/core/indexing/parsers/pdf/marker.py
- openrag/core/indexing/parsers/image_parser.py
- openrag/core/indexing/parsers/eml_parser.py
Legacy BaseLoaders become thin adapters: read file -> bytes, build
CoreDocument, call core parser, map ProcessedDocument back to a
langchain Document. ImageBlock markdown_ref substitution preserves
the legacy captioned-page_content contract.
Shimmed: TextLoader, MarkdownLoader, ImageLoader, DocxLoader, DocLoader,
PPTXLoader, PyMuPDFLoader / PyMuPDF4LLMLoader, MarkerLoader,
LocalWhisperLoader, OpenAIAudioLoader.
Skipped (rationale in REFACTORING_DECISION_LOG.md): EmlLoader (new
EmlParser attachment-parser DI doesn't match the legacy
BaseLoader-keyed fallback chain); pdf_loaders/{openai,dotsocr}.py
(services-side clients need a concrete core.vlm.VLM that doesn't exist
yet; both are dead code on this branch).
Runtime fixes:
- core/indexing/parsers/pdf/pymupdf.py: serialize pymupdf work onto a
module-level single-thread ThreadPoolExecutor (upstream
pymupdf/PyMuPDF#3771 -- pymupdf is not thread-safe, wontfix); retain
empty pages for 1-to-1 source pagination.
- core/config/indexation.py: port direct_upload_suffixes field +
validator from the now-vestigial config/models.py:TranscriberConfig
(the active config loader is core/config; the missing field caused
AttributeError on audio loader init).
- services/workers/parsers/marker_workers.py: description=lambda ->
format-string template (_resolve_description only handles strings).
Other:
- loaders/base.py Stage 1: re-export the four image_preprocessor
symbols already in core; rewrite _pil_image_to_base64 on top of
pil_to_png_bytes. The captioning mixin stays in place pending
Stage 2.
- core/indexing/parsers/audio/client_based.py: delegate supported_types
to the wrapped client (matches MarkerParser pattern).
- test_doc_loader.py: rewritten for shim-level coverage (langchain
Document round-trip, save_markdown integration, parser error
propagation); spire internals are covered by
core/indexing/parsers/test_doc_parser.py.
- REFACTORING_DECISION_LOG.md: Phase 5E entries; consolidated 5D
groupings (validators, parser layering, ray_utils API, ImageBlock
contract, client-backed facades).
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
openrag/services/workers/parsers/marker_workers.py (1)
348-368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty pages still dropped — page-level contract still violated.
_split_pagesskips pages with empty text (line 447), which meansparse()emits fewerTextBlocks than the source PDF has pages, breaking the "one TextBlock per page withpage_numberset" contract this PR introduces. As a downstream effect,page_count=pages[-1][0] if pages else 0(line 367) is wrong whenever the document ends with one or more blank pages — the last retained tuple is no longer the real last page.💡 Proposed fix to retain blank pages and correct page_count
pairs: list[tuple[int, str]] = [] cursor = 0 last_page = 0 for match in cls._PAGE_MARKER_RE.finditer(markdown): page = int(match.group(1)) text = markdown[cursor : match.start()].strip() - if text: - pairs.append((page, text)) + pairs.append((page, text)) cursor = match.end() last_page = page tail = markdown[cursor:].strip() - if tail: - pairs.append((last_page + 1, tail)) + if tail or last_page == 0: + pairs.append((max(last_page + 1, 1), tail)) return pairsAlso consider deriving
page_countfrommax(p for p, _ in pages)(or tracking it separately during the split) instead of relying on the last tuple, so a blank trailing page can't desync the count.Also applies to: 441-454
🤖 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 `@openrag/services/workers/parsers/marker_workers.py` around lines 348 - 368, The parse() implementation currently relies on _split_pages which drops empty pages, violating the "one TextBlock per page" contract; update the split/assembly so every source page produces a TextBlock (including ones with empty text) and stop using pages[-1][0] to compute page_count. Specifically, ensure _split_pages (or parse after calling it) returns/expands entries for blank pages (so pages contains a tuple for every page number), build TextBlock(text=text, page_number=page) for each page including empty strings, and compute page_count as the maximum page number (e.g., max(p for p, _ in pages) or track page_count while splitting) instead of relying on the last tuple. Ensure symbols referenced: parse, _split_pages, TextBlock, page_count, pages.
🧹 Nitpick comments (2)
openrag/components/indexer/loaders/audio/local_whisper.py (2)
28-30: 💤 Low valueMisleading alias:
_ServicesWhisperPoolis aBasePooledParser, not a pool.
services.workers.parsers.whisper_workers.LocalWhisperLoaderis theBasePooledParserfacade that holds a handle to the namedWhisperPoolRay actor — it isn't itself the pool. Aliasing it to_ServicesWhisperPoolhere makes the constructor at line 47 (LocalWhisperParser(pool=_ServicesWhisperPool())) read as if a pool object is being passed, when in fact it's the parser-side wrapper. A name like_ServicesWhisperParseror_WhisperPoolParserbetter matches the type and thepool=parameter contract onLocalWhisperParser.♻️ Suggested rename
from services.workers.parsers.whisper_workers import ( # noqa: F401 (re-exported for legacy import paths) - LocalWhisperLoader as _ServicesWhisperPool, + LocalWhisperLoader as _ServicesWhisperParser, ) @@ - self._parser = LocalWhisperParser(pool=_ServicesWhisperPool()) + self._parser = LocalWhisperParser(pool=_ServicesWhisperParser())🤖 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 `@openrag/components/indexer/loaders/audio/local_whisper.py` around lines 28 - 30, The alias _ServicesWhisperPool is misleading because services.workers.parsers.whisper_workers.LocalWhisperLoader is a BasePooledParser facade, not the pool; rename the alias to something reflecting that (e.g., _ServicesWhisperParser or _WhisperPoolParser) and update the constructor call LocalWhisperParser(pool=_ServicesWhisperPool()) to use the new alias so the pool= parameter clearly receives the parser facade symbol LocalWhisperLoader under a descriptive name.
64-64: 💤 Low valuePage boundaries lost when multiple
TextBlocks are emitted.Today
LocalWhisperLoader(services) emits a singleTextBlock, so this is benign. However, the PR explicitly switches paginated parsers to "oneTextBlockper page withpage_numberset" and removes in-band[PAGE_N]markers; if the Whisper backend ever produces per-segment/per-chunk blocks,"".join(...)will concatenate them with no separator and no page metadata, silently corrupting downstream chunking. Consider joining with"\n\n"and/or preservingpage_numbervia per-blockDocuments.🤖 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 `@openrag/components/indexer/loaders/audio/local_whisper.py` at line 64, LocalWhisperLoader currently flattens processed.text_blocks into content with "".join(...) which drops page boundaries and page_number metadata; change the assembly to join blocks with a clear separator (e.g. "\n\n") and/or emit a Document per TextBlock preserving block.page_number so downstream chunkers still see page boundaries. Locate the join of processed.text_blocks (the content = "".join(...) line) and replace it with code that either (a) uses "\n\n".join(b.text for b in processed.text_blocks) to keep separators or (b) maps each TextBlock to its own Document carrying b.text and b.page_number (or both approaches together) so page metadata is not lost.
🤖 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 `@openrag/components/indexer/loaders/audio/local_whisper.py`:
- Line 65: The code constructs a langchain Document with metadata that may be
None (doc = Document(page_content=content, metadata=metadata)); instead, ensure
you pass a dict and merge any parser-produced metadata from processed (the
CoreDocument) so validation succeeds and parser metadata is preserved: build the
final metadata as {**(dict(processed.metadata) if processed and
processed.metadata else {}), **(dict(metadata) if metadata else {})} and pass
that into Document(page_content=content, metadata=final_metadata); update the
variable where Document is created (doc) to use this merged dict.
- Around line 49-65: aload_document currently hardcodes
content_type=DocumentType.AUDIO which mislabels videos; update aload_document
(or its callsites) to determine the content_type dynamically (e.g., inspect
Path(file_path).suffix and map common video extensions to DocumentType.VIDEO,
audio extensions to DocumentType.AUDIO) or add an explicit parameter (e.g.,
content_type=None) that, if provided, is used; set
CoreDocument(content_type=determined_type, ...) instead of DocumentType.AUDIO
and ensure compatibility with LocalWhisperParser.supported_types() and any
existing LocalWhisperLoader behavior.
In `@openrag/components/indexer/loaders/audio/openai.py`:
- Around line 37-46: The helper _get_whisper_actor has a race: replace the
current ray.get_actor call with ray.get_actor(actor_name, namespace="openrag",
get_if_exists=True) and if that returns a non-None actor return it; if it
returns None call WhisperActor.options(name=actor_name,
namespace="openrag").remote() inside a try/except that catches the name-conflict
ValueError and in that except block re-call ray.get_actor(actor_name,
namespace="openrag", get_if_exists=True) to return the actor created by the
concurrent caller; preserve the generic Exception branch to log and re-raise as
before.
In `@openrag/components/indexer/loaders/doc.py`:
- Around line 53-71: The current logic only calls
replace_markdown_images_with_captions when processed.images exists and leaves
markdown_ref placeholders when self.image_captioning is False; instead, always
invoke self.replace_markdown_images_with_captions(result, ...) whenever
self.image_captioning is True (so linked markdown images are captioned even if
processed.images is empty), and when self.image_captioning is False remove/strip
embedded refs from processed.images (the (block.metadata or
{}).get("markdown_ref") placeholders) so they are not left in result; keep the
existing flow that opens images and calls self.caption_images(pil_images, ...)
and replaces extracted-image refs with captions when processed.images is
present, but decouple that from the linked-image pass using
replace_markdown_images_with_captions.
In `@openrag/components/indexer/loaders/docx.py`:
- Around line 58-76: When image_captioning is False the docx loader logs
"Ignoring images" but leaves embedded markdown refs in the output, causing
placeholders to be indexed; modify the branch in the code around
image_captioning in load/process (refer to image_captioning, processed.images
and the markdown_ref field) to strip any embedded image placeholders by
iterating processed.images and removing/replacing each (block.metadata or
{}).get("markdown_ref") from result (similar to what
replace_markdown_images_with_captions does for linked images) so the returned
markdown no longer contains raw markdown_ref placeholders when captioning is
disabled.
In `@openrag/components/indexer/loaders/pdf_loaders/pymupdf.py`:
- Around line 47-58: Normalize the metadata argument at the start of
PyMuPDF4LLMLoader.aload_document (and the second loader method in the same file
around lines 71-102) by converting None to an empty dict (e.g., metadata =
dict(metadata) if metadata else {}) before constructing CoreDocument and before
creating the returned langchain Document; ensure both CoreDocument(...) and
Document(page_content=..., metadata=metadata) receive the same normalized dict
to keep behavior consistent.
In `@openrag/components/indexer/loaders/pptx_loader.py`:
- Around line 34-67: aload_document currently builds core_doc with a normalized
metadata dict but later passes the original metadata (which may be None) into
Document; normalize metadata once at the top (e.g., metadata = dict(metadata) if
metadata else {}) and use that same variable when constructing CoreDocument and
when creating the returned Document(page_content=md_content, metadata=metadata)
so Document never receives None; update references in the function
(aload_document, core_doc, and the final Document call) accordingly.
---
Duplicate comments:
In `@openrag/services/workers/parsers/marker_workers.py`:
- Around line 348-368: The parse() implementation currently relies on
_split_pages which drops empty pages, violating the "one TextBlock per page"
contract; update the split/assembly so every source page produces a TextBlock
(including ones with empty text) and stop using pages[-1][0] to compute
page_count. Specifically, ensure _split_pages (or parse after calling it)
returns/expands entries for blank pages (so pages contains a tuple for every
page number), build TextBlock(text=text, page_number=page) for each page
including empty strings, and compute page_count as the maximum page number
(e.g., max(p for p, _ in pages) or track page_count while splitting) instead of
relying on the last tuple. Ensure symbols referenced: parse, _split_pages,
TextBlock, page_count, pages.
---
Nitpick comments:
In `@openrag/components/indexer/loaders/audio/local_whisper.py`:
- Around line 28-30: The alias _ServicesWhisperPool is misleading because
services.workers.parsers.whisper_workers.LocalWhisperLoader is a
BasePooledParser facade, not the pool; rename the alias to something reflecting
that (e.g., _ServicesWhisperParser or _WhisperPoolParser) and update the
constructor call LocalWhisperParser(pool=_ServicesWhisperPool()) to use the new
alias so the pool= parameter clearly receives the parser facade symbol
LocalWhisperLoader under a descriptive name.
- Line 64: LocalWhisperLoader currently flattens processed.text_blocks into
content with "".join(...) which drops page boundaries and page_number metadata;
change the assembly to join blocks with a clear separator (e.g. "\n\n") and/or
emit a Document per TextBlock preserving block.page_number so downstream
chunkers still see page boundaries. Locate the join of processed.text_blocks
(the content = "".join(...) line) and replace it with code that either (a) uses
"\n\n".join(b.text for b in processed.text_blocks) to keep separators or (b)
maps each TextBlock to its own Document carrying b.text and b.page_number (or
both approaches together) so page metadata is not lost.
🪄 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: 21f03c23-adf1-4406-929c-acff6d1f3aaf
📒 Files selected for processing (16)
REFACTORING_DECISION_LOG.mdopenrag/components/indexer/loaders/audio/local_whisper.pyopenrag/components/indexer/loaders/audio/openai.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/doc.pyopenrag/components/indexer/loaders/docx.pyopenrag/components/indexer/loaders/image.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pdf_loaders/pymupdf.pyopenrag/components/indexer/loaders/pptx_loader.pyopenrag/components/indexer/loaders/test_doc_loader.pyopenrag/components/indexer/loaders/txt_loader.pyopenrag/core/config/indexation.pyopenrag/core/indexing/parsers/audio/client_based.pyopenrag/core/indexing/parsers/pdf/pymupdf.pyopenrag/services/workers/parsers/marker_workers.py
- contextualize.py: bind tasks before background scheduling to avoid late-binding closure over loop variable - eml_parser.py: raise on attachment parse errors instead of silently dropping content; propagate ProcessedDocument metadata - image_parser.py: offload PIL decode + pixel-count check to a thread - pdf/__init__.py: repair module docstring (removed stale phase ref) - pptx_parser.py: return len(presentation.slides) for page_count instead of len(non-empty slides) - test_doc_parser.py: monkeypatch DocxParser inside DocParser to avoid coupling test to Spire.Doc internals - document.py: use delete=False + explicit os.unlink for Windows compat; add _safe_unlink to swallow already-deleted-file races - text.py: guard codec_info lookup with LookupError to handle unknown encoding names gracefully - openai_audio.py: add .wav to direct_upload_suffixes default - marker_workers.py: preserve blank pages for 1-to-1 source pagination; fix description= arg (lambda → format-string template) - whisper_workers.py: use call_ray_actor_with_timeout for pool dispatch - ray_utils.py: do not retry on TaskCancelledError
- local_whisper.py: derive content_type from filename so video files get DocumentType.VIDEO; normalize metadata before passing to langchain Document - openai.py: use get_if_exists=True for idempotent WhisperActor get-or-create (avoids race condition between concurrent callers) - doc.py: decouple linked-image captioning from embedded-image check so DOC files with linked markdown images but no extracted ImageBlocks still call replace_markdown_images_with_captions; normalize metadata - docx.py: strip markdown_ref placeholders when captioning is disabled (parity with PyMuPDF and PPTX shims); normalize metadata - pymupdf.py: normalize metadata once at method entry in both loaders so CoreDocument and the returned langchain Document use the same dict - pptx_loader.py: normalize metadata once at method entry
4 tasks
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
Phases 5D–5E of the hexagonal refactor: rebuilds the parser layer and adapts the legacy loaders as thin shims.
core/indexing/parsers/): native parsers + thin facades naming each backend.services/inference|workers/parsers/): VLM-client and Ray-pool implementations.components/indexer/loaders/): each legacy loader now delegates to the matching core parser, layers image captioning on top, and preserves the[PAGE_N]anchor layout expected by the current chunker.ray_utils,text_sanitizer,consts, exceptions, and file-id validation re-export from their new locations.Parsers added
services/inference)services/workers)pymupdfopenai_pdf,dotsocrmarkerlocal_whisperBehaviour change worth flagging
Paginated parsers (Marker / PPTX / PyMuPDF) now emit one
TextBlockper page withpage_numberset. In-band[PAGE_N]markers are gone — the future chunker must iteratetext_blocksand carrypage_number, not scan text for markers. See decision log #20.The loader shims reconstruct the legacy
\n[PAGE_N]\nlayout fromtext_blocksso existing chunkers are unaffected during migration.Other additions
Document.as_temporary_file()— async context manager replacing the per-parserNamedTemporaryFiledance.ImageBlockparser→caption contract documented on the model:markdown_refmetadata,image_urlproperty,source_urlfor HTTP refs.@with_timeout/@with_retrydecorators inservices/workers/ray_utilswithstr.format-style description templates.PyMuPDFLoader,PyMuPDF4LLMLoader,PPTXLoader,DocxLoader,DocLoader,OpenAIAudioLoader,LocalWhisperLoader.Out of scope
EmlLoadershim deferred — no legacy consumer discovered;EmlParseris available directly.Full rationale:
REFACTORING_DECISION_LOG.mdentries 6–20.Test plan
markdown_refthrough the legacy chunkerSummary by CodeRabbit
New Features
Improvements
Tests