feat(entities): bridge EntityClient onto NemoClient typed transport - #731
Conversation
|
|
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:
📝 WalkthroughWalkthroughThe PR introduces typed Entities DTOs, endpoints, and clients; rewires ChangesTyped Entities API
Shared client wiring
Consumer migration
Deployment runtime
Deployment validation
Possibly related PRs
Suggested labels: 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 |
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-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 150-167: The GPU allocation flow in the deployment method must
release reservations on every failure path. Wrap all allocation-dependent work,
including port lookup, image pulling, container-argument construction, and the
containers.run call, in try/finally; invoke release_gpu_allocation in finally
unless containers.run completed successfully, while preserving existing
GPUAllocationError handling and success 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: f3a0c3e2-5820-4c98-b7c9-61911f4c0d3b
📒 Files selected for processing (44)
packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-controller/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-entities/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-inference-middleware/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-platform-services/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/docs/CONTROLLER.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/__init__.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/endpoints.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/types.pypackages/nemo_platform_plugin/tests/entities/test_endpoints.pypackages/nmp_common/src/nmp/common/entities/client.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/entities/test_client.pypackages/nmp_testing/src/nmp/testing/client.pyplugins/example-plugin/src/nemo_example_plugin/middleware.pyplugins/example-plugin/tests/test_inference_middleware.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/tests/unit/test_runner_controller.pyplugins/nemo-agents/tests/unit/test_runner_deployments.pyplugins/nemo-auditor/src/nemo_auditor/jobs/audit.pyplugins/nemo-auditor/tests/test_audit_job.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/src/nemo_deployments_plugin/controller.pyplugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.pyplugins/nemo-deployments/tests/integration/backends/k8s/test_k8s_backend.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/integration/test_reconcile_k8s.pyplugins/nemo-deployments/tests/unit/backends/docker/conftest.pyplugins/nemo-deployments/tests/unit/backends/k8s/conftest.pyplugins/nemo-deployments/tests/unit/reconciler/test_controller.pyplugins/nemo-deployments/tests/unit/test_registry.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/result_persistence.pyplugins/nemo-evaluator/tests/test_result_persistence.pyplugins/nemo-insights/src/nemo_insights_plugin/controller.pyservices/core/models/src/nmp/core/models/controllers/backends/deployments_plugin/backend.pyservices/core/models/tests/integration/test_model_deployment_config_service_integration.pyservices/core/models/tests/integration/test_model_deployment_service_integration.pyservices/platform-seed/src/nmp/platform_seed/tasks/seed/run.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@packages/nemo_platform_plugin/src/nemo_platform_plugin/docs/INFERENCE_MIDDLEWARE.md`:
- Around line 154-159: Update get_middleware_config to replace the undefined sdk
reference with the SDK available on self when constructing the
NemoEntitiesClient, or explicitly add sdk as a parameter and use that parameter
consistently. Ensure the documentation example executes without a NameError.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.py`:
- Around line 680-681: Update the entity update-field construction around the
project assignment so it distinguishes an omitted project from an explicitly
cleared project. When EntityUpdate.project is None because the field was
cleared, include project with a null value in update_fields; retain omission
only when project was not provided.
- Around line 758-766: The delete_by_id flow around _client.get_entity_by_id and
delete_entity_by_name is not atomic and ignores the caller’s expected entity
type. Replace the lookup-plus-name-delete sequence with an ID-targeted delete
operation that enforces the supplied entity type, or ensure the delete request
validates the fetched entity ID/version and expected type before deletion.
🪄 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: 0e2910d4-6cc8-48c5-9f28-b6d661b63d70
📒 Files selected for processing (17)
packages/nemo_platform_plugin/src/nemo_platform_plugin/docs/INFERENCE_MIDDLEWARE.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/types.pypackages/nemo_platform_plugin/tests/test_entity_client.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/tests/unit/test_runner_controller.pyplugins/nemo-agents/tests/unit/test_runner_deployments.pyplugins/nemo-auditor/src/nemo_auditor/jobs/audit.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/controller.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py
- plugins/nemo-agents/tests/unit/test_runner_deployments.py
- packages/nmp_common/src/nmp/common/service/base.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/types.py
- plugins/nemo-auditor/src/nemo_auditor/jobs/audit.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py
- plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
2b6d59e to
6c6d5e1
Compare
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-deployments/src/nemo_deployments_plugin/backends/docker/backend.py`:
- Around line 188-189: Update the post-allocation flow in the Docker backend
around gpu_pool and gpu_ids so every operation before successful
primary-container creation, including port discovery, local-image lookup, and
run-argument construction, is enclosed in a cleanup boundary. Track whether the
primary container was created successfully and release the deployment key
through the existing GPU-pool cleanup path whenever any earlier step raises;
preserve the reservation only after successful creation.
🪄 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: f64ad43d-fdf3-40d1-bcf6-c9553db1bd8c
📒 Files selected for processing (44)
packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-controller/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-entities/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-inference-middleware/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-platform-services/SKILL.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/docs/CONTROLLER.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/docs/INFERENCE_MIDDLEWARE.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/__init__.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/base.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/endpoints.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/entities/types.pypackages/nemo_platform_plugin/tests/entities/test_endpoints.pypackages/nemo_platform_plugin/tests/test_entity_client.pypackages/nmp_common/src/nmp/common/entities/client.pypackages/nmp_common/src/nmp/common/service/base.pypackages/nmp_common/tests/entities/test_client.pypackages/nmp_common/tests/nmp_common/test_dependency_provider.pypackages/nmp_testing/src/nmp/testing/client.pyplugins/example-plugin/src/nemo_example_plugin/middleware.pyplugins/example-plugin/tests/test_inference_middleware.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/controller.pyplugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.pyplugins/nemo-agents/tests/unit/test_runner_controller.pyplugins/nemo-agents/tests/unit/test_runner_deployments.pyplugins/nemo-auditor/src/nemo_auditor/jobs/audit.pyplugins/nemo-auditor/tests/test_audit_job.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/controller.pyplugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.pyplugins/nemo-deployments/tests/integration/backends/k8s/test_k8s_backend.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/integration/test_reconcile_k8s.pyplugins/nemo-deployments/tests/unit/backends/docker/conftest.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_image_pull_auth.pyplugins/nemo-deployments/tests/unit/backends/k8s/conftest.pyplugins/nemo-deployments/tests/unit/reconciler/test_controller.pyplugins/nemo-deployments/tests/unit/test_registry.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/result_persistence.pyplugins/nemo-evaluator/tests/test_result_persistence.pyplugins/nemo-insights/src/nemo_insights_plugin/controller.py
🚧 Files skipped from review as they are similar to previous changes (34)
- plugins/example-plugin/src/nemo_example_plugin/middleware.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-controller/SKILL.md
- plugins/nemo-evaluator/tests/test_result_persistence.py
- packages/nmp_common/src/nmp/common/entities/client.py
- packages/nmp_common/tests/nmp_common/test_dependency_provider.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/endpoints.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/init.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py
- plugins/nemo-deployments/tests/unit/backends/k8s/conftest.py
- packages/nmp_testing/src/nmp/testing/client.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-entities/SKILL.md
- plugins/nemo-deployments/tests/unit/test_registry.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/deployments_backend.py
- packages/nmp_common/src/nmp/common/service/base.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/docs/CONTROLLER.md
- packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/types.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-platform-services/SKILL.md
- packages/nemo_platform_plugin/tests/entities/test_endpoints.py
- plugins/nemo-agents/tests/unit/test_runner_deployments.py
- plugins/nemo-deployments/tests/integration/test_reconcile_docker.py
- plugins/nemo-agents/src/nemo_agents_plugin/runner/controller.py
- plugins/example-plugin/tests/test_inference_middleware.py
- plugins/nemo-auditor/tests/test_audit_job.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/.agents/skills/plugin-inference-middleware/SKILL.md
- packages/nemo_platform_plugin/src/nemo_platform_plugin/entities/client.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/docs/INFERENCE_MIDDLEWARE.md
- plugins/nemo-insights/src/nemo_insights_plugin/controller.py
- plugins/nemo-deployments/tests/integration/backends/k8s/test_k8s_backend.py
- plugins/nemo-deployments/tests/integration/test_reconcile_k8s.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/result_persistence.py
- plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py
- plugins/nemo-deployments/tests/unit/reconciler/test_controller.py
- plugins/nemo-auditor/src/nemo_auditor/jobs/audit.py
Convert nemo_platform_plugin/entities.py into a package. The rich generic EntityClient moves verbatim to entities/base.py behind an __init__.py re-export shim, so the existing `from nemo_platform_plugin.entities import ...` surface (46 direct sites, 179 via the nmp_common wrapper) keeps working. Add the low-level NemoClient typed layer alongside it: - types.py: Entity / EntityCreateInput / EntityUpdate / DeleteResponse wire DTOs plus list and by-name query params (no nmp_common deps; NAME_PATTERN inlined for the python-re engine) - endpoints.py: the 6 entities-router endpoints as decorated stubs - client.py: EntitiesClient / AsyncEntitiesClient via the method() descriptor - tests/entities/test_endpoints.py: PreparedRequest shape tests No behavior change: EntityClient still rides the Stainless AsyncEntitiesResource transport. This lands the typed layer and package restructure; the transport re-point follows. Part of AIRCORE-875. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Swap the transport under the rich generic EntityClient from the Stainless AsyncEntitiesResource to the NemoClient-based AsyncEntitiesClient, keeping EntityClient's public API identical so its ~225 import sites are unchanged. - base.py: EntityClient now holds an AsyncEntitiesClient. The six methods call it and unwrap the response (.data() for single, .page() for list, whose metadata maps straight into PaginationInfo). Caught errors are now nemo_platform_plugin.client.errors.* (.detail/.body). The service-principal header check reads the client's headers, and close() closes the shared httpx transport. - as_service() re-expressed via NemoClient.with_options(headers=...). - 4 construction sites (service DI, nmp_testing create_test_client x3, platform-seed) build via client_from_platform(sdk, AsyncEntitiesClient). - test_client.py: rewire the transport-touching mocks to the typed client and the new Entity DTO; expected_db_version now travels in the request body. Drops the Stainless entities resource from the EntityClient path. Verified: entities suites green (103 + 91), hello-world integration green (41), ty 0 net-new (base.py 6 diagnostics -> 1 pre-existing), no circular import. Part of AIRCORE-875. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
…error type Two consumer breakages surfaced by the EntityClient re-point in CI: - EntityCreateInput / EntityUpdate carried a client-side name pattern, so a service creating an entity with an invalid name (e.g. secrets) raised a pydantic ValidationError internally instead of receiving the entities server's 422. Drop the pattern from the request DTOs so the server validates and returns 422, restoring pre-migration behavior (the secrets "422 not 500" test asserts exactly this). - EntityClient.list leaks the transport error on a 404, whose type changed from nemo_platform.NotFoundError to the client's NotFoundError. Repoint the two model-deployment integration test imports to nemo_platform_plugin.client.errors, matching the already-migrated secrets tests. Verified locally: the 8 previously-failing tests pass (6 models + 2 secrets); entities 103, models 43, secrets 37, hello-world 41 all green; ty neutral on the touched files; no production code catches a Stainless transport error around an entity_client.list call. Part of AIRCORE-875. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
count_by reads group_counts straight off the response envelope rather than going through page(), and paginated responses defer raise_for_status to page()/items()/pages(). Nothing raised on a non-2xx, so a 404, 400 or 401 came back as "Grouped counts not found in response", and a 5xx with a non-JSON body escaped as a raw JSONDecodeError instead of a client error. The pre-migration SDK path raised on status, so this was a regression introduced while porting count_by onto the typed client. Check the status explicitly before reading the body, and cover 404 and 400 with a regression test. Also refresh two doc examples the migration missed that still constructed a client the old way. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
get_middleware_config built its entity client from an `sdk` name the method never defines or receives, so the snippet raised NameError as written. The example was already broken before the typed-client migration, where it called NemoEntitiesClient() with no arguments at all. Mirror the working example-plugin instead: construct the client once in setup() via get_async_platform_sdk, then reuse it per request. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
6c6d5e1 to
2abb4d4
Compare
Summary
Bridges the Entities service onto the
NemoClienttyped HTTP client (part of AIRCORE-827), following the Files/Secrets/Jobs template. This is the first slice: the generic entities router (6 endpoints). The rich genericEntityClientkeeps its public API and now ridesAsyncEntitiesClientinstead of the StainlessAsyncEntitiesResource, so its ~225 import sites are untouched.Part of AIRCORE-875.
What's in here
Commit 1 — scaffold the typed client
nemo_platform_plugin/entities.py→entities/package: the richEntityClient/EntityBasemove verbatim toentities/base.pybehind a thin__init__.pyre-export shim, preservingfrom nemo_platform_plugin.entities import ...(46 direct sites + 179 via thenmp_commonwrapper).entities/types.py—Entity,EntityCreateInput,EntityUpdate,DeleteResponsewire DTOs plus list / by-name query params. Nonmp_commondeps;NAME_PATTERNinlined for the python-re engine.entities/endpoints.py— the 6 entities-router endpoints as decorated stubs.entities/client.py—EntitiesClient/AsyncEntitiesClient.tests/entities/test_endpoints.py—PreparedRequestshape tests.Commit 2 — re-point the transport
entities/base.py—EntityClientnow holds anAsyncEntitiesClient; the six methods unwrap the response (.data()for single,.page()for list, whose metadata maps straight intoPaginationInfo). Caught errors arenemo_platform_plugin.client.errors.*(.detail/.body). The service-principal header check reads the client's headers;close()closes the shared httpx transport.as_service()re-expressed viaNemoClient.with_options(headers=...).client_from_platform(sdk, AsyncEntitiesClient):nmp_commonservice DI,nmp_testingcreate_test_client(x3),platform-seed.tests/entities/test_client.py— transport-touching mocks rewired to the typed client and the newEntityDTO;expected_db_versionnow travels in the request body.Endpoints
Verification
nmp_commonentities tests pass.EntityClientcreate/get/list/update/delete throughclient_from_platform→AsyncEntitiesClient→ in-memory entities routes).services/core/entities474 tests pass.nmp_commonfull package 1035 tests pass.base.py6 diagnostics → 1 pre-existing); ruff clean; no circular import.Deferred (follow-ups on AIRCORE-875)
sdk.projects.*/sdk.workspaces.*, they need their own low-level endpoints plus consumer migration.schemas.pyatentities/types.py.sdk.entities.*consumer sweep, then drop the Stainless entities resource entirely.Summary by CodeRabbit
New Features
Documentation
Bug Fixes