docs(py): clarify genkit.plugins namespace discovery and add test#5543
docs(py): clarify genkit.plugins namespace discovery and add test#5543cabljac wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the documentation in genkit/plugins/__init__.py to clarify how plugin namespace discovery works, and adds unit tests in plugins_discovery_test.py to verify the runtime discovery mechanism. Feedback is provided to improve test isolation in the test fixture by deleting the dynamically imported submodule attribute from the parent module, preventing potential test pollution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| sys.modules.pop(f'genkit.plugins.{FAKE_PLUGIN}', None) | ||
| importlib.invalidate_caches() |
There was a problem hiding this comment.
When a submodule is imported in Python, it is set as an attribute on its parent module (e.g., genkit.plugins.fake_discovery_plugin). Simply popping the submodule from sys.modules does not remove this attribute from the parent module. To ensure complete test isolation and prevent potential test pollution, we should also delete the attribute from genkit.plugins if it exists.
| sys.modules.pop(f'genkit.plugins.{FAKE_PLUGIN}', None) | |
| importlib.invalidate_caches() | |
| sys.modules.pop(f'genkit.plugins.{FAKE_PLUGIN}', None) | |
| if hasattr(genkit.plugins, FAKE_PLUGIN): | |
| delattr(genkit.plugins, FAKE_PLUGIN) | |
| importlib.invalidate_caches() |
2ab0d82 to
2985ddd
Compare
Reword the genkit.plugins package docstring to correctly explain why pkgutil.extend_path is not used here. extend_path searches the parent package's __path__ (genkit.__path__) rather than sys.path, and because genkit is a regular single-directory package its __path__ does not span the separately installed plugin distributions, so extend_path would find nothing. Runtime discovery via extend_plugin_namespace scans sys.path instead. Also clarify that genkit.plugins is a regular package (it ships __init__.py) whose __path__ is extended at runtime, not a strict PEP 420 namespace package. Add a regression test for extend_plugin_namespace: a fake plugin laid out on a temporary sys.path entry is discovered and importable, discovery is idempotent, and a sys.path entry without genkit/plugins is ignored.
2985ddd to
9a664cc
Compare
What
Reword the
genkit.pluginspackage docstring so it correctly explains ourplugin-discovery setup, and add a regression test for the runtime discovery that
nothing currently covers. Doc + test only, no behaviour change.
Why
In #5521 I'd added
pkgutil.extend_pathtogenkit/plugins/__init__.py, then removed it again once it turned out to be ano-op, leaving a comment so the next person doesn't re-add it. While reviewing, we
realised the comment's stated reason was wrong: it claimed
extend_pathonlypicks up portions that ship their own
genkit/plugins/__init__.py. Reading theCPython source, that isn't how it works (modern
extend_pathhandles PEP 420portions with no
__init__.py).The real reason
extend_pathis useless here:It only searches the parent package's
__path__.genkitis a regularsingle-directory package, so its
__path__doesn't span the separately installedplugin distributions, and there's nothing for
extend_pathto find. Our runtimeextend_plugin_namespace(ingenkit/_core/_plugins.py, called fromgenkit/__init__.py) scanssys.pathinstead, which does reach them.I've reworded the docstring to say that, and to clarify that
genkit.pluginsis aregular package (it ships
__init__.py) whose__path__is extended at runtime,rather than a strict PEP 420 namespace package.
Test
Adds
tests/genkit/core/plugins_discovery_test.pycoveringextend_plugin_namespace:sys.pathentry (as a bare PEP 420portion, matching how real plugins ship) is discovered and importable
__path__entries on repeat calls)sys.pathentry withoutgenkit/pluginsis ignoredThere was no test for this mechanism before. Run:
All three pass locally; ruff check/format clean.
Worth a careful look at
extend_pathvsextend_plugin_namespaceabove, in case I'vemisjudged the behaviour.
genkit.plugins.__path__andsys.path) is solid enough not to leak into other tests.