Repository navigation
fix: make external plugin discovery explicit - #3878
M-Hassan-Raza wants to merge 21 commits into
Conversation
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>
|
✅ DCO Check Passed Thanks @M-Hassan-Raza, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@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. |
|
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>
|
@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 :) |
|
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! |
|
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:
|
|
@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! |
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