Add image_captioning_url config param to control indexing of image URL - #217
Conversation
📝 WalkthroughWalkthroughCentralizes image-captioning into BaseLoader (new async caption utilities and regex patterns), adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Loader as Loader
participant Base as BaseLoader
participant VLM as VLM API
participant Indexer as Indexer
Client->>Loader: Upload file (md/docx/pdf/...)
Loader->>Loader: extract content and images (embedded & linked)
Loader->>Base: caption_images(list_of_PIL_images)
Base->>VLM: async requests for image descriptions
VLM-->>Base: captions
Base-->>Loader: captions (ordered)
Loader->>Base: replace_markdown_images_with_captions(content, flags)
Base-->>Loader: content with captions substituted
Loader->>Indexer: save indexed document
Indexer-->>Client: indexing complete / content available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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
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: 2
🤖 Fix all issues with AI agents
In @.hydra_config/config.yaml:
- Around line 92-95: The YAML default for loader.image_captioning_url is
inverted; change its decoded default from true to false so HTTP/HTTPS captioning
is opt-in. Update the loader.image_captioning_url entry (and verify
loader.image_captioning remains true by default if intended) to use
${oc.decode:${oc.env:IMAGE_CAPTIONING_URL, false}} and ensure the environment
variable handling matches the intended opt-in behavior.
In `@openrag/components/indexer/loaders/base.py`:
- Around line 38-39: The loader currently enables external HTTP/HTTPS captioning
by default because self.image_captioning_url uses
self.config.loader.get("image_captioning_url", True); change this to opt-in by
using False as the default so external fetching is disabled unless explicitly
set. Update the reference in openrag/components/indexer/loaders/base.py where
image_captioning_url is assigned, and verify any consumers of the attribute
(e.g., code paths in the image captioning flow) respect the boolean; also adjust
any tests or docs that assumed the old default.
🧹 Nitpick comments (2)
tests/api_tests/test_indexer.py (2)
292-320: Make the data‑URI assertion deterministic.
The"image"fallback can pass even if captioning didn’t occur; assert the wrapper tag instead to reduce false positives.Proposed tweak
- assert "<image_description>" in indexed_content or "image" in indexed_content.lower() + assert "<image_description>" in indexed_content, "Expected caption wrapper in indexed content"
321-366: Consider tying the HTTP-URL expectation to config.
As written, the test passes regardless ofimage_captioning_url, which can hide regressions. Parameterizing by the flag (or reading it from test config) would make the behavior explicit.
d794b76 to
fc82a78
Compare
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
LGTM.
I've tested PDFs files with images and it works fine
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/api_tests/test_indexer.py`:
- Around line 321-324: The test's fallback assertion is too broad and can match
original alt text; update the assertions in test_indexer.py to require the
explicit caption tag and also assert no markdown image syntax remains: keep the
existing check that "data:image/png;base64" is not in indexed_content, replace
the or condition with a strict assert that "<image_description>" is present in
indexed_content, and add an assertion that the markdown image pattern "![" (or
"![") followed by "](" is not present in indexed_content to ensure images were
captioned and not left as markdown embeds; locate these changes around the
indexed_content assembly using the documents variable in the test.
♻️ Duplicate comments (1)
openrag/components/indexer/loaders/base.py (1)
39-40: Defaultimage_captioning_urlshould be opt‑in.This currently enables external URL fetching when the flag is omitted; the PR objective says it should default to disabled.
✅ Proposed fix
- self.image_captioning_url = self.config.loader.get("image_captioning_url", True) + self.image_captioning_url = self.config.loader.get("image_captioning_url", False)
| # Verify image was replaced with caption (not raw data URI) | ||
| indexed_content = " ".join(doc.get("page_content", "") for doc in documents) | ||
| assert "data:image/png;base64" not in indexed_content, "Image should be captioned, not raw" | ||
| assert "<image_description>" in indexed_content or "image" in indexed_content.lower() |
There was a problem hiding this comment.
Assertion may pass spuriously due to alt text containing "image".
The fallback condition "image" in indexed_content.lower() could match the original alt text (e.g., "test image") rather than an actual generated caption, causing false positives if captioning silently fails.
Consider tightening the assertion to require the <image_description> tag specifically, or verify the absence of the markdown image syntax  as additional confirmation.
Suggested fix
# Verify image was replaced with caption (not raw data URI)
indexed_content = " ".join(doc.get("page_content", "") for doc in documents)
assert "data:image/png;base64" not in indexed_content, "Image should be captioned, not raw"
- assert "<image_description>" in indexed_content or "image" in indexed_content.lower()
+ assert "<image_description>" in indexed_content, "Expected caption tag in indexed content"📝 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.
| # Verify image was replaced with caption (not raw data URI) | |
| indexed_content = " ".join(doc.get("page_content", "") for doc in documents) | |
| assert "data:image/png;base64" not in indexed_content, "Image should be captioned, not raw" | |
| assert "<image_description>" in indexed_content or "image" in indexed_content.lower() | |
| # Verify image was replaced with caption (not raw data URI) | |
| indexed_content = " ".join(doc.get("page_content", "") for doc in documents) | |
| assert "data:image/png;base64" not in indexed_content, "Image should be captioned, not raw" | |
| assert "<image_description>" in indexed_content, "Expected caption tag in indexed content" |
🤖 Prompt for AI Agents
In `@tests/api_tests/test_indexer.py` around lines 321 - 324, The test's fallback
assertion is too broad and can match original alt text; update the assertions in
test_indexer.py to require the explicit caption tag and also assert no markdown
image syntax remains: keep the existing check that "data:image/png;base64" is
not in indexed_content, replace the or condition with a strict assert that
"<image_description>" is present in indexed_content, and add an assertion that
the markdown image pattern "![" (or "![") followed by "](" is not present in
indexed_content to ensure images were captioned and not left as markdown embeds;
locate these changes around the indexed_content assembly using the documents
variable in the test.
- Add IMAGE_CAPTIONING_URL config option (default: false) to control whether HTTP/HTTPS image URLs should be fetched and captioned - MarkdownLoader: add missing self.image_captioning guard and skip HTTP URLs when IMAGE_CAPTIONING_URL is false (fixes badge errors) - DocxLoader: add HTTP URL detection and optional captioning for linked images based on IMAGE_CAPTIONING_URL setting - PPTXLoader: add missing self.image_captioning guard for consistency - Update CLAUDE.md with loader image captioning pattern documentation - Document IMAGE_CAPTIONING_URL env var in docs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add shared regex patterns HTTP_IMAGE_PATTERN and DATA_URI_IMAGE_PATTERN - Add caption_images() for list of PIL images with cancellation handling - Add replace_markdown_images_with_captions() for markdown content - Simplify MarkdownLoader, DocxLoader, PPTXLoader, MarkerLoader, DoclingLoader to use the new shared methods instead of duplicated code - Update CLAUDE.md with new BaseLoader methods documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add sample_markdown_with_image fixture - Add TestImageCaptioning class with tests for data URI and HTTP URL handling - Skip tests when IMAGE_CAPTIONING is disabled (CI) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Catch BadRequestError separately to avoid full stack traces - Log as warning with truncated error message Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
66e92cc to
5bf4857
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/content/docs/documentation/env_vars.md`:
- Around line 21-22: Update the docs to make IMAGE_CAPTIONING_URL opt-in: change
its default value from `true` to `false` and adjust the description to state
that when set to `true` the app will fetch HTTP/HTTPS image URLs in markdown
files and use the VLM to describe them; keep IMAGE_CAPTIONING description as-is
but ensure wording clarifies that URL fetching only happens if
IMAGE_CAPTIONING_URL is enabled.
♻️ Duplicate comments (2)
.hydra_config/config.yaml (1)
92-94: Default forimage_captioning_urlshould be opt‑in (false).The current default enables HTTP/HTTPS captioning by default, which conflicts with the PR intent.
✅ Proposed fix
- image_captioning_url: ${oc.decode:${oc.env:IMAGE_CAPTIONING_URL, true}} + image_captioning_url: ${oc.decode:${oc.env:IMAGE_CAPTIONING_URL, false}}openrag/components/indexer/loaders/base.py (1)
39-40: Defaultimage_captioning_urlshould beFalseper PR objectives.The PR description states HTTP/HTTPS URLs should not be fetched/captioned by default (opt-in). The current default of
Truecontradicts this intent.- self.image_captioning_url = self.config.loader.get("image_captioning_url", True) + self.image_captioning_url = self.config.loader.get("image_captioning_url", False)
🧹 Nitpick comments (1)
openrag/components/indexer/loaders/base.py (1)
180-186: Dead cancellation logic—coroutines lack.cancel().The
taskslist contains coroutines, notasyncio.Taskobjects. Coroutines don't have acancel()method, sohasattr(task, "cancel")will beFalseand this loop never executes. The code is harmless but misleading.Either remove the ineffective block (as done in
replace_markdown_images_with_captions) or wrap coroutines withasyncio.create_task()if cancellation is truly needed.Option 1: Remove dead code (simpler)
try: results = await tqdm.gather(*tasks, desc=desc) except asyncio.CancelledError: - for task in tasks: - if hasattr(task, "cancel"): - task.cancel() raise return results
| | `IMAGE_CAPTIONING` | `bool` | `true` | If `true`, an LLM is used to describe images and convert them into text using a [specific prompt](https://github.com/linagora/openrag/blob/main/prompts/example1/image_captioning_tmpl.txt). The image in files are replaced by their descriptions | | ||
| | `IMAGE_CAPTIONING_URL` | `bool` | `true` | If `true`, HTTP/HTTPS image URLs in markdown files are fetched and described by the VLM. | |
There was a problem hiding this comment.
Align IMAGE_CAPTIONING_URL default with opt‑in behavior.
Docs show default true, but the PR intent is opt‑in to avoid fetching HTTP/HTTPS images by default. Please update the default and wording accordingly.
✏️ Proposed doc fix
-| `IMAGE_CAPTIONING_URL` | `bool` | `true` | If `true`, HTTP/HTTPS image URLs in markdown files are fetched and described by the VLM. |
+| `IMAGE_CAPTIONING_URL` | `bool` | `false` | If `true`, HTTP/HTTPS image URLs in markdown files are fetched and described by the VLM. |🤖 Prompt for AI Agents
In `@docs/content/docs/documentation/env_vars.md` around lines 21 - 22, Update the
docs to make IMAGE_CAPTIONING_URL opt-in: change its default value from `true`
to `false` and adjust the description to state that when set to `true` the app
will fetch HTTP/HTTPS image URLs in markdown files and use the VLM to describe
them; keep IMAGE_CAPTIONING description as-is but ensure wording clarifies that
URL fetching only happens if IMAGE_CAPTIONING_URL is enabled.
2 participants
When
image_captioningis true, we used to try to caption with VLM any image found as a URL in markdown.This can lead to errors if the image is wrong or not reachable. Also, this capability might be not expected, because it is a costly operation and the image could be irrelevant to caption, because it is an icon for example.
So we add a new param
image_captioning_urlto control this behaviour.This also make a refactoring of the loaders, to mutualize code and be consistent for image captioning
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.