feat(cli): add plugin catalog core#618
Conversation
Code Review: PR #618 — feat(cli): add plugin catalog coreSummaryAdds a Overall the PR is well-structured, strictly validated, and thoughtfully tested. Comments below are mostly minor; one high-value item (entry point group constant usage) and a few smaller polish items. FindingsCorrectness
Schema validation
Security
Tests
Style / minor
VerdictApprove with minor requests. The architecture is sound, the schema validation is appropriately strict for a v2 contract freeze, and the test coverage is solid. Action items in priority order:
|
Greptile SummaryThis PR introduces the
|
| Filename | Overview |
|---|---|
| packages/data-designer/src/data_designer/cli/plugin_catalog.py | Schema v2 models, strict catalog validator, and shared dataclasses; deduplication keyed correctly by entry_point.name; specifier/marker cross-consistency enforced. |
| packages/data-designer/src/data_designer/cli/repositories/plugin_catalog_repository.py | Catalog alias persistence, URL normalization (including GitHub tree subdirectory fix), atomic PID-keyed cache writes, and stale-cache fallback are all correct. |
| packages/data-designer/src/data_designer/cli/services/plugin_install_service.py | uv/pip plan construction, active-project detection, Data Designer protection (excludes/constraints/no-install-package), and entry-point verification via importlib.metadata are correct. |
| packages/data-designer/src/data_designer/cli/controllers/plugin_catalog_controller.py | Compatibility gate, dry-run path, install/uninstall dispatch, and empty-list UX guidance are all implemented correctly. |
| packages/data-designer/src/data_designer/cli/services/plugin_catalog_service.py | Compatibility evaluation correctly merges the supplied python_version override into the default PEP 508 environment; alias resolution, grouping, and installed-plugin listing look correct. |
| packages/data-designer/src/data_designer/cli/commands/plugin.py | CLI surface; parent-catalog alias propagation and unused-catalog warnings are correctly implemented. |
| packages/data-designer/src/data_designer/cli/lazy_group.py | Adds parse_args override to restore no_args_is_help behavior for lazy-loaded subcommand groups; correct and non-breaking. |
| packages/data-designer/src/data_designer/cli/main.py | Wires plugin_app and plugin_catalog_app into the main CLI; plugin callback correctly captures --catalog for propagation to subcommands. |
Reviews (17): Last reviewed commit: "remove plugin catalog trust flag" | Re-trigger Greptile
4549bd7 to
3916648
Compare
|
Greptile follow-up addressed in 955a675. What changed:
Verified with:
|
| """Return a catalog entry by runtime plugin name or package name.""" | ||
| entries = self.list_entries(catalog_alias, refresh=refresh, include_incompatible=True) | ||
| canonical_name = canonicalize_name(name) | ||
| matches = [ |
There was a problem hiding this comment.
Separate from the casing issue already noted in the PR summary: this combines exact runtime-plugin matches and package-name matches, then picks by sort order. If package alpha exists and another package exposes runtime plugin alpha, plugins install alpha can select the package match instead of the exact runtime name. Could separate the two match sets and either prefer runtime names explicitly or raise an ambiguity error.
There was a problem hiding this comment.
Addressed in fb27046, and the CLI path is now package-only as well. plugins install, plugins info, and plugins uninstall resolve through get_package_entries() using a full package name or package alias, so runtime plugin names no longer provide an install affordance. I also tightened get_entry() itself to separate runtime-plugin matches from package-name matches and prefer exact runtime-plugin matches, with a regression test for the alpha collision.
|
While testing the plugin install flow, I hit one more install-safety issue. The plugin package installs successfully, but the resolver can also replace the Data Designer packages in the current environment. In my test, installing A simple approach for the and include it in the install plan: uv pip install \
--python <current-python> \
--excludes <protected-packages.txt> \
--default-index https://pypi.org/simple/ \
--index <catalog-index> \
<plugin-requirement>I tried that shape locally with For |
|
@eric-tramel thanks, this install-safety issue is now addressed in the latest pushed commits. What changed:
Coverage added/verified:
I did not add a separate pre/post mutation guard in code because the resolver commands now carry the protection directly, and pip has the equivalent constraints protection for the pip-only case. Happy to add an extra post-install belt-and-suspenders version check later if we decide the extra noise is worth it, but I don't think it should block this PR now. |
Add typed catalog and tap models, persistent tap storage, cached catalog loading, compatibility evaluation, install plan generation, and runtime plugin discovery helpers. Refs #617
Wire list, search, info, install, installed, and tap management commands through the existing command-controller CLI pattern. Refs #617
Add regression coverage for tap caching, catalog compatibility, installer command generation, local path resolution, and Typer command delegation. Refs #617
Validate tap catalogs against the schema v2 contract used by NVIDIA-NeMo/DataDesignerPlugins#36, including source union fields, docs URLs, package paths, compatibility metadata, and unique runtime plugin names. Derive Git install targets as package-qualified PEP 508 direct references so git tap entries install the package described by the catalog source metadata. Refs #617
- Invalidate import caches before post-install entry point verification - Make tap aliases case-insensitive and cache catalogs by alias plus URL - Prefer compatible catalog entries before falling back to forced installs - Clarify unused --tap behavior and list installed entry points without imports - Add direct controller coverage and update CLI plugin documentation Refs #617
Fetch install targets before compatibility filtering so the controller owns the final --force decision and the incompatible install guard stays reachable. Refs #617
Apply ruff formatting to the plugin command and tap repository tests so CI format checks pass on the PR merge commit. Refs #617
Key catalog duplicate detection by entry_point.name so distinct catalog entries cannot register the same runtime plugin name. Refs #617
- adopt catalog terminology for plugin source aliases - parse package-first plugin catalog metadata from the plugin repo - install package requirements with optional catalog indexes
8cda7f4 to
ff50a1e
Compare
ff50a1e to
13a1c9b
Compare
📋 Summary
This PR adds the client-side plugin package workflow for Data Designer. The new
data-designer plugincommand discovers package-first catalogs, shows compatibility-aware package metadata, installs and uninstalls plugin packages, and verifies the runtime plugin entry points exposed by those packages.The implementation follows the current
NVIDIA-NeMo/DataDesignerPluginscatalog contract and uses the stable NVIDIA catalog athttps://nvidia-nemo.github.io/DataDesignerPlugins/catalog/plugins.jsonas the built-in trusted source.🔗 Related Issue
Closes #617
🔄 Changes
Public CLI
data-designer plugincommand group withlist,search,info,install,uninstall, andinstalled.data-designer plugin catalog list/add/remove.data-designer-calculator, or its package alias, such ascalculator.Catalog Contract And Discovery
nvidiacatalog, user-managed catalog aliases inplugin_catalogs.yaml, andDATA_DESIGNER_DEFAULT_PLUGIN_CATALOG_URLfor QA/staging overrides.catalog/plugins.jsonlayout used by the plugin repo.Compatibility And Package Management
uvproject installs,uv pipenvironment installs, and pip-only environments.uv add --activewhen the user is in an active virtual environment with a userpyproject.toml, so plugin packages are recorded in the project.uvis unavailable; auto mode prefers supporteduvand falls back to pip when needed.data-designer,data-designer-config, anddata-designer-engine) from being reinstalled or upgraded as plugin package dependencies.Docs And Tests
packaginglockfile entry used by the new catalog/compatibility code.👀 Review Focus
plugin_catalog.py: schema v2 validation and the package-first compatibility boundary with DataDesignerPlugins.plugin_install_service.py: uv/pip command construction, active-project detection, Data Designer package-family protection, and post-mutation verification.plugin_catalog_repository.py: catalog alias persistence, source normalization, cache keying, and stale-cache behavior.plugin_catalog_controller.pyandcommands/plugin.py: user-facing package/runtime distinction, prompts, dry runs, and table output.🧪 Testing
Local verification:
uv run --package data-designer pytest packages/data-designer/tests/cli -q(690 passed,1 skipped,86 warnings)make format-checkmake lintgit diff --check145 checks)✅ Checklist
Description updated with AI