[faysou]

Three independent fixes for the VLM pipeline, found by running docling --pipeline vlm --vlm-model granite_docling on arXiv 2206.01062 on Apple Silicon (auto-inline selects MLX). Each is its own commit.

1. Black picture images. DocTags bboxes are in the pixels of the page image, which is rendered at images_scale. The picture crop scaled the bbox by images_scale a second time, so with the CLI export setting (images_scale=2) the crop box fell outside the page image and PIL filled it with black. DocTags crops now use the bbox as it is. The other response formats (Chandra HTML, DocLang, dots, MinerU 2, DeepSeek-OCR, Nemotron Parse) give bboxes in page points and keep the scaled crop.
2. Lost text after a dense figure on MLX. On page 5 of the paper, Granite-Docling on the MLX engine splits Figure 4 into about 70 empty <picture> elements and ends the page before the caption and the text that follows. The Transformers engine does not do this on the same page. This adds EmptyPictureRunStopper (a run of 12 or more empty pictures) and makes the MLX engine regenerate such a page once with repetition_penalty=1.1. The check also runs on the finished output, because the streaming detokenizer can hold text back until generation ends. The retry runs without stoppers, so a false positive costs one penalized rerun and never truncates a page. The retry is enabled only in the Granite-Docling MLX preset, through the retry_repetition_penalty key of extra_generation_config. Only the MLX engine reads this key, as the MlxVlmEngine docstring states.
3. Shutdown error. VlmConvertModel.__del__ logged through a module logger that can already be None at interpreter shutdown, which printed an AttributeError after every conversion. It now has the same guard as CodeFormulaVlmModel.__del__.

Checklist:

• Documentation updated, necessary. The MlxVlmEngine docstring documents retry_repetition_penalty. No public API or option changes.
• Examples added, necessary. Not needed.
• Tests added, necessary.

Review evidence:

Source: arXiv 2206.01062 (9 pages), docling --pipeline vlm --vlm-model granite_docling --to md, MLX engine, macOS arm64.

	Before	After
Fully black embedded images	75 of 78	0 of 10
Page 5 after Figure 4: caption, "were carried out over a timeframe of 12 weeks", Phase 4 paragraph	missing	present
Error printed at exit	AttributeError: 'NoneType' object has no attribute 'warning'	none

Page 5 alone reproduces the lost text in about 7 s. The first generation emits 71 empty pictures and ends at 2320 tokens. The retry produces 1334 tokens with the missing text. Without the retry, a global repetition_penalty of 1.1 also fixed page 5 but changed table structure on other pages, which is why the retry applies only to a page that looped.

Tests:

• tests/test_vlm_pipeline_page_document.py (renamed from test_vlm_pipeline_doclang.py): crops a known red box through _finalize_page_document for a DocTags and a Chandra HTML response at images_scale 1.0 and 2.0, and checks the crop size and colour. At 2.0, the DocTags case fails with the old crop and the Chandra case fails with a crop that skips the scale.
• tests/test_mlx_vlm_engine.py: retry on a streamed loop, retry on a loop visible only in the finished output, no retry without the retry config, and a failing stopper that is skipped with and without the retry config.
• tests/test_vlm_convert_model.py: __del__ logs a cleanup failure, and ignores it when the module logger is already None.
• tests/test_generation_stopper.py: four cases for EmptyPictureRunStopper.
• pytest tests/test_vlm_pipeline_page_document.py tests/test_mlx_vlm_engine.py tests/test_vlm_convert_model.py tests/test_generation_stopper.py tests/test_vlm_pipeline_force_backend_text.py tests/test_vlm_pipeline_status.py tests/test_chandra_vlm.py tests/test_transformers_engine_stop_reason.py tests/test_vlm_presets_and_runtime_options.py: 160 passed, 1 skipped, after rebasing on main. The rebase keeps the temperature keyword from fix(mlx): pass the sampling temperature as temperature to mlx-vlm #4769 in the shared _generate path that both the first run and the retry use, and reports the stop reason that the stopper declares (feat(vlm): let a GenerationStopper declare a valid end of the output #4927). prek hooks pass on the changed files.

Not run: the full test suite, non-macOS platforms, and MLX models other than Granite-Docling.

The 12-picture threshold and the 1.1 penalty come from this one paper. A page with a real run of 12 or more empty pictures gets one penalized rerun.

The force_backend_text branch of _finalize_page_output applies a similar images_scale factor to text bboxes. I left it unchanged because I could not confirm it is wrong.

[github-actions]

✅ DCO Check Passed

Thanks @faysou, all your commits are properly signed off. 🎉

[mergify]

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

• title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

AI triage (advisory)

Checked commit 689728240a. The analysis read the diff only. It did not run code from this pull request.

This PR fixes DocTags picture crop coordinates in VlmPipeline. It adds EmptyPictureRunStopper with a retry mechanism in MlxVlmEngine and guards VlmConvertModel.__del__ during interpreter shutdown.

CI safety: no concern found. A maintainer must still approve CI.

3 related pull request(s) in the same code

PR	State	Signal	Assessment
#4901	open	changes the same code	related: PR #4901 normalizes stop reasons in vlm_convert_model.py, while this PR fixes __del__ cleanup and VLM generation logic.
#4927	merged	referenced as #4927	related: PR #4927 added is_valid_end to GenerationStopper, while this PR adds EmptyPictureRunStopper and MLX retry logic.
#4769	merged	referenced as #4769	related: PR #4769 corrected the temperature parameter for MlxVlmEngine, while this PR adds retry handling for looping generations.

_{The ai:* labels are advisory. They do not approve CI and they do not add tests:full. A new push runs this check again.}

[github-actions]

AI first review (advisory): no blocker found

This review covers the full diff at 21b203ad02. It read the code and did not run it.

I checked the picture crop coordinate calculation, the MLX repetition retry logic, the empty picture generation stopper, and the cleanup error guard. The main risk is an unnecessary retry on pages with many small consecutive pictures; verify that the 12-picture threshold in EmptyPictureRunStopper fits expected multi-panel figures.

_{A maintainer makes the merge decision. This review never approves the pull request.}

[ceberam]

Thanks @faysou for your PR! The three fixes are clearly explained, and the evidence table is useful. I reviewed your PR and compared it with current main (26ef35ac) on macOS arm64 (Python 3.14, mlx-vlm 0.7.6, mlx 0.30.3, Granite-Docling MLX).

The picture crop change (fix 1) fixes DocTags but breaks picture crops for the other VLM response formats (confirmed by test for Chandra, and by reading the parsers for the rest). That must be fixed before merge. The retry (fix 2) also has an unguarded error path. Details and minor points follow.

The crop change breaks picture crops for non-DocTags VLM formats

_finalize_page_output runs for every ResponseFormat, not only DocTags. Only DocTags gives bboxes in image pixels (load_from_doctags uses the image size as the page size). The other parsers (Chandra HTML, dots JSON, MinerU 2, DeepSeek-OCR, Nemotron Parse) create bboxes in page units (points) from the normalized model output. For those formats the old code (scaled(scale) then to_top_left_origin(page_height * scale)) was correct, and the new code is wrong whenever images_scale != 1. The CLI sets images_scale = 2 when images are exported, so this is the default path for --image-export-mode embedded.

Reproduction through the real entry point VlmPipeline._finalize_page_document with a Chandra HTML response (ResponseFormat.CHANDRA_HTML), a 612x792 pt page, and a red rectangle drawn at 20%–60% x and 20%–40% y of the page image. The response is <div data-label="Image" data-bbox="200 200 600 400"><img src="x" alt="fig"/></div>. The provenance bbox is the same on both branches (l=122.4 t=158.4 r=367.2 b=316.8, in points).

images_scale	main crop	PR head crop
1.0	245x159, red	245x159, red (same)
2.0	489x317, red (correct, 154524 red pixels)	245x159, almost all white (123 red pixels)

At 2.0 the PR head crops the top-left region at half the size, so the picture image is wrong. I did not get a black image here, because the crop box still falls inside the larger image. At images_scale = 1.0 there is no difference, so the problem only appears with the CLI export setting (images_scale = 2).

Why other formats are affected: I read the parsers. Chandra (data-bbox / 1000 * page size), MinerU 2 (* original_page_size), DeepSeek-OCR (pg_width / 1000), dots (pg_width / model_image_size.width) and Nemotron Parse v2 (clamped to original_page_size) all create the bbox in page units. I tested Chandra only. For the others I confirmed it by reading the code, not by running them. The old code (scaled(scale) and page_height * scale) was correct for these formats. Only DocTags uses the image size as the page size.

I also ran the three formats whose output I could build in a test (DocTags, Chandra HTML and DocLang) at both scales through _finalize_page_document, with the same red rectangle at 20%–60% x and 20%–40% y of a 200x400 pt page:

Format, images_scale	main	PR head crop change on main
DocTags 1.0	correct	correct
DocTags 2.0	wrong (320x320, no red: the black-picture bug)	correct
Chandra HTML 1.0	correct	correct
Chandra HTML 2.0	correct	wrong (80x80, no red)
DocLang 1.0	correct	correct
DocLang 2.0	correct	wrong (80x80, no red)

DocLang is also affected by the PR's crop change. main already maps DocLang boxes to page points (_scale_doclang_to_page, added after the PR base), so DocLang behaves like the other formats and DocTags is the odd one out. By the way, DocTags may be deprecated soon.

Please, fix the picture crop so it is correct for DocTags and for the other formats. Either way, please extend test_vlm_pipeline_picture_crops.py with at least one non-DocTags response (for example Chandra or DocLang) at images_scale = 2.0. With the current test, the change passes while the regression above exists.

_looped has no try/except around should_stop

_generate wraps stopper.should_stop(...) in try/except Exception and skips a failing stopper. _looped calls the same method without a guard, so a stopper that raises, together with retry_repetition_penalty, raises out of predict_batch. I reproduced this with a stub engine (ValueError: boom). The only stopper that is combined with the retry is the built-in EmptyPictureRunStopper, and it does not raise: I ran it on an empty string, 100,000 characters, 45,000 <picture> tags and a lone surrogate, and nothing failed. The key retry_repetition_penalty is set by the preset only, so a user stopper reaches _looped only when a user builds a custom spec with both. If the engine raises, VlmConvertModel logs the error and re-raises, the same as for any other engine failure, so it is not silent. A small guard that mirrors _generate would keep both paths consistent, and a test would be easy (test_failing_stopper_is_skipped with the retry config), so I suggest it.

Minor: retry_repetition_penalty placement.

I think your choice is the right one for placing retry_repetition_penaltyThe setting is a runtime generation setting, and VlmModelSpec.get_runtime_input_extra_config documents that such settings flow through the nested extra_generation_config of the engine override into VlmEngineInput.extra_generation_config, while load-time settings stay in EngineModelConfig.extra_config. Other presets already do this per engine, for example the Nemotron Parse presets set "extra_generation_config": {"repetition_penalty": 1.1, ...} for Transformers and vLLM. The only gap is that the key is not documented. Please add a short note where the preset sets it, or in the MlxVlmEngine docstring, that retry_repetition_penalty is read by the MLX engine only and replaces the engine-level penalty for the retry.

Minor: no need to create new test files

It may become unmanageable if we create several new test files for every bug. In addition, it helps review and locate issues if they are grouped with similar tests in a single file. I think there is a better home for every one of them, and no new file is needed.

PR test file	Better home	Why
test_vlm_pipeline_picture_crops.py	test_vlm_pipeline_doclang.py (a rename to a format-neutral name is fine)	It already tests the picture crop through _finalize_page_document at images_scale 1.0 and 2.0, with the same red-area check.
test_mlx_engine_repetition_retry.py	test_mlx_vlm_engine.py	It tests MlxVlmEngine.predict_batch with a fake stream_generate, the same pattern, and it has stopper tests.
test_vlm_convert_model_cleanup.py	test_vlm_convert_model.py	Same class, with the same VlmConvertModel.__new__ and stub engine setup.
New cases in test_generation_stopper.py	Already in the right place	The PR adds them to the existing stopper test file.

[faysou]

Thanks for the detailed review. Fixed in the latest push: DocTags pictures are cropped from the bbox as it is, and the other formats keep the scaled crop. The crop test now covers DocTags and Chandra HTML at images_scale 1.0 and 2.0. _looped now skips a failing stopper like _generate, with a test for the retry config. The MlxVlmEngine docstring documents retry_repetition_penalty. The new tests are now in the existing files, and test_vlm_pipeline_doclang.py is renamed to test_vlm_pipeline_page_document.py.