correct IndexError caused by empty list of chunks when performing chu… - #202
Conversation
📝 WalkthroughWalkthroughAdds explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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: 0
🧹 Nitpick comments (1)
openrag/components/indexer/chunker/chunker.py (1)
301-313: Correct empty-chunk handling; minor log clarity improvement possible.The empty-chunk guard correctly prevents downstream errors. However, the log message "Contextualizing chunks" is always emitted even when
contextual_retrievalisFalse, which could be slightly misleading when reading logs.Optional: Adjust log message for clarity
if chunks: - # Apply contextualization if enabled - log.info( - "Contextualizing chunks", - apply_contextualization=self.contextual_retrieval, - ) + # Apply contextualization if enabled + log.info( + "Applying chunk formatting", + contextual_retrieval=self.contextual_retrieval, + ) chunks = await self._apply_contextualization( chunks, lang=detected_lang, filename=filename )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openrag/components/indexer/chunker/chunker.py
🔇 Additional comments (5)
openrag/components/indexer/chunker/chunker.py (5)
69-74: LGTM!Good addition of the
filenameparameter with a sensible default. This maintains backward compatibility while enabling explicit filename propagation.
138-158: LGTM!The filename parameter is correctly propagated through to
contextualize_chunksand used in chunk formatting.
167-175: Substring check vs exact match inconsistency with PR description.The PR description states this uses "exact pattern matching," but the implementation uses
in(substring check) rather than==(exact equality). While checking for the full placeholder format is a significant improvement over the previous check for just[Image Placeholder], this could still produce false positives if content contains the placeholder string embedded in larger text.If uninformative images always have exactly this content, consider using exact equality:
Suggested fix for exact matching
img_placeholder = ( """<image_description>\n\n[Image Placeholder]\n\n</image_description>""" ) for e in md_elements: if e.type in ("table", "image"): if ( - e.type == "image" and img_placeholder.lower() in e.content.lower() + e.type == "image" and e.content.strip().lower() == img_placeholder.lower() ): # skip placeholder images continuePlease verify whether placeholder images always have exactly this content or may include surrounding whitespace/text.
274-281: LGTM!This correctly addresses the IndexError issue by checking for an empty chunks list before attempting to sort. The warning log provides useful debugging information when no chunks are created.
39-39: Note: Different metadata keys used for filename.
_generate_contextextracts filename frommetadata.get("source", "unknown")whilesplit_documentextracts frommetadata.get("filename", ""). If these metadata keys can have different values, ensure this is intentional.
paultranvan
left a comment
paultranvan
left a comment
There was a problem hiding this comment.
Looks good, with small remarks
0344755 to
45861df
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openrag/components/indexer/chunker/chunker.py`:
- Around line 274-279: The code currently logs "No chunks created. Content is
empty or image is not informative." with log.info but the PR asks for a warning
level; update the logging call from log.info to log.warning (the call
referencing chunks and the log variable in chunker.py) so empty-chunk cases are
emitted as warnings instead of informational messages.
- Around line 167-175: The code currently uses substring matching ("in") to
detect placeholders via the img_placeholder string, which contradicts the PR's
requirement for exact pattern matching; update the check in chunker.py to
compare the normalized contents exactly (e.g., normalize both sides with strip()
and lower() and use equality) when inspecting md_elements entries (use e.type
and e.content) so only true placeholders are skipped, and move the
img_placeholder value out to a shared constant (e.g., IMAGE_PLACEHOLDER) in the
constants module and import it into the chunker to avoid duplicating the
literal.
🧹 Nitpick comments (1)
openrag/components/indexer/chunker/chunker.py (1)
31-39: Potential IndexError ifcontextualize_chunksis called directly with an empty list.While the current internal flow guards against this (via the
len(chunks) < 2check in_apply_contextualization), the public methodcontextualize_chunkswould raise anIndexErrorat line 39 and 77 if called directly with an empty list.Consider adding a guard at the start of
contextualize_chunks:async def contextualize_chunks( self, chunks: list[Document], lang: Literal["fr", "en"] = "en", filename: str = "", ) -> list[Document]: """Contextualize a list of document chunks.""" + if not chunks: + return [] try: first_chunks = chunks[:2]Also note that
_generate_contextextractsfilenamefromfirst_chunks[0].metadata.get("source", "unknown")(line 39), but thefilenameparameter passed tocontextualize_chunksis only used for progress display and formatting—not in_generate_context. This creates a subtle inconsistency where the context generation uses a different filename source than the chunk formatting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openrag/components/indexer/chunker/chunker.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (3)
openrag/components/indexer/chunker/chunker.py (3)
69-108: LGTM!The
filenameparameter addition properly propagates through the contextualization flow, enabling explicit filename handling for progress descriptions and chunk formatting.
138-158: LGTM!The
filenameparameter is correctly propagated to both the non-contextual formatting path and the contextualizer, ensuring consistent chunk formatting regardless of whether contextualization is enabled.
281-311: LGTM!The changes properly address the original IndexError by:
- Extracting
filenamesafely with a default empty string- Checking for empty chunks before proceeding to contextualization
- Returning early with an empty list when no chunks are created
This makes the pipeline more robust against empty-chunk scenarios.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| img_placeholder = ( | ||
| """<image_description>\n\n[Image Placeholder]\n\n</image_description>""" | ||
| ) | ||
| for e in md_elements: | ||
| if e.type in ("table", "image"): | ||
| if ( | ||
| e.type == "image" and "[Image Placeholder]" in e.content | ||
| e.type == "image" and img_placeholder.lower() in e.content.lower() | ||
| ): # skip placeholder images | ||
| continue |
There was a problem hiding this comment.
The implementation uses substring matching, not exact pattern matching as described in the PR.
The PR objective states this should use "exact pattern matching so only true placeholders are removed," but the code still uses in (substring check). If exact matching is intended, this should use equality comparison:
- if (
- e.type == "image" and img_placeholder.lower() in e.content.lower()
- ): # skip placeholder images
+ if (
+ e.type == "image" and e.content.strip().lower() == img_placeholder.lower()
+ ): # skip placeholder imagesIf substring matching is intentional, the case-insensitive check could still filter out informative images if the VLM output happens to contain the phrase [image placeholder] within actual descriptive content.
Additionally, consider moving img_placeholder to a constants file as suggested in prior review.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| img_placeholder = ( | |
| """<image_description>\n\n[Image Placeholder]\n\n</image_description>""" | |
| ) | |
| for e in md_elements: | |
| if e.type in ("table", "image"): | |
| if ( | |
| e.type == "image" and "[Image Placeholder]" in e.content | |
| e.type == "image" and img_placeholder.lower() in e.content.lower() | |
| ): # skip placeholder images | |
| continue | |
| img_placeholder = ( | |
| """<image_description>\n\n[Image Placeholder]\n\n</image_description>""" | |
| ) | |
| for e in md_elements: | |
| if e.type in ("table", "image"): | |
| if ( | |
| e.type == "image" and e.content.strip().lower() == img_placeholder.lower() | |
| ): # skip placeholder images | |
| continue |
🤖 Prompt for AI Agents
In `@openrag/components/indexer/chunker/chunker.py` around lines 167 - 175, The
code currently uses substring matching ("in") to detect placeholders via the
img_placeholder string, which contradicts the PR's requirement for exact pattern
matching; update the check in chunker.py to compare the normalized contents
exactly (e.g., normalize both sides with strip() and lower() and use equality)
when inspecting md_elements entries (use e.type and e.content) so only true
placeholders are skipped, and move the img_placeholder value out to a shared
constant (e.g., IMAGE_PLACEHOLDER) in the constants module and import it into
the chunker to avoid duplicating the literal.
45861df to
f84eac5
Compare
2 participants
This PR makes the chunking pipeline more robust by:
filenameexplicitly to avoid the metadata extraction issue in IndexError: list index out of range in chunker contextualization #201"""<image_description>\n\n[Image Placeholder]\n\n</image_description>"""[Image Placeholder]appeared anywhere in the description, incorrectly removing informative images where the VLM happened to include that phraseSummary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.