feat(plugin-api): export the configuration, build, and run surfaces - #2148
Open
DABH wants to merge 3 commits into
Open
feat(plugin-api): export the configuration, build, and run surfaces#2148DABH wants to merge 3 commits into
DABH wants to merge 3 commits into
Conversation
Third-party plugins that build workflows or drive runs (front ends, execution instrumentation, test harnesses) currently have to import implementation modules for that flow, contradicting the guidance that external plugin packages import only from nat.plugin_api. - Re-export Config, load_config, PluginTypes, discover_and_register_plugins, WorkflowBuilder, Runner, and ExporterManager through nat.plugin_api and extend __all__. - Add a public read-only entry_fn property to Workflow so callers can reach the entry Function without touching private attributes. - Pin the new exports in EXPECTED_PLUGIN_API_EXPORTS and add a consumer-style test that loads a config, builds it with WorkflowBuilder, reads workflow.entry_fn, and drives a run with Runner using facade imports alone. - Document the new provisional surface rows in docs/source/extend/plugin-api.md. Signed-off-by: David Hyde <DABH@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe plugin API now exports workflow, runtime, configuration, discovery, and exporter interfaces. ChangesPlugin API surface
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Public API test
participant Discovery as discover_and_register_plugins
participant Config as load_config
participant Builder as WorkflowBuilder
participant Workflow as Workflow
participant Runner
Test->>Discovery: discover configuration plugins
Test->>Config: load workflow configuration
Test->>Builder: build configured workflow
Builder-->>Workflow: return built workflow
Test->>Workflow: read entry_fn and exporter_manager
Test->>Runner: execute workflow
Runner->>Workflow: invoke entry function
Workflow-->>Runner: return transformed result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
DABH
marked this pull request as ready for review
August 5, 2026 22:25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nvidia_nat_core/src/nat/builder/workflow.py (1)
83-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Google-style
Returns:section.
Workflow.entry_fnreturns aFunction, but the public API docstring does not document the return value. Add aReturns:section.As per coding guidelines, “Provide Google-style docstrings for every public module, class, function and CLI command.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_core/src/nat/builder/workflow.py` around lines 83 - 89, Add a Google-style Returns section to the public Workflow.entry_fn docstring, documenting that it returns the workflow’s entry Function used to dispatch each run. Keep the existing read-only guidance unchanged.Source: Coding guidelines
packages/nvidia_nat_core/tests/nat/test_plugin_api.py (1)
550-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a type annotation for
tmp_path.Annotate
tmp_pathasPath. This keeps the new test compatible with the required type-checking standard.As per coding guidelines, “All public APIs require Python 3.11+ type hints on parameters and return values.” As per path instructions, “Python methods should use type hints for all parameters.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nvidia_nat_core/tests/nat/test_plugin_api.py` at line 550, Update the test_consumer_style_config_build_and_run function signature to annotate tmp_path with Path, using the existing pathlib import or adding it if needed; preserve the test’s behavior and annotate only the requested parameter.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/extend/plugin-api.md`:
- Line 107: Rewrite the possessive phrase in the “Workflow build and run” table
entry, replacing “the built workflow’s read-only exporter_manager property” with
“the read-only exporter_manager property of the built workflow” while preserving
the surrounding guidance.
---
Nitpick comments:
In `@packages/nvidia_nat_core/src/nat/builder/workflow.py`:
- Around line 83-89: Add a Google-style Returns section to the public
Workflow.entry_fn docstring, documenting that it returns the workflow’s entry
Function used to dispatch each run. Keep the existing read-only guidance
unchanged.
In `@packages/nvidia_nat_core/tests/nat/test_plugin_api.py`:
- Line 550: Update the test_consumer_style_config_build_and_run function
signature to annotate tmp_path with Path, using the existing pathlib import or
adding it if needed; preserve the test’s behavior and annotate only the
requested parameter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 422c5d0b-3754-4fcf-9e90-f48bc470ecd1
📒 Files selected for processing (4)
docs/source/extend/plugin-api.mdpackages/nvidia_nat_core/src/nat/builder/workflow.pypackages/nvidia_nat_core/src/nat/plugin_api/__init__.pypackages/nvidia_nat_core/tests/nat/test_plugin_api.py
…sive phrasing Address review feedback: add a Google-style Returns section to the Workflow.entry_fn docstring, annotate the tmp_path parameter of the consumer-style end-to-end test with Path, and rewrite the possessive phrase for the built workflow in the plugin API surface table. Signed-off-by: David Hyde <DABH@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Third-party plugin packages are asked to import plugin-authoring symbols only from the stable
nat.plugin_apifacade (perdocs/source/extend/third-party-plugins.md). However, plugins that programmatically load, build, or drive workflows — front ends, execution instrumentation, and test harnesses — currently have to import implementation modules for every step of that flow:Config(nat.data_models.config) plusload_config,PluginTypes, anddiscover_and_register_plugins(nat.runtime.loader) to discover installed plugins and produce a validated configuration object.WorkflowBuilder(nat.builder.workflow_builder) to build the configured workflow.Runner(nat.runtime.runner) andExporterManager(nat.observability.exporter_manager) to drive a run the same wayWorkflow.rundoes.On top of that, the built
Workflowkeeps its entry function private (self._entry_fn), so a caller that wants to construct aRunnerfor it, or introspect the workflow's input and output contracts, has to reach into a private attribute.This change is purely additive:
nat.plugin_apiand extend__all__.entry_fnproperty toWorkflowwith a docstring; internal call sites are unchanged.EXPECTED_PLUGIN_API_EXPORTS.nat.plugin_api, registers a function, loads a minimal config, builds it withWorkflowBuilder, readsworkflow.entry_fn, and drives a run withRunner, sourcing the exporter manager from the built workflow's public read-onlyexporter_managerproperty the same wayWorkflow.rundoes, so the demonstrated pattern preserves configured telemetry exporters.docs/source/extend/plugin-api.md: two new surface-review rows ("Configuration loading and plugin discovery" and "Workflow build and run", both proposed as provisional public, matching the promotion tier of the runtime context access row), a new public-surface bullet, and a reworkedWorkflowBuildernote in the private-modules section. The runtime context access row no longer lists exporter management as unpromoted, and the builder-type row no longer describes concrete builders as implementation details — it now points at the "Workflow build and run" row, which also tells callers constructing aRunnerdirectly to source the exporter manager from the built workflow.Workflow.runremains the simplest way to execute a built workflow.RunnerandExporterManagerare exported for callers that need to own the run scope — for example supplying their ownContextStateor instrumenting the runner lifecycle — andExporterManageris the declared type of both the built workflow's publicexporter_managerproperty andRunner's requiredexporter_managerparameter, so driving a run through the facade needs it importable.register_front_endand the front-end hosting contract remain deferred; this PR only promotes the objects needed to load a configuration, build it, and drive runs.No tracking issue exists for this yet; happy to file one if the team prefers.
Testing
uv run pytest packages/nvidia_nat_core/tests/nat/test_plugin_api.py— 8 passed (includes the new consumer-style test).uv run pytest packages/nvidia_nat_core/tests/nat/builder packages/nvidia_nat_core/tests/nat/runtime packages/nvidia_nat_core/tests/nat/middleware packages/nvidia_nat_core/tests/nat/observability— 1341 passed.uv run pytest packages/nvidia_nat_core/tests --ignore=packages/nvidia_nat_core/tests/eval/test_eval_callbacks.py --ignore=packages/nvidia_nat_core/tests/nat/finetuning/interfaces/test_trajectory_builder.py— 2585 passed, 236 skipped. The two ignored modules import the eval plugin extra (not installed locally) and fail to collect identically on the base branch.uv run pre-commit run yapf --files <touched files>anduv run pre-commit run ruff-check --files <touched files>,uv run python ci/scripts/copyright.py --verify-apache-v2,valeand the markdown-link-check hook on the touched Markdown file, anduv run python ci/scripts/path_checks.pyall pass.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests