feat(nemo-agents): stage Fabric agent.yaml artifacts into Docker/K8s deployments - #1007
Open
tylersbray wants to merge 3 commits into
Open
feat(nemo-agents): stage Fabric agent.yaml artifacts into Docker/K8s deployments#1007tylersbray wants to merge 3 commits into
tylersbray wants to merge 3 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFabric deployments now propagate agent identifiers, stage and validate agent filesets, and materialize multiple configuration files in Docker and Kubernetes. The CLI uploads specification filesets, rolls back failed uploads, and removes filesets during agent deletion. ChangesFabric deployment artifacts
Sequence Diagram(s)sequenceDiagram
participant CLI
participant NeMoPlatform
participant DeploymentsRunnerBackend
participant FabricArtifactStaging
participant DeploymentRuntime
CLI->>NeMoPlatform: Create Fabric agent
CLI->>NeMoPlatform: Upload agent specification fileset
DeploymentsRunnerBackend->>FabricArtifactStaging: Stage fileset and rewritten configuration
FabricArtifactStaging-->>DeploymentsRunnerBackend: Return resolved ConfigFile list
DeploymentsRunnerBackend->>DeploymentRuntime: Configure Docker or Kubernetes files
DeploymentRuntime-->>DeploymentsRunnerBackend: Materialize files and start agent
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py (1)
384-404: 🚀 Performance & Scalability | 🔵 TrivialMulti-file payload transported entirely via one env var.
All staged file contents are base64-encoded into a single JSON env var. Docker/OS environment-size limits could be hit for filesets with many or large skill/prompt files, causing container start failures that are hard to diagnose from the deployment status alone. Worth a sanity check on typical fileset sizes, or a size guard/clear error before this path is hit at scale.
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py` around lines 384 - 404, The multi-file branch in the Docker backend can exceed environment-size limits by placing the entire staged fileset in one `_STAGED_CONFIG_FILES_ENV` variable. Add a preflight size check before appending that env var, using the serialized base64 payload size and an appropriate documented limit; raise a clear deployment error when exceeded, while preserving the existing materialization flow for payloads within the limit.
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py`:
- Around line 247-256: Update _materialize_staged_config_files_and_exec to
invoke the staging inline script with python instead of python3, keeping the
existing exec {quoted_argv} behavior unchanged so Fabric Docker deployments
consistently use the available interpreter.
In
`@plugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.py`:
- Around line 96-102: Guard the file read in the staging loop around
path.read_text within the artifact staging function, handling UnicodeDecodeError
alongside the existing file-related exceptions. Convert it into a clear staging
error that identifies the unreadable file, while preserving the special
rewritten_agent_yaml path and normal UTF-8 content behavior.
- Around line 70-77: Ensure the fileset-missing fallback in the staging flow
still invokes _validate_referenced_skill_paths before returning the inline
agent.yaml configuration, so configured skills.paths entries fail deployment
immediately when unavailable. Add coverage in test_fabric_artifact_staging.py
for a missing fileset with skills.paths configured, asserting validation raises
or otherwise reports the missing skill rather than allowing staging to succeed.
---
Nitpick comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py`:
- Around line 384-404: The multi-file branch in the Docker backend can exceed
environment-size limits by placing the entire staged fileset in one
`_STAGED_CONFIG_FILES_ENV` variable. Add a preflight size check before appending
that env var, using the serialized base64 payload size and an appropriate
documented limit; raise a clear deployment error when exceeded, while preserving
the existing materialization flow for payloads within the limit.
🪄 Autofix (Beta)
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: db4c4926-279e-40cf-9861-3dea1c445f9e
📒 Files selected for processing (7)
plugins/nemo-agents/src/nemo_agents_plugin/runner/backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/in_memory.pyplugins/nemo-agents/tests/unit/test_fabric_artifact_staging.pyplugins/nemo-agents/tests/unit/test_runner_deployments.py
tylersbray
force-pushed
the
AIRCORE-966-fabric-artifact-staging/tbray
branch
from
July 31, 2026 00:10
1d5cdc2 to
e982a03
Compare
Contributor
|
Contributor
|
The existing changes look good to me, one thing I noticed is that we don't do the So for docker/k8s the below changes are for cli.py Createdef create(...):
config_dict = _load_yaml(agent_config)
config_format = config_dict.get(
"config_format",
NAT_WORKFLOW_CONFIG_FORMAT,
)
if config_format == NEMO_AGENTS_SPEC_CONFIG_FORMAT:
config_dict = _validate_platform_agent_config_for_cli(
config_dict,
base_dir=agent_config.parent,
)
elif config_format == NAT_WORKFLOW_CONFIG_FORMAT:
config_dict = inject_default_model(config_dict)
...
else:
typer.echo(f"Unsupported config format: {config_format}", err=True)
raise typer.Exit(code=1)
payload = {
"name": name,
"description": description,
"config": config_dict,
"config_format": config_format,
}
agent_created = False
try:
response = _api_request(
"POST",
base_url,
f"/apis/agents/v2/workspaces/{workspace}/agents",
json_body=payload,
)
agent_created = True
if config_format == NEMO_AGENTS_SPEC_CONFIG_FORMAT:
_upload_agent_spec_fileset(
agent_name=name,
workspace=workspace,
agent_root=agent_config.parent,
base_url=base_url,
)
except Exception:
if agent_created:
_delete_agent_and_spec_fileset(
agent_name=name,
workspace=workspace,
base_url=base_url,
)
raise
typer.echo(json.dumps(response, indent=2))Deletedef delete(...):
base_url = _resolve_base_url(base_url)
if not yes:
typer.confirm(f"Delete agent '{name}'?", abort=True)
_delete_agent_and_spec_fileset(
agent_name=name,
workspace=workspace,
base_url=base_url,
)
typer.echo(f"Agent '{name}' deleted.")Fileset Uploaddef _upload_agent_spec_fileset(...):
_upload_directory_to_fileset(
local_path=agent_root,
fileset_name=agent_spec_fileset_name(agent_name),
workspace=workspace,
base_url=base_url,
)Agent And Fileset Deletiondef _delete_agent_and_spec_fileset(...):
_api_request(
"DELETE",
base_url,
f"/apis/agents/v2/workspaces/{workspace}/agents/{agent_name}",
)
try:
_delete_fileset(
fileset_name=agent_spec_fileset_name(agent_name),
workspace=workspace,
base_url=base_url,
)
except FilesetNotFound:
pass |
tylersbray
added a commit
that referenced
this pull request
Jul 31, 2026
Pair Fabric agent create/delete with the conventional {agent}-spec
fileset so Docker/K8s staging has artifacts to download. Roll back the
agent entity if upload fails; best-effort delete the fileset on agent
delete. Closes the producer gap flagged on #1007.
Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
Author
|
Good catch — agreed this belonged with the consumer staging work. Landed on this branch: Fabric Out of scope for this PR (happy to file follow-ups if useful):
Thanks for the sketch — the create/delete pairing and rollback shape matched what we needed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
plugins/nemo-agents/src/nemo_agents_plugin/cli.py (1)
1512-1519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the concrete
NeMoPlatformtype.
_platform_sdkreturnsAny, so callers lose type checking on.files.filesets. ImportNeMoPlatformnormally and annotate the return type.♻️ Proposed refactor
-def _platform_sdk(base_url: str) -> Any: +def _platform_sdk(base_url: str) -> NeMoPlatform: """Return an auth-aware platform SDK client for fileset upload/delete.""" - from nemo_platform import NeMoPlatform - headers = _resolve_context_headers() if headers: return NeMoPlatform(base_url=base_url, default_headers=headers) return NeMoPlatform(base_url=base_url)Add the import at module level:
from nemo_platform import NeMoPlatformIf the lazy import exists to protect CLI startup latency, keep the local import and annotate with the concrete type instead.
As per coding guidelines: "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible."🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py` around lines 1512 - 1519, Update _platform_sdk to return the concrete NeMoPlatform type instead of Any, and make NeMoPlatform a normal module-level import when possible. If the lazy local import is required for startup behavior, retain it while annotating the function with the concrete type so callers receive type checking for .files.filesets.Source: Coding guidelines
plugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py (2)
104-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a non-
NotFoundErrorfileset failure.
cli.pytoleratesNotFoundErrorinside_delete_agent_spec_filesetand tolerates every other exception inside_delete_agent_and_spec_fileset. Only the first branch is tested. A regression that lets a generic fileset error fail the delete command stays undetected.💚 Proposed test
def test_delete_succeeds_when_fileset_delete_errors(self, app) -> None: def handler(req: httpx.Request) -> httpx.Response: assert req.method == "DELETE" return httpx.Response(204) filesets = MagicMock() filesets.delete.side_effect = RuntimeError("boom") sdk = MagicMock() sdk.files.filesets = filesets with ( _install_mock_transport(handler), patch(f"{_PATCH_PREFIX}._platform_sdk", return_value=sdk), ): result = runner.invoke(app, ["delete", "my-agent", "--yes", "--base-url", "http://test"]) assert result.exit_code == 0, result.output assert "deleted" in result.output.lower()🤖 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 `@plugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py` around lines 104 - 123, Add a unit test alongside test_delete_succeeds_when_fileset_already_absent covering a generic fileset deletion failure: configure filesets.delete to raise RuntimeError, invoke the same confirmed delete command with the mocked SDK and transport, then assert a successful exit and “deleted” in the output. Preserve the existing NotFoundError test unchanged.
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
_install_mock_transportin a shared test helper.
plugins/nemo-agents/tests/unitcontains multiple copies with divergent patch targets and signatures. Move the helper to a shared module, add typed parameters, and allow the patch target to be specified where needed.🤖 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 `@plugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py` around lines 34 - 42, Move _install_mock_transport from the individual unit test module into a shared helper under the unit test utilities, then update all callers to import and reuse it. Give the helper typed handler and patch-target parameters, defaulting the patch target to the existing httpx.Client path while allowing callers with different targets to override it; preserve the current MockTransport and real-client construction behavior.
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 1522-1542: Update _upload_agent_spec_fileset to stage only the
intended agent files from agent_root before calling upload_to_fileset, excluding
unrelated checkout content. Validate the staged files against the applicable
file-count and total-size limits, and abort before upload when either limit is
exceeded; preserve the existing fileset name, workspace, and SDK arguments.
In `@plugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py`:
- Around line 57-61: Update the test assertion around
mock_delete.assert_called_once_with to use unittest.mock.ANY for the base_url
argument instead of reading mock_delete.call_args.kwargs["base_url"], and add
ANY to the existing unittest.mock imports.
In `@plugins/nemo-agents/tests/unit/test_cli.py`:
- Around line 265-287: Extend the assertions in the CLI create test to verify
that the captured uploaded["sdk_base_url"] equals "http://test". Keep the
existing assertions and fake_upload capture behavior unchanged.
---
Nitpick comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 1512-1519: Update _platform_sdk to return the concrete
NeMoPlatform type instead of Any, and make NeMoPlatform a normal module-level
import when possible. If the lazy local import is required for startup behavior,
retain it while annotating the function with the concrete type so callers
receive type checking for .files.filesets.
In `@plugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py`:
- Around line 104-123: Add a unit test alongside
test_delete_succeeds_when_fileset_already_absent covering a generic fileset
deletion failure: configure filesets.delete to raise RuntimeError, invoke the
same confirmed delete command with the mocked SDK and transport, then assert a
successful exit and “deleted” in the output. Preserve the existing NotFoundError
test unchanged.
- Around line 34-42: Move _install_mock_transport from the individual unit test
module into a shared helper under the unit test utilities, then update all
callers to import and reuse it. Give the helper typed handler and patch-target
parameters, defaulting the patch target to the existing httpx.Client path while
allowing callers with different targets to override it; preserve the current
MockTransport and real-client construction behavior.
🪄 Autofix (Beta)
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: 6ddef55a-1b4c-4c1d-9f9b-4c4a3ec77ebf
📒 Files selected for processing (3)
plugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/tests/unit/test_cli.pyplugins/nemo-agents/tests/unit/test_cli_delete_undeploy.py
tylersbray
added a commit
that referenced
this pull request
Jul 31, 2026
Pair Fabric agent create/delete with the conventional {agent}-spec
fileset so Docker/K8s staging has artifacts to download. Roll back the
agent entity if upload fails; best-effort delete the fileset on agent
delete. Closes the producer gap flagged on #1007.
Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
AIRCORE-966-fabric-artifact-staging/tbray
branch
from
July 31, 2026 21:00
8a45419 to
2a54a1c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 733-762: Update the rollback exception branches around
_delete_agent_and_spec_fileset in the config_format upload flow to call
typer.echo(..., err=True) for both typer.Exit and generic Exception failures.
Clearly tell the user that rollback failed and the agent may still exist,
indicating manual cleanup may be required, while preserving the existing
logger.error/logger.exception behavior and final exit.
🪄 Autofix (Beta)
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: 58213727-0c31-4377-9d9b-559ad912581d
📒 Files selected for processing (11)
plugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/entities.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/in_memory.pyplugins/nemo-agents/tests/unit/test_cli.pyplugins/nemo-agents/tests/unit/test_cli_delete_undeploy.pyplugins/nemo-agents/tests/unit/test_fabric_artifact_staging.pyplugins/nemo-agents/tests/unit/test_runner_deployments.py
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/nemo-agents/src/nemo_agents_plugin/runner/controller.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/backend.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/in_memory.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.py
Comment on lines
+733
to
+762
| if config_format == NEMO_AGENTS_SPEC_CONFIG_FORMAT: | ||
| try: | ||
| _upload_agent_spec_fileset( | ||
| agent_name=name, | ||
| workspace=workspace, | ||
| agent_root=agent_config.parent, | ||
| base_url=base_url, | ||
| ) | ||
| except Exception as exc: | ||
| typer.echo( | ||
| f"Error: failed to upload agent spec fileset for {name!r}: {exc}", | ||
| err=True, | ||
| ) | ||
| try: | ||
| _delete_agent_and_spec_fileset( | ||
| agent_name=name, | ||
| workspace=workspace, | ||
| base_url=base_url, | ||
| ) | ||
| except typer.Exit: | ||
| logger.error( | ||
| "Failed to roll back agent %r after fileset upload failure", | ||
| name, | ||
| ) | ||
| except Exception: | ||
| logger.exception( | ||
| "Failed to roll back agent %r after fileset upload failure", | ||
| name, | ||
| ) | ||
| raise typer.Exit(code=1) from exc |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Surface rollback failures to the user.
When _upload_agent_spec_fileset fails, the code attempts to roll back the just-created agent through _delete_agent_and_spec_fileset. If that rollback itself fails (typer.Exit or another exception), the code only calls logger.error/logger.exception. It never calls typer.echo(err=True).
The user sees only the upload-failure message and exit code 1. The user does not learn that the agent may still exist on the platform. Add a typer.echo call in both except branches so the CLI tells the user when manual cleanup is needed.
🔧 Proposed fix
try:
_delete_agent_and_spec_fileset(
agent_name=name,
workspace=workspace,
base_url=base_url,
)
except typer.Exit:
logger.error(
"Failed to roll back agent %r after fileset upload failure",
name,
)
+ typer.echo(
+ f"Error: failed to roll back agent {name!r}; it may still exist on the platform.",
+ err=True,
+ )
except Exception:
logger.exception(
"Failed to roll back agent %r after fileset upload failure",
name,
)
+ typer.echo(
+ f"Error: failed to roll back agent {name!r}; it may still exist on the platform.",
+ err=True,
+ )
raise typer.Exit(code=1) from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if config_format == NEMO_AGENTS_SPEC_CONFIG_FORMAT: | |
| try: | |
| _upload_agent_spec_fileset( | |
| agent_name=name, | |
| workspace=workspace, | |
| agent_root=agent_config.parent, | |
| base_url=base_url, | |
| ) | |
| except Exception as exc: | |
| typer.echo( | |
| f"Error: failed to upload agent spec fileset for {name!r}: {exc}", | |
| err=True, | |
| ) | |
| try: | |
| _delete_agent_and_spec_fileset( | |
| agent_name=name, | |
| workspace=workspace, | |
| base_url=base_url, | |
| ) | |
| except typer.Exit: | |
| logger.error( | |
| "Failed to roll back agent %r after fileset upload failure", | |
| name, | |
| ) | |
| except Exception: | |
| logger.exception( | |
| "Failed to roll back agent %r after fileset upload failure", | |
| name, | |
| ) | |
| raise typer.Exit(code=1) from exc | |
| if config_format == NEMO_AGENTS_SPEC_CONFIG_FORMAT: | |
| try: | |
| _upload_agent_spec_fileset( | |
| agent_name=name, | |
| workspace=workspace, | |
| agent_root=agent_config.parent, | |
| base_url=base_url, | |
| ) | |
| except Exception as exc: | |
| typer.echo( | |
| f"Error: failed to upload agent spec fileset for {name!r}: {exc}", | |
| err=True, | |
| ) | |
| try: | |
| _delete_agent_and_spec_fileset( | |
| agent_name=name, | |
| workspace=workspace, | |
| base_url=base_url, | |
| ) | |
| except typer.Exit: | |
| logger.error( | |
| "Failed to roll back agent %r after fileset upload failure", | |
| name, | |
| ) | |
| typer.echo( | |
| f"Error: failed to roll back agent {name!r}; it may still exist on the platform.", | |
| err=True, | |
| ) | |
| except Exception: | |
| logger.exception( | |
| "Failed to roll back agent %r after fileset upload failure", | |
| name, | |
| ) | |
| typer.echo( | |
| f"Error: failed to roll back agent {name!r}; it may still exist on the platform.", | |
| err=True, | |
| ) | |
| raise typer.Exit(code=1) from exc |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 762-762: use jsonify instead of json.dumps for JSON output
Context: json.dumps(resp, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py` around lines 733 - 762,
Update the rollback exception branches around _delete_agent_and_spec_fileset in
the config_format upload flow to call typer.echo(..., err=True) for both
typer.Exit and generic Exception failures. Clearly tell the user that rollback
failed and the agent may still exist, indicating manual cleanup may be required,
while preserving the existing logger.error/logger.exception behavior and final
exit.
…deployments Download the agent-spec fileset at deploy time and materialize sibling files (skills, prompts, etc.) under /workspace so Fabric relative paths resolve in containers the same way they do locally. Closes AIRCORE-966. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Pair Fabric agent create/delete with the conventional {agent}-spec
fileset so Docker/K8s staging has artifacts to download. Roll back the
agent entity if upload fails; best-effort delete the fileset on agent
delete. Closes the producer gap flagged on #1007.
Signed-off-by: Tyler Bray <tbray@nvidia.com>
The spec fileset upload is recursive with no server-side filtering, so an agent.yaml sitting in a source checkout would ship the whole tree and only fail later when the ConfigMap/env payload is assembled. Check file count and total size up front against limits shared with the staging consumer. Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
AIRCORE-966-fabric-artifact-staging/tbray
branch
from
July 31, 2026 22:25
2a54a1c to
a62bb57
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-agents/tests/unit/test_cli.py (1)
264-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
_platform_sdkreceives the correctbase_url.
mock_sdk.return_valueis fixed regardless of the call arguments. The test does not verify that_upload_agent_spec_filesetpasses the correctbase_urlinto_platform_sdk. A regression that hardcodes or dropsbase_urlbefore calling_platform_sdkwould not fail this test.♻️ Proposed fix
assert uploaded["local_dir"] == tmp_path assert uploaded["fileset"] == "fabric-agent-spec" assert uploaded["workspace"] == "default" assert uploaded["sdk_base_url"] == "http://test" + mock_sdk.assert_called_once_with("http://test")🤖 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 `@plugins/nemo-agents/tests/unit/test_cli.py` around lines 264 - 293, Update the test around the patched cli._platform_sdk mock so it asserts the mock was called with base_url="http://test" during the create command. Keep the existing return-value setup and upload assertions, ensuring the test verifies the base_url propagated into _platform_sdk rather than only checking the SDK instance afterward.
🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py`:
- Around line 1577-1601: Add a user-visible typer.echo(..., err=True) call in
the exception handler of _delete_agent_and_spec_fileset, stating that the agent
was deleted but cleanup of its spec fileset failed and may require manual
removal. Keep the existing logger.warning behavior and successful deletion
output unchanged.
---
Nitpick comments:
In `@plugins/nemo-agents/tests/unit/test_cli.py`:
- Around line 264-293: Update the test around the patched cli._platform_sdk mock
so it asserts the mock was called with base_url="http://test" during the create
command. Keep the existing return-value setup and upload assertions, ensuring
the test verifies the base_url propagated into _platform_sdk rather than only
checking the SDK instance afterward.
🪄 Autofix (Beta)
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: d7a5198c-3e17-4c0a-a796-0be8f69b32e3
📒 Files selected for processing (11)
plugins/nemo-agents/src/nemo_agents_plugin/cli.pyplugins/nemo-agents/src/nemo_agents_plugin/entities.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/in_memory.pyplugins/nemo-agents/tests/unit/test_cli.pyplugins/nemo-agents/tests/unit/test_cli_delete_undeploy.pyplugins/nemo-agents/tests/unit/test_fabric_artifact_staging.pyplugins/nemo-agents/tests/unit/test_runner_deployments.py
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/nemo-agents/src/nemo_agents_plugin/runner/controller.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/backend.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/in_memory.py
- plugins/nemo-agents/src/nemo_agents_plugin/entities.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/fabric_artifact_staging.py
Comment on lines
+1577
to
+1601
| def _delete_agent_spec_fileset(*, agent_name: str, workspace: str, base_url: str) -> None: | ||
| """Best-effort delete of the conventional ``{agent}-spec`` fileset.""" | ||
| from nemo_platform import NotFoundError | ||
|
|
||
| fileset_name = agent_spec_fileset_name(agent_name) | ||
| try: | ||
| _platform_sdk(base_url).files.filesets.delete(name=fileset_name, workspace=workspace) | ||
| except NotFoundError: | ||
| logger.info("Agent spec fileset %s/%s already absent", workspace, fileset_name) | ||
|
|
||
|
|
||
| def _delete_agent_and_spec_fileset(*, agent_name: str, workspace: str, base_url: str) -> None: | ||
| """Delete the agent entity, then best-effort remove its spec fileset.""" | ||
| _api_request("DELETE", base_url, f"/apis/agents/v2/workspaces/{workspace}/agents/{agent_name}") | ||
| try: | ||
| _delete_agent_spec_fileset(agent_name=agent_name, workspace=workspace, base_url=base_url) | ||
| except Exception: | ||
| logger.warning( | ||
| "Agent %r deleted but failed to remove spec fileset %r", | ||
| agent_name, | ||
| agent_spec_fileset_name(agent_name), | ||
| exc_info=True, | ||
| ) | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fileset cleanup failure is silent to the CLI user.
In _delete_agent_and_spec_fileset, if _delete_agent_spec_fileset raises, the code only calls logger.warning (Line 1594). The delete command then still prints "Agent 'name' deleted." The user has no way to know the {agent}-spec fileset may be orphaned and needs manual cleanup.
Add a typer.echo(..., err=True) in the except branch, mirroring the visibility gap already flagged for the create-rollback path.
🔧 Proposed fix
try:
_delete_agent_spec_fileset(agent_name=agent_name, workspace=workspace, base_url=base_url)
except Exception:
logger.warning(
"Agent %r deleted but failed to remove spec fileset %r",
agent_name,
agent_spec_fileset_name(agent_name),
exc_info=True,
)
+ typer.echo(
+ f"Warning: agent {agent_name!r} deleted but failed to remove its spec fileset "
+ f"{agent_spec_fileset_name(agent_name)!r}; manual cleanup may be required.",
+ err=True,
+ )🤖 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 `@plugins/nemo-agents/src/nemo_agents_plugin/cli.py` around lines 1577 - 1601,
Add a user-visible typer.echo(..., err=True) call in the exception handler of
_delete_agent_and_spec_fileset, stating that the agent was deleted but cleanup
of its spec fileset failed and may require manual removal. Keep the existing
logger.warning behavior and successful deletion output unchanged.
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
2 participants
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.
Summary
{agent}-specfileset into multi-fileconfig_filesat Fabric Docker/K8s deploy time so relative paths (skills.paths, sibling prompt assets) resolve under/workspace.STAGED_CONFIG_FILES_B64_JSON; K8s continues to use ConfigMap multi-file mounts.agent.yamlwhen the fileset is missing; fail deploy early when referencedskills.pathsentries are absent after staging.Closes AIRCORE-966.
Test plan
uv run --frozen pytest plugins/nemo-agents/tests/unit/test_fabric_artifact_staging.py plugins/nemo-agents/tests/unit/test_runner_deployments.py -v(56 passed)uv run ruff check/uv run --frozen ty checkon runner packageSummary by CodeRabbit