Skip to content

fix: make external plugin discovery explicit - #3878

Closed
M-Hassan-Raza wants to merge 21 commits into
docling-project:mainfrom
M-Hassan-Raza:fix/plugin-discovery
Closed

M-Hassan-Raza wants to merge 21 commits into
docling-project:mainfrom
M-Hassan-Raza:fix/plugin-discovery

Conversation

@M-Hassan-Raza

Copy link
Copy Markdown
Contributor

The CLI was discovering plugins during import, before it knew whether --allow-external-plugins was set. That is what caused the misleading disabled-plugin warning in #3284. This moves discovery into one cached registry and filters external entry points before importing their code.

The registry is shared by all four model factories, rejects ambiguous entry points and invalid hooks with useful errors, and keeps the existing factory override path intact. --show-external-plugins now also includes picture-description models and the package that provided each plugin. Pluggy is removed since this path no longer uses it.

Validation: branch hooks pass, along with the focused plugin, CLI, factory, options, and table tests. A full pytest run completed with 1,258 passing; the only two failures were tesserocr checks because this machine has no Tesseract language data installed.

Fixes #3284
Addresses #1444

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

Thanks @M-Hassan-Raza, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

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

@dolfim-ibm
dolfim-ibm requested review from cau-git and dolfim-ibm July 29, 2026 08:55
@codecov

codecov Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.16867% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
docling/models/factories/base_factory.py 89.24% 10 Missing ⚠️
docling/models/factories/plugin_registry.py 94.54% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@vku-ibm

vku-ibm commented Jul 29, 2026

Copy link
Copy Markdown
Member

@M-Hassan-Raza just to clarify - previously, a misc-configured plugin would cause only warning in the logs, with these change it becomes a hard stop. Is that the intended behavior? It might brake existing pipelines.

@M-Hassan-Raza

Copy link
Copy Markdown
Contributor Author

You’re right, that behavior change wasn’t intentional. The validation was only meant to make bad configs easier to diagnose. I’ll update it to warn and skip the plugin, while leaving import and hook execution errors as they were.

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@vku-ibm

vku-ibm commented Jul 30, 2026

Copy link
Copy Markdown
Member

@M-Hassan-Raza just for clarity, PluginConfigurationError is now non-braking but PluginHookError will still cause crash, are they treated separately deliberately or should they behave the same for consistency?

@M-Hassan-Raza

Copy link
Copy Markdown
Contributor Author

@M-Hassan-Raza just for clarity, PluginConfigurationError is now non-braking but PluginHookError will still cause crash, are they treated separately deliberately or should they behave the same for consistency?

Thanks sm for taking a detailed look at my PR and for this back and forth! Yes, they’re separate deliberately. Configuration errors are recoverable and were previously logged, so the plugin is skipped. A hook error means the plugin code raised while loading, which already stopped discovery before this PR. I kept that behavior to avoid hiding runtime failures.

Would you prefer it if I made soem changes? Perhaps try and make them consistent? I am open to anything you suggest :)

@dolfim-ibm

Copy link
Copy Markdown
Member

for this I would like also like review from @dolfim-ibm and @cau-git

@M-Hassan-Raza

Copy link
Copy Markdown
Contributor Author

for this I would like also like review from @dolfim-ibm and @cau-git

Great! I'd love to take in any feedback and get this PR to a mergeable state. Nor rush here as I understand the team is busy with various things!

@dolfim-ibm

Copy link
Copy Markdown
Member

Thanks a lot @M-Hassan-Raza for the work here.

#4413 has since landed on main and covers the core of this PR: external entry points are now filtered by module before they are imported. With that change and the CLI changes made since, this branch would need a large rebase, and the registry rewrite (dropping pluggy, the stricter validation) is a bigger change than we want for this fix. I'm closing it as superseded.

A few pieces from here are still missing on main, and a small focused PR for them would be very welcome:

  • the CLI still builds the OCR/layout/table factories with allow_external_plugins=False at import time, so the "will not be loaded" warning shows up even when --allow-external-plugins is passed
  • --show-external-plugins doesn't list picture-description plugins (--show-external-plugins won't #1444)
  • the "Package" column could show the distribution name instead of the top-level module

@M-Hassan-Raza

Copy link
Copy Markdown
Contributor Author

@dolfim-ibm Thank you for taking another look and suggesting a future PR. I took some time and opened one here based on your guidance: #4676

please take a look when you get the time!

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.

--allow-external-plugins not working with suryaocr

3 participants