feat(relay)!: initialize static/dynamic plugins via native integration, remove opt-in plugin - #77915
Conversation
b49b4b6 to
7ebaa49
Compare
7ebaa49 to
905cae5
Compare
e7098ee to
bc90160
Compare
|
I tested this with a real
The e2e test works after I applied those changes locally. The issues are pre-existing before the PR, so either creating a separate PR for those fixes, or including the fixes in this PR works for me. |
#### Overview
Adds a minimal Python compatibility API that converts standard `[[plugins.dynamic]]` records from one explicit `plugins.toml` into the existing `DynamicPluginActivationSpec` objects accepted by `initialize_with_dynamic_plugins()`.
This unblocks Python applications that embed Relay without introducing the larger file-backed activation, lifecycle reconciliation, dynamic layering, or initialization redesign proposed for a later release. The new API is intentionally a temporary 0.7 surface:
```python
plugin_config_path = os.environ["NEMO_RELAY_PLUGINS_TOML"]
dynamic_plugins = plugin.load_dynamic_plugin_activation_specs(plugin_config_path)
activation = await plugin.initialize_with_dynamic_plugins({}, dynamic_plugins)
```
`NEMO_RELAY_PLUGINS_TOML` is an optional host-side convention in this example. Relay does not read the environment variable automatically; the embedding application resolves a path through its environment, command-line, or configuration system and passes that path to the helper.
- [X] I confirm this contribution is my own work, or I have the right to submit it under this project's license.
- [X] I searched existing issues and open pull requests, and this does not duplicate existing work.
A broader implementation exists in #684. This PR is a deliberately scoped 0.7 alternative that reuses the existing activation owner instead of introducing shared lifecycle and host-configuration infrastructure.
#### Details
##### Public API
Adds:
```python
def load_dynamic_plugin_activation_specs(
plugin_config_path: str | os.PathLike[str],
) -> list[DynamicPluginActivationSpec]: ...
```
The helper:
* Reads one explicitly selected `plugins.toml`.
* Parses every `[[plugins.dynamic]]` record in declaration order.
* Resolves relative manifest paths against the selected file.
* Reads `plugin.id` and `plugin.kind` from each manifest.
* Preserves the record's JSON-compatible `config`.
* Rejects malformed TOML, invalid record shapes, unsupported fields, invalid plugin identities, duplicate plugin IDs, and non-JSON configuration.
* Returns the existing activation-spec type without loading code.
The existing dynamic initializer now accepts a `Sequence` rather than only a `list`. This reflects its existing behavior and allows parser results, lists, and tuples to compose without casts.
##### Developer flow
```mermaid
flowchart LR
User["User selects a plugins.toml"] -->
Host["Embedding host resolves the path"]
Env["Optional NEMO_RELAY_PLUGINS_TOML"] --> Host
Host --> Helper["load_dynamic_plugin_activation_specs(path)"]
Helper --> Config["Read one explicit plugins.toml"]
Config --> Records["Parse [[plugins.dynamic]] records"]
Records --> Manifests["Resolve and read relay-plugin.toml manifests"]
Manifests --> Specs["Build DynamicPluginActivationSpec list"]
Specs --> Initialize["initialize_with_dynamic_plugins(config, specs)"]
Initialize --> Activation["Owned PluginHostActivation"]
Activation --> Runtime["Host retains activation while work is admitted"]
Runtime --> Close["await activation.close() during shutdown"]
```
##### Configuration behavior
The temporary dynamic path and existing static configuration path remain separate:
```mermaid
flowchart TB
subgraph Static["Existing static component resolution"]
UserConfig["User plugins.toml"] --> StaticLayering["User → project → system → programmatic overlay"]
ProjectConfig["Project .nemo-relay/plugins.toml"] --> StaticLayering
SystemConfig["System /etc/nemo-relay/plugins.toml"] --> StaticLayering
end
subgraph Dynamic["New 0.7 compatibility path"]
ExplicitPath["One explicit plugins.toml path"] --> DynamicParser["Parse [[plugins.dynamic]] only"]
DynamicParser --> DynamicSpecs["Explicit activation specs"]
end
StaticLayering --> HostInitializer["Existing dynamic host initializer"]
DynamicSpecs --> HostInitializer
HostInitializer --> OwnedHost["PluginHostActivation"]
```
The helper does not perform dynamic-plugin layering. It reads only the explicitly supplied file. Static `[[components]]` from that file are inherited only when the same file is also selected by Relay's normal static discovery.
Every dynamic declaration in the selected file becomes an activation spec. Passing those specs to `initialize_with_dynamic_plugins()` is explicit consent to load the referenced trusted native libraries or worker processes.
Python workers that require a lifecycle-managed `environment_ref` still require the existing explicit activation or CLI lifecycle path.
##### Intentional non-goals
This PR does not:
* Consolidate `initialize()` and `initialize_with_dynamic_plugins()`.
* Add a unified `initialize_from_plugins_toml()` API.
* Discover or merge dynamic records across user, project, and system layers.
* Read or reconcile `.dynamic-plugins.json`.
* Consult CLI enablement or tombstone state.
* Provision or attest Python worker environments.
* Change plugin enablement, install plugins, or execute package managers.
* Change Rust, Node.js, Go, FFI, or CLI behavior.
The helper is documented as a 0.7 compatibility surface and is expected to be deprecated after the unified file-backed initializer lands. Keeping the conversion behind one Relay API lets embedded hosts remove their TOML and manifest parsing now while keeping the future migration localized to one call site.
##### Documentation and validation
Updates the Python type stub, plugin-configuration guide, and 0.7 release notes. Tests cover relative and absolute manifest resolution, native and worker spec construction, config preservation, malformed records and TOML, missing manifests, duplicate IDs, and end-to-end native activation from a real `[[plugins.dynamic]]` record.
Validation completed:
* Focused parser and native-activation tests: `16 passed`.
* Ruff formatting and linting.
* `ty` type checking.
* Changed-file and repository-wide pre-commit suites.
* Cargo formatting, clippy, check, and dependency-policy checks.
* Python worker protobuf compatibility.
* Go formatting and vet.
* Node formatting and public docstring checks.
* Fern structure and strict broken-link validation.
The complete dynamic-host Python module was also attempted locally. Pre-existing tests inherited an invalid machine-level `/etc/nemo-relay/plugins.toml`, and sandboxed worker tests could not bind Unix sockets. The tests directly covering this change passed independently.
Breaking changes: none.
#### Where should the reviewer start?
Start with `python/nemo_relay/plugin.py`, specifically `load_dynamic_plugin_activation_specs()`.
The central design decision is that this helper performs only the missing file-to-activation-spec conversion. It deliberately reuses the existing dynamic initializer and owned activation lifetime rather than introducing another activation owner or pulling CLI lifecycle behavior into the Python binding.
Then review `python/tests/test_dynamic_plugin_host.py` for the standard TOML parsing, failure behavior, and end-to-end native activation coverage.
#### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
* Relates to #673
* Relates to #684
* Relates to [NousResearch/hermes-agent#77915](<NousResearch/hermes-agent#77915>)
## Summary by CodeRabbit
* **New Features**
* Added a Python compatibility helper for loading dynamic plugin activation specifications from a selected `plugins.toml` file.
* Supports manifest path resolution, ordered activation specifications, nested configuration, duplicate detection, and validation of plugin records and JSON values.
* Dynamic plugin initialization now accepts any ordered collection of activation specifications.
* **Documentation**
* Added guidance covering configuration resolution, explicit loading consent, supported behavior, limitations, and planned deprecation.
* **Tests**
* Expanded coverage for valid configurations, absolute and nested manifest paths, malformed files, duplicate IDs, and missing manifests.
## Summary by CodeRabbit
* **New Features**
* Added support for loading dynamic plugin activation settings from a selected `plugins.toml` file.
* Added validation for manifests, duplicate identifiers, malformed configuration, and invalid JSON values.
* Dynamic plugin initialization now accepts any ordered collection of activation specifications.
* **Documentation**
* Added configuration guidance, behavior details, limitations, compatibility notes, and planned deprecation information.
* **Tests**
* Added coverage for valid configurations, path resolution, nested settings, and common loading errors.
Authors:
- Bryan Bednarski (https://github.com/bbednarski9)
Approvers:
- Will Killian (https://github.com/willkill07)
- Maryam Najafian (https://github.com/mnajafian-nv)
URL: #694
623be67 to
08c76bb
Compare
Blocking: concurrent session close can deadlock the active asyncio loopI reproduced a hard deadlock at the current PR head ( The path is:
A four-way comparison isolated the interaction:
The sequential control completed in about 2 ms. The trigger is concurrent sessions sharing one Relay runtime, not normal sequential close. The per-session process-wide flush predates this PR, but the new awaitable bridge changes the failure mode into a hard deadlock. I think this needs to be fixed before merge. The narrow options are to remove the global subscriber flush from per-session close and perform it once during process/runtime shutdown after active operations drain, or make session close fully asynchronous and await it without blocking the event-loop thread. Separate non-regression found during the same lifecycle audit: one-shot execution hard-exits without emitting the root Relay session end. I filed that independently as #79471. |
|
Stable nemo-relay 0.7.0 is now published on PyPI. I updated the dependency constraint and uv.lock and opened a focused PR directly against this PR's head branch: bbednarski9#1 Validation: uv lock --check passes and all 18 Relay plugin-runtime tests pass against the stable package. |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Alex Fournier <afournier@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
bde2c09 to
6f7596e
Compare
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
Intentionally differed items from review:
@andrexibiza we have addressed your comments above, caught up to main, and CI is green. CC @afourniernv |
andrexibiza
left a comment
There was a problem hiding this comment.
Approval-style re-review of exact current head 0e5499b15300f10ad51a211ccf1c76824193feb7 against PR base b5455fdd16fe608214f91149233660e1836b067c and current main d21cd51d860454b921abf9f2fe1d5dfd4c071e33. This is a materially new head since my prior review at 00d108e075480fd3d22a3d03f5121cbb1998ef4d, so I rechecked the blocker, lifecycle ownership, tests, CI, and adjacent Relay work rather than repeating the old review.
Previous blocker is closed
The earlier problem was a state-collapse at the configuration load boundary: an unset HERMES_NEMO_RELAY_PLUGINS_TOML and an explicitly selected but missing/malformed/invalid configuration both became DISABLED. That is fixed structurally now.
_configured_plugin_inputs() returns None only when no non-empty config path was selected. Once a path is explicitly selected, load/parse/dynamic-spec failures raise _RelayPluginConfigurationLoadError; _ProcessRelayPluginConfiguration.acquire() catches that under the failure path and records FAILED. The process state now preserves the intended four-way policy:
- no explicit selection →
DISABLED; - explicit selection that cannot be loaded/validated/initialized →
FAILED; - pre-existing process-global Relay activation not owned by Hermes →
FOREIGN; - successful Hermes-owned activation →
ACTIVE.
The new tests close the exact acceptance edges I asked for: missing selected config is FAILED for the first and later hosted profile; malformed config is FAILED; an invalid plugins section is FAILED; legacy [[dynamic_plugins]] is rejected into FAILED; initialization failures stay FAILED for concurrent hosts; and only after the final failed owner exits may a later host retry and become ACTIVE. The old DISABLED/FAILED ambiguity is gone.
Lifecycle / other side of the shape
I also rechecked the process-global teardown boundary because that is the dangerous half of this cutover.
- @afourniernv's reproduced concurrent-session deadlock is addressed at the ownership layer, not papered over: per-session close no longer performs a process-wide subscriber flush. Final teardown waits for tracked operations to drain, then closes session scopes and flushes/clears the process plugin configuration once.
RelayOperationLeaseextends runtime/plugin lifetime across managed LLM streams and their deferred logical completion/cleanup, so teardown cannot invalidate the process-global activation while a stream still owns Relay work.- The two-profile tests prove one process activation with distinct profile/session scope stacks, no teardown after the first host exits, and teardown only after the final host. The real binding witness also proves ATOF sees both hosted profiles while ATIF trajectories remain separated by top-level Agent scope.
- @yczhang-nv's earlier full-agent integration findings are represented in the current implementation: canonical operation identity, removal of client-only timeout from the Relay wire payload, and tolerant handling of sparse optional response fields.
- Removing
plugins/observability/nemo_relaynow leaves one lifecycle/configuration owner instead of the previous core-plugin double ownership. That is the right class-level fix.
I do not see a new merge blocker on this head.
CI / merge state
Exact-head CI is now real and green: the main CI workflow completed successfully, as did Docker and Nix. The executed matrix includes blocking Ruff, Ruff+ty diff, E2E, Windows footguns, Windows-only tests, macOS-only tests, docs, attribution and supply-chain/OSV checks. GitHub currently reports the PR mergeable against main.
Interlocks / provenance
- NVIDIA/NeMo-Relay#694 is the upstream API/format dependency this consumes; complementary, not duplicated here.
- @poisdahl's deterministic Relay 0.6 cross-call-starvation regression remains valuable adjacent evidence and should retain attribution. It was explicitly dispositioned rather than absorbed into this PR. The production dependency floor is now
nemo-relay>=0.7.1,<0.8, so this cutover no longer claims compatibility with the 0.6 worker-pool behavior that regression demonstrates. - #79471 (one-shot root Relay session finalization) remains separate work rather than a reason to hold this lifecycle-owner cutover.
- A public non-secret Relay readiness/status surface remains a downstream/integration follow-up. The important prerequisite from this PR is now satisfied:
FAILEDandFOREIGNremain truthful process states instead of being collapsed intoDISABLED, so a future admission/readiness consumer can fail closed without reconstructing intent from logs.
Verdict: clear on exact head 0e5499b15300f10ad51a211ccf1c76824193feb7. I attempted to submit this as an APPROVE review, but the connected GitHub integration is not permitted to file an approval review on this repository; posting the same exact-head verification as a formal COMMENT review instead. A later head that changes Relay configuration classification, operation leases, shutdown/drain ordering, or the plugin cutover should be re-reviewed.
…ive-plugin-init feat(relay)!: initialize static/dynamic plugins via native integration, remove opt-in plugin
…ive-plugin-init feat(relay)!: initialize static/dynamic plugins via native integration, remove opt-in plugin
Warning
Breaking change — migration is required for existing NeMo Relay plugin users.
This PR removes the bundled Hermes
observability/nemo_relayplugin and movesplugin.initialize()lifecycle ownership into the coreRelayRuntime. Beforeupgrading, remove
observability/nemo_relay(or the legacynemo_relayalias)from
plugins.enabledand move exporter/middleware configuration into a Relayplugins.tomlselected withHERMES_NEMO_RELAY_PLUGINS_TOML.Legacy
HERMES_NEMO_RELAY_ATOF_*andHERMES_NEMO_RELAY_ATIF_*variables nolonger activate exporters. When
HERMES_NEMO_RELAY_PLUGINS_TOMLis unset,Hermes does not initialize Relay plugins and Relay performs no plugin discovery
or configuration layering. When it points to a valid file, Relay performs its
normal static discovery/layering and Hermes owns the resulting process-global
activation. If another host already has an active Relay plugin configuration,
Hermes leaves it unchanged and disables Hermes-managed Relay middleware rather
than replacing it.
Relay plugin policy is a process boundary. The selected static configuration,
discovered layers, middleware, exporters, guardrails, and dynamic plugins apply
to every profile hosted by that Hermes process. ATIF keeps top-level Agent
trajectories separate, while ATOF and other global subscribers observe events
from every hosted profile. Run profiles in separate Hermes processes when they
require different trust levels, plugin credentials, exporter destinations, or
guardrail policies.
Upgrade safeguards in this PR migrate stale
plugins.enabledentries out ofconfig v35, warn through
hermes doctorand runtime diagnostics, and rejectuser-installed copies of the removed plugin identity.
What does this PR do?
This PR adds explicitly configured NeMo Relay plugin activation to Hermes's native in-process Relay runtime and removes the overlapping bundled
observability/nemo_relayHermes plugin.The native SDK integrations remain in place: Hermes provides Relay session, turn, task, and manual scopes, shared-metrics lifecycle events, and SDK-managed LLM/tool adapter paths independently of whether plugin activation is enabled.
The native runtime calls neither
nemo_relay.plugin.initialize(...)norinitialize_with_dynamic_plugins(...)unlessHERMES_NEMO_RELAY_PLUGINS_TOMLis set to a non-empty path and the selected file loads successfully. With the variable unset, native Relay instrumentation continues normally, but Hermes does not activate Relay middleware, exporters, dynamic plugins, or ambientplugins.tomldiscovery.When explicitly enabled and the selected file loads successfully, Hermes passes its static components to Relay as a programmatic overlay. Relay performs its normal static
plugins.tomldiscovery and layering, with the selected static configuration taking precedence. Hermes delegates canonical[[plugins.dynamic]]manifest resolution and validation to Relay 0.7.1 before opening the first Relay session scope; dynamic records are loaded from the selected file only. The unreleased Hermes-specific top-level[[dynamic_plugins]]format and parser have been removed. If the selected file is missing, unreadable, malformed, contains an invalid dynamic declaration, or Relay rejects it, Hermes reports the failure and continues without native plugin activation. Because Hermes does not invoke Relay's initializer on that failure path, it does not fall back to ambient discovery.Initialization and activation are process-wide. The first hosted profile's plugin decision is represented explicitly as disabled, active, foreign, or failed, and additional profile-scoped Hermes hosts share that decision rather than retrying or selecting a different policy. Successful initialization emits
Relay plugins are active process-wide and apply to all profiles hosted by this Hermes process.exactly once. After the final host stops admitting work and active operations drain, Hermes closes sessions, flushes subscribers/exporters, and clears or closes the Relay plugin activation. Per-session close does not perform a global subscriber flush, avoiding a concurrent-session event-loop deadlock.Profile scopes preserve causal isolation inside that shared policy. ATIF produces separate trajectories for separate top-level Agent scopes, while ATOF and other global subscribers observe activity from every hosted profile. Static and dynamic middleware applies to calls from every profile, and worker invocations retain the invoking profile's Relay scope stack. Profiles requiring different trust, plugin credentials, exporter destinations, or guardrail policies must run in separate Hermes processes.
Removing
plugins/observability/nemo_relayestablishes a single lifecycle owner for Relay configuration. Deployments that previously enabled that Hermes plugin should remove it from their enabled-plugin configuration and express middleware/exporter configuration in the TOML selected byHERMES_NEMO_RELAY_PLUGINS_TOML. Hermes-native shared metrics do not require the removed plugin.This PR relies on the Relay
>=0.7.1,<0.8requirement already present onmain; it does not modifypyproject.tomloruv.lock.Because Relay 0.7.1 is the minimum supported version, the native runtime calls
initialize,initialize_with_dynamic_plugins,load_dynamic_plugin_activation_specs,flush_async, andclear_asyncdirectly. It retains fail-open handling for operational failures, but no longer carries Relay 0.6-era missing-method checks or synchronous cleanup fallbacks.Why remove
observability/nemo_relay?The bundled Hermes plugin and the native runtime were two independent owners of the same process-global Relay state. If both were enabled, each could initialize Relay from its own configuration and register middleware, subscribers, exporters, or dynamic plugins. That made behavior dependent on startup order and allowed several invalid or buggy configurations:
The native integration now provides the complete supported path: Hermes owns session and operation context, while Relay owns plugin parsing, activation, exporter/subscriber registration, and dynamic-plugin lifecycle. Removing the overlapping Hermes plugin gives that process-global state one owner and one teardown boundary. It does not remove Relay observability or shared metrics; deployments configure those components in the
plugins.tomlselected byHERMES_NEMO_RELAY_PLUGINS_TOML.Related Issue
Relates to NVIDIA/NeMo-Relay#694
Type of Change
observability/nemo_relayto coreRelayRuntimeownership)Changes Made
plugins.enabled;agent/relay_runtime.pyHERMES_NEMO_RELAY_PLUGINS_TOMLpath;[[plugins.dynamic]]records from the selected file only;[[dynamic_plugins]]format instead of maintaining a second parser;agent/relay_llm.pyagent/chat_completion_helpers.pyandagent/transports/chat_completions.pyplugins/observability/nemo_relay/scripts/toolperf_abevalHow to Test
Cutover guard validation:
166 passedacross the full config, doctor, and plugin-manager suites, plus31 passedin the focused Relay ownership/migration suite. Ruff and whitespace checks passed.Focused validation after removing the legacy plugin:
224 passed. The final canonical configuration and Relay 0.7.1 API tightening was additionally validated with217 passedacross the native Relay, nested-execution, shared-metrics, and smoke suites. After rebasing onto currentmain, all 12 Python CI slices and every required check passed.Latest process-policy follow-up:
26 passedin the focused plugin-ownership suite and208 passedacross Relay runtime, LLM, tool, and shared-metrics tests. Ruff and whitespace checks passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings)cli-config.yaml.exampleif I added/changed config keys — N/A; this uses an existing deployment environment variableCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AFor New Skills
Not applicable.
Screenshots / Logs