Repository navigation
Remove duplicate LLM caption helper from ImageConverter - #2826
Open
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Open
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Conversation
`ImageConverter` now calls the shared `llm_caption` helper in `converters/_llm_caption.py` instead of its own private copy, `_get_llm_description`. `PptxConverter` already uses that helper. The private method and the imports only it needed are deleted. Behaviour stays the same: errors from the client still propagate, and a `None` caption still leaves the description out.
Mohamed Emad (TheGeekyM)
marked this pull request as ready for review
November 4, 2026 07:32
|
Mohamed Emad (@TheGeekyM) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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
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
ImageConverternow calls the sharedllm_captionhelper inconverters/_llm_caption.pyinstead of its own private copy,_get_llm_description.PptxConverteralready uses that helper. The private method and the imports only it needed are deleted. Behaviour stays the same: errors from the client still propagate, and aNonecaption still leaves the description out.Motivation
There were two copies of the image-captioning request code.
ImageConverter._get_llm_descriptionandllm_captionin_llm_caption.pywere the same line for line. Both used the same default prompt ("Write a detailed caption for this image."), resolved the content type the same way (mimetype, thenmimetypes.guess_typeon the extension, thenapplication/octet-stream), saved and restored the stream position around the base64 read, and sent the same OpenAI-style message.PptxConverter(_pptx_converter.py:186) already calledllm_caption. Only the image converter still had its own copy.The duplication was already known.
tests/test_module_misc.pyhad a TODO saying the LLM parameter test "may only use one test after the llm caption method duplicate has been removed", with a link to #1254.Keeping two copies means any change to the request has to be made twice: the default prompt, how content types are handled, the message format, or support for a different client. If someone misses one, images and PPTX slides start getting different captions. This affects anyone who passes
llm_client/llm_modeltoMarkItDown, and it is the main thing standing in the way of the larger goal of unifying the vision-caption code paths.What changed
packages/markitdown/src/markitdown/converters/_image_converter.pyllm_captionfrom._llm_captionand calls it inconvert()with the same arguments as before: stream, stream info,client,model,prompt=kwargs.get("llm_prompt")._get_llm_description.base64,mimetypesandUnionimports, which only that method used.packages/markitdown/tests/test_image.pytest_image_without_llm_is_unchangednow patchesimage_module.llm_captioninstead ofImageConverter._get_llm_description, so it still checks that no caption call happens when no LLM is configured.packages/markitdown/tests/test_module_misc.pytest_markitdown_llm_parameters. The image and PPTX assertions both stay: they exercise two different converters end to end, and each converter decides on its own whether to forwardllm_prompt.How it works
llm_captionis imported as a module-level name in_image_converter.py. That matches how_pptx_converter.pyimports it, and tests (and anyone extending the converter) can patch it in one place per converter module:monkeypatch.setattr(image_module, "llm_caption", ...). Existing PPTX tests already do this.The call site in
convert()keeps everything around it the same:llm_clientandllm_modelare set.Nonereturn is skipped, and anything else is stripped and appended under# Description:.try/exceptwas added. Before this change, an exception fromclient.chat.completions.createpropagated out ofImageConverter.convert(), and it still does.PptxConvertercatches caption failures and falls back to its own handling. I left that difference alone because changing it would change behaviour.Alternatives I considered:
_get_llm_descriptionas a thin wrapper aroundllm_caption. That would keep a subclass override point, but it is a private method, nothing in the repo overrides it (I checkedmarkitdownandmarkitdown-ocr), and keeping it would leave two names for one operation.DocumentConverteror a mixin. That is a bigger API decision, and it belongs with the CLI and vision-caption unification work, not this cleanup.test_module_misc.pyinto one, as the TODO suggested. I did not do this. Sharing the helper does not prove that each converter passesllm_promptthrough, and those two assertions are what check it.Overlap with #1254
Open PR #1254 covers the same duplication, and the TODO removed here links to it. This PR is deliberately narrow: one call site moves to the existing helper, and nothing else changes behaviour. If #1254 lands first, this PR can be closed or rebased down to the extra tests. If this one lands first, #1254 will need a rebase where it touches
_image_converter.pyand the TODO. Reviewers on both may want to agree which one goes first.Tests
packages/markitdown/tests/test_image.pytest_image_without_llm_is_unchanged(updated): with no LLM configured,llm_captionis not called and the output is the metadata only.test_image_converter_uses_shared_llm_caption:image_module.llm_captionis the same object as_llm_caption.llm_captionand_pptx_converter.llm_caption, andImageConverterno longer has_get_llm_description.test_image_converter_delegates_to_llm_caption(run with and withoutllm_prompt):convert()callsllm_captiononce with the original stream, stream info, client, model and prompt, strips the returned caption, formats it under# Description:, and does not call the client directly.test_image_converter_skips_description_when_llm_caption_returns_none: aNonecaption leaves the metadata output as it is, with no description heading.test_image_converter_requires_client_and_model_for_caption(run with only a client, then only a model):llm_captionis not called unless both are set.test_image_converter_default_prompt_via_llm_caption(promptNone,""," "): the request goes through the real helper and sends the default prompt.test_llm_caption_builds_data_uri_and_restores_stream: checks content-type resolution three ways (explicit mimetype wins over extension, extension-based guess,application/octet-streamfallback). It also checks that the data URI encodes only the bytes from the current position onward, that the stream position is restored afterwards, and that the custom prompt and model are passed through.packages/markitdown/tests/test_module_misc.pytest_markitdown_llm_parameters: only the TODO comment is removed. It still checks thatllm_promptreaches the client for both a JPEG and a PPTX.To run them:
The full suite and lint checks run through
hatch testandpre-commit run --all-files.Compatibility & risk
ImageConverter.convert()takes the same arguments and gives the same output for the same inputs.ImageConverter._get_llm_descriptionno longer exists. Code that subclassedImageConverterand overrode it, or patched it in tests, will no longer change captioning. That code should patchmarkitdown.converters._image_converter.llm_captioninstead. Nothing in this repo or inmarkitdown-ocrrelies on the old method.ImageConverter, and the stream-read failure path still returnsNoneand is skipped.convert()(_image_converter.py:71) and the overlap with Support Async LLM Client, reuse llm_caption helper #1254 described above.