Skip to content

fix(nemo-deployments): wrap AsyncEntitiesClient via client_from_platform in OpenShell backend - #1077

Merged
maxdubrinsky merged 2 commits into
mainfrom
mdubrinsky/aircore-977-openshell-backend-wraps-the-wrong-entities-client-every
Aug 5, 2026
Merged

fix(nemo-deployments): wrap AsyncEntitiesClient via client_from_platform in OpenShell backend#1077
maxdubrinsky merged 2 commits into
mainfrom
mdubrinsky/aircore-977-openshell-backend-wraps-the-wrong-entities-client-every

Conversation

@maxdubrinsky

@maxdubrinsky maxdubrinsky commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Every deployment to an openshell-backed executor failed on its first reconcile. Docker-backed deploys of the same config reached READY, isolating the defect to the openshell backend.

Two independent fixes, one commit each.

1. Wrong entities client wrapping (root cause, Fixes AIRCORE-977)

OpenShellDeploymentBackend.init() wrapped the SDK's generated AsyncEntitiesResource directly:

self._entities = NemoEntitiesClient(AsyncEntitiesResource(self._sdk))

NemoEntitiesClient.get() forwards a query_params kwarg that the generated resource does not accept, so _load_deployment_config died with:

TypeError: AsyncEntitiesResource.get_entity_by_name() got an unexpected keyword argument 'query_params'

The docker and k8s backends already do the correct thing: adapt the SDK with client_from_platform(self._sdk, AsyncEntitiesClient). This change mirrors them:

self._entities = NemoEntitiesClient(client_from_platform(self._sdk, AsyncEntitiesClient))

Regression test

Added test_load_deployment_config_wraps_an_entities_client_that_accepts_query_params, which drives the real contract instead of a hollow constructor assert: it builds the backend against a real AsyncNeMoPlatform SDK backed by an httpx MockTransport that returns 404, then calls _load_deployment_config. A 404 surfaces as NemoEntityNotFoundError, which is only reachable once get_entity_by_name(query_params=...) is accepted and the request actually goes out.

I verified it is genuinely adversarial: with the source fix reverted the test fails with the exact TypeError ... unexpected keyword argument 'query_params'; with the fix applied it passes. The openshell conftest was updated to patch client_from_platform instead of the removed AsyncEntitiesResource.

2. Broken install hint (separate commit)

The MissingBackendDependencyError hint in backend.py and policy.py told users to run uv sync --package nemo-deployments-plugin --extra openshell, which narrows the workspace venv and uninstalls the platform (AIRCORE-980). Both copies now use the non-destructive form:

uv pip install "openshell>=0.0.92" "grpcio>=1.78.0" "protobuf>=6.31.1"

Testing

  • make test-deployments-openshell (installs the openshell extra, runs the backend unit suite): 79 passed.
  • Adversarial check: new test fails pre-fix with the query_params TypeError, passes post-fix.
  • uv run ruff format / uv run ruff check on all changed files: clean.
  • uv run --frozen ty check plugins/nemo-deployments: no new diagnostics in changed files. The 17 pre-existing diagnostics are unrelated (unused # ty: ignore[unresolved-import] on the openshell imports, which only fire when the openshell extra is installed, plus unrelated pre-existing test typing issues).

Fixes AIRCORE-977
https://linear.app/nvidia/issue/AIRCORE-977

Summary by CodeRabbit

  • Bug Fixes

    • Improved OpenShell deployment configuration loading when requests include query parameters.
    • Deployment-not-found responses are now reported with the appropriate error.
    • Updated OpenShell client setup for improved compatibility.
  • Documentation

    • Updated installation guidance to use uv pip install with the required OpenShell, gRPC, and Protocol Buffers packages.

…orm in OpenShell backend

OpenShellDeploymentBackend.init() wrapped the SDK's generated AsyncEntitiesResource
directly instead of adapting it with client_from_platform(sdk, AsyncEntitiesClient),
as the docker and k8s backends do. NemoEntitiesClient.get() forwards a query_params
kwarg the generated resource does not accept, so _load_deployment_config raised
TypeError: AsyncEntitiesResource.get_entity_by_name() got an unexpected keyword
argument 'query_params' and every first reconcile against an openshell executor failed.

Adapt the SDK the same way as the docker backend so the wrapped client accepts
query_params. Add a regression test that drives the real contract (a live entities
client over a mock transport): a 404 surfaces as NemoEntityNotFoundError, which is
only reachable once get_entity_by_name(query_params=...) is accepted. The test fails
against the old wrapping with the exact query_params TypeError. Update the openshell
conftest to patch client_from_platform instead of the removed AsyncEntitiesResource.

Fixes AIRCORE-977

Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
The MissingBackendDependencyError hint told users to run
uv sync --package nemo-deployments-plugin --extra openshell, which narrows the
workspace venv and uninstalls the platform (AIRCORE-980). Replace it in both the
backend and policy modules with the non-destructive
uv pip install "openshell>=0.0.92" "grpcio>=1.78.0" "protobuf>=6.31.1".

Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
@github-actions github-actions Bot added the fix label Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 30257/38273 79.1% 63.7%
Integration Tests 17897/36942 48.4% 20.9%

@maxdubrinsky
maxdubrinsky marked this pull request as ready for review August 5, 2026 15:02
@maxdubrinsky
maxdubrinsky requested review from a team as code owners August 5, 2026 15:02
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: acb622e2-3fda-443f-a0e4-f20c91ed750c

📥 Commits

Reviewing files that changed from the base of the PR and between 997172c and 9957729.

📒 Files selected for processing (4)
  • plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py
  • plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/policy.py
  • plugins/nemo-deployments/tests/unit/backends/openshell/conftest.py
  • plugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py

📝 Walkthrough

Walkthrough

The OpenShell backend now creates its entities client through client_from_platform. Installation guidance lists explicit package minimums. Tests patch the new factory and verify that missing deployment configurations raise NemoEntityNotFoundError.

Changes

OpenShell client migration

Layer / File(s) Summary
Entities client construction and installation guidance
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py, plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/policy.py
The backend uses client_from_platform(self._sdk, AsyncEntitiesClient). Installation hints now use uv pip install with minimum versions for openshell, grpcio, and protobuf.
Regression coverage for entities-client errors
plugins/nemo-deployments/tests/unit/backends/openshell/conftest.py, plugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py
Fixtures patch client_from_platform. A mocked transport test verifies that a missing deployment configuration raises NemoEntityNotFoundError.

Possibly related PRs

Suggested reviewers: svvarom, tylersbray

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary OpenShell backend fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mdubrinsky/aircore-977-openshell-backend-wraps-the-wrong-entities-client-every

Comment @coderabbitai help to get the list of available commands.

@maxdubrinsky
maxdubrinsky added this pull request to the merge queue Aug 5, 2026
Merged via the queue into main with commit 795bba2 Aug 5, 2026
54 checks passed
@maxdubrinsky
maxdubrinsky deleted the mdubrinsky/aircore-977-openshell-backend-wraps-the-wrong-entities-client-every branch August 5, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants