fix: jobs tests and repo config yaml#58
Merged
Merged
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:
📝 WalkthroughWalkthroughForces subprocess-only executor defaults, changes endpoint translation to read subprocess profiles from merged ChangesCPU-to-Subprocess Execution Translation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![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
🤖 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 `@services/core/jobs/tests/test_config.py`:
- Line 375: The variable default_executors is annotated with a bare list type;
change its annotation to a concrete element type (e.g. list[ExecutorConfig] or
list[dict] depending on the actual items) so the declaration becomes something
like default_executors: list[ExecutorConfig] = [...] (use the actual element
type used in the list), updating any necessary imports for the referenced type.
🪄 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: c0ada19c-5f89-4f68-888d-3c7f6f5f9d06
📒 Files selected for processing (6)
services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/app/test_helpers.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/config.pyservices/core/jobs/tests/conftest.pyservices/core/jobs/tests/test_config.pyservices/core/jobs/tests/test_jobs_api.py
Contributor
|
mikeknep
commented
May 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@services/core/jobs/tests/integration/test_jobs_secrets_access.py`:
- Around line 55-56: The test sets "entrypoint" as a string but the job endpoint
code expects executor.container.entrypoint to be a list (it builds command via
[*executor.container.entrypoint, *executor.container.command] in
jobs/endpoints.py), so update the test in test_jobs_secrets_access.py to use a
list form for entrypoint (e.g., ["entrypoint"]) to match list[str] and prevent
unintended unpacking; ensure any other fixtures in that test that provide
container.entrypoint follow the same list format.
In `@services/evaluator/tests/integration/tasks/test_results_handler.py`:
- Around line 80-81: The test sets "entrypoint" as a string but
ContainerSpec.entrypoint expects list[str], causing Pydantic validation to fail;
update the test data in test_results_handler.py so the entrypoint value is a
list (e.g., ["entrypoint"]) wherever the ContainerSpec/Job creation payload is
assembled (look for the dict literal with keys "entrypoint" and "command" in the
test), ensuring the test constructs a list[str] to match
ContainerSpec.entrypoint.
🪄 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: 00279bd4-5421-4d7c-9353-5707e30fb74a
📒 Files selected for processing (4)
services/core/jobs/tests/integration/test_jobs_auth_propagation.pyservices/core/jobs/tests/integration/test_jobs_secrets_access.pyservices/evaluator/tests/integration/tasks/conftest.pyservices/evaluator/tests/integration/tasks/test_results_handler.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/core/jobs/tests/test_config.py (1)
375-375:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing concrete type hint.
Line 375 still has bare
listassignment. Annotate aslist[KubernetesJobExecutionProfile | VolcanoJobExecutionProfile | SubprocessJobExecutionProfile]or import/use the common base type if one exists.Proposed fix
- default_executors = [ + default_executors: list[KubernetesJobExecutionProfile | VolcanoJobExecutionProfile | SubprocessJobExecutionProfile] = [As per coding guidelines, always prefer concrete type hints over string based ones.
🤖 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 `@services/core/jobs/tests/test_config.py` at line 375, The variable default_executors is currently untyped; add a concrete type annotation such as list[KubernetesJobExecutionProfile | VolcanoJobExecutionProfile | SubprocessJobExecutionProfile] (or the shared base type if one exists) to the default_executors declaration, and add any necessary imports for KubernetesJobExecutionProfile, VolcanoJobExecutionProfile, SubprocessJobExecutionProfile (or the base type) so the annotation resolves.
🤖 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.
Duplicate comments:
In `@services/core/jobs/tests/test_config.py`:
- Line 375: The variable default_executors is currently untyped; add a concrete
type annotation such as list[KubernetesJobExecutionProfile |
VolcanoJobExecutionProfile | SubprocessJobExecutionProfile] (or the shared base
type if one exists) to the default_executors declaration, and add any necessary
imports for KubernetesJobExecutionProfile, VolcanoJobExecutionProfile,
SubprocessJobExecutionProfile (or the base type) so the annotation resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cfe22434-2559-43fa-85b7-5d32c06c9c7d
📒 Files selected for processing (4)
services/core/jobs/tests/integration/test_jobs_auth_propagation.pyservices/core/jobs/tests/test_config.pyservices/evaluator/tests/integration/tasks/conftest.pyservices/evaluator/tests/integration/tasks/test_results_handler.py
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
Signed-off-by: Mike Knepper <mknepper@nvidia.com>
mikeknep
force-pushed
the
fix-job-executors/mknepper
branch
from
29002a3 to
8e656bd
Compare
benmccown
approved these changes
May 27, 2026
aray12
pushed a commit
that referenced
this pull request
May 28, 2026
* Only support subprocess backend for now Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Fix TEST_EXECUTOR Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Dedupe test helpers Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Fix remaining unit tests Signed-off-by: Mike Knepper <mknepper@nvidia.com> * More executor container fixes Signed-off-by: Mike Knepper <mknepper@nvidia.com> * entrypoint is a seq Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Drop superfluous type hint Signed-off-by: Mike Knepper <mknepper@nvidia.com> * And one more Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Ok seriously this is the last one Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Pivot per Sam's changes Signed-off-by: Mike Knepper <mknepper@nvidia.com> * Revert some more, closer to main Signed-off-by: Mike Knepper <mknepper@nvidia.com> --------- Signed-off-by: Mike Knepper <mknepper@nvidia.com> Signed-off-by: Alex Ray <alray@nvidia.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
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.
What was happening
If you set
NMP_CONFIG_FILE_PATH="packages/nmp_platform/config/local.yaml"and have docker running when you spin upnemo services run, the Jobs service registers the docker executor but not the subprocess executor, and all jobs you attempt to create try to go through the docker backed only to fail with an Image Not Found error (or, if you have a stale version ofnmp-cpu-taskslaying around, some other error since that image hasn't been rebuilt).What's here
This PR registers the subprocess executor on that YAML config so that its executors match the bundled YAML config that gets shipped in the wheel and that serves as the default when the env var is unset:
packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yamlLonger term
It'd be nice to consolidate these two YAML files into a single source of truth, but this is a larger effort.
Unit test cleanup
Fixes some tests failing on
mainby:Summary by CodeRabbit
Refactor
Tests