Skip to content

fix: skip picture description model init when disabled#3052

Closed
Br1an67 wants to merge 1 commit intodocling-project:mainfrom
Br1an67:fix/issue-2515-skip-vlm-init
Closed

fix: skip picture description model init when disabled#3052
Br1an67 wants to merge 1 commit intodocling-project:mainfrom
Br1an67:fix/issue-2515-skip-vlm-init

Conversation

@Br1an67
Copy link
Contributor

@Br1an67 Br1an67 commented Mar 1, 2026

Issue resolved by this Pull Request:
Resolves #2515

When do_picture_description is False, ConvertPipeline.__init__ no longer calls the picture description model factory. This prevents RuntimeError when optional VLM dependencies are not installed, which is common for SimplePipeline use cases (DOCX, HTML, PPTX conversions).

Previously, the factory was always invoked regardless of the flag, and would fail with No class found with the name 'vlm' if the VLM plugin was not registered.

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

When do_picture_description is False, ConvertPipeline.__init__ no longer
calls the picture description model factory. This prevents RuntimeError
when optional VLM dependencies are not installed, which is common for
SimplePipeline use cases (DOCX, HTML, PPTX conversions).

Previously, the factory was always called regardless of the flag, and
would fail with 'No class found' if the VLM plugin was not registered.

Signed-off-by: Br1an67 <932039080@qq.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2026

DCO Check Passed

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

@mergify
Copy link

mergify bot commented Mar 1, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

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)(?:\(.+\))?(!)?:

@dosubot
Copy link

dosubot bot commented Mar 1, 2026

Related Documentation

Checked 16 published document(s) in 1 knowledge base(s). No updates required.

How did I do? Any feedback?  Join Discord

@dolfim-ibm
Copy link
Member

@Br1an67 which dependency is missing? I think we should have lazy loading for all the model inference engines.

@dolfim-ibm dolfim-ibm self-requested a review March 2, 2026 11:43
@Br1an67
Copy link
Contributor Author

Br1an67 commented Mar 2, 2026

Good point — I traced through the current code and you're right: the plugin registration in defaults.py imports the model classes without pulling in heavy VLM dependencies, and PictureDescriptionVlmEngineModel.__init__ already guards engine creation behind if self.enabled:. So with the current plugin system (introduced in #2919), the factory + enabled=False path should work without errors.

The original issue was reported on v2.58.0, before the pluggable VLM runtime system existed. The error (No class found with the name 'vlm', known classes are: with an empty list) suggests the factory had no registered classes at that point.

That said, this change still provides a small benefit: when do_picture_description=False, it avoids the factory lookup, plugin loading, and model instantiation entirely — which is arguably the expected behavior for a disabled feature. But I understand if you'd prefer to close this as already addressed by the engine refactor. Happy to adjust the approach if you have a different direction in mind.

@Br1an67
Copy link
Contributor Author

Br1an67 commented Mar 2, 2026

Apologies, I missed the second part of your comment about lazy loading.

Regarding the missing dependency — the original reporter was on v2.58.0, where the factory produced No class found with the name 'vlm', known classes are: (empty list). In the current codebase, the plugin system registers the classes fine and the if self.enabled: guard in PictureDescriptionVlmEngineModel.__init__ correctly skips engine creation. So strictly speaking, the specific error from #2515 is no longer reproducible on main.

On lazy loading for all model inference engines — I agree that's the right approach. Would you prefer I:

  1. Expand this PR to audit all model engines and ensure their __init__ consistently short-circuits when enabled=False (no imports, no downloads, no initialization), or
  2. Keep this PR as a pipeline-level guard and open a separate PR for the broader engine audit?

Happy to go either direction.

@dolfim-ibm
Copy link
Member

For the moment we should not change the approach of adding stages to the pipeline, given that they should have the proper lazy-loading in place. We also have an important WIP in #2952 which will rely on it.

I don't think #2515 is a valid issue anymore and should more likely be closed.

So I proposed you close this PR, and if you want to check that the __init__ consistently short-circuits on the enabled argument, go ahead in a clean PR.

@Br1an67
Copy link
Contributor Author

Br1an67 commented Mar 2, 2026

Thanks for the clear guidance. Closing this PR — agreed that #2515 is no longer valid with the current plugin system.

I'll look into auditing the __init__ short-circuit behavior across model engines and open a separate PR if I find any gaps.

@Br1an67 Br1an67 closed this Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docling fails to convert docx document

2 participants