feat(jobs): optional output_location param to set location for job artifacts#878
Open
nv-odrulea wants to merge 6 commits into
Open
feat(jobs): optional output_location param to set location for job artifacts#878nv-odrulea wants to merge 6 commits into
nv-odrulea wants to merge 6 commits into
Conversation
nv-odrulea
changed the title
nv-odrulea
force-pushed
the
od/jobs-output-fileset-subpath
branch
from
July 23, 2026 22:26
7947ddd to
599e5b4
Compare
Contributor
|
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:
📝 WalkthroughWalkthroughJob creation now accepts an optional validated ChangesOutput location lifecycle
Customizer RL API contracts
Deployment schema additions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/types.py (1)
222-233: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
output_locationunvalidated at the core API boundary.The plugin's
BaseJobRequest._validate_output_location(api_factory.py) rejects empty/whitespace,#subpaths, and/-qualified names — butCreatePlatformJobRequest.output_locationhere has no such validator. Sinceendpoints.create_jobaccepts this model directly, any caller bypassing the plugin can submit malformed values (empty string, subpaths, slash-qualified names) that only get "validated" indirectly via afiles.get_filesetlookup in the dispatcher, not via an explicit contract check.🛡️ Mirror the plugin's validation
from pydantic import BaseModel +from pydantic import field_validator class CreatePlatformJobRequest(BaseModel): ... output_location: Optional[str] = None + + `@field_validator`("output_location") + `@classmethod` + def _validate_output_location(cls, value: Optional[str]) -> Optional[str]: + if value is None: + return None + stripped = value.strip() + if not stripped: + raise ValueError("output_location must not be empty") + if "#" in stripped or "/" in stripped: + raise ValueError("output_location must be a bare fileset name") + return stripped🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/types.py` around lines 222 - 233, Update CreatePlatformJobRequest.output_location to apply the same explicit validation as BaseJobRequest._validate_output_location, rejecting empty or whitespace-only values, # subpaths, and slash-qualified names before endpoints.create_job accepts the request. Reuse the existing validator logic or shared validation symbol rather than relying on dispatcher fileset lookup.
🤖 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/src/nmp/core/jobs/app/dispatcher.py`:
- Around line 286-301: Update the output-location lookup in the dispatcher’s
fileset resolution block to also catch ClientPermissionDeniedError alongside
ClientNotFoundError. Translate the permission failure into the established
client-facing JobOutputLocationError, preserving the referenced fileset and
workspace context and chaining the original exception.
---
Outside diff comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/types.py`:
- Around line 222-233: Update CreatePlatformJobRequest.output_location to apply
the same explicit validation as BaseJobRequest._validate_output_location,
rejecting empty or whitespace-only values, # subpaths, and slash-qualified names
before endpoints.create_job accepts the request. Reuse the existing validator
logic or shared validation symbol rather than relying on dispatcher fileset
lookup.
🪄 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: 46383832-d606-41a3-9ded-4d3cc7c7869c
📒 Files selected for processing (8)
packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/api_factory.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/types.pypackages/nemo_platform_plugin/tests/test_output_location_validation.pyservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/app/dispatcher.pyservices/core/jobs/src/nmp/core/jobs/entities.pyservices/core/jobs/tests/test_dispatcher.pyservices/core/jobs/tests/test_jobs_api.py
Signed-off-by: Octavian Drulea <[email protected]>
Signed-off-by: Octavian Drulea <[email protected]>
Signed-off-by: Octavian Drulea <[email protected]>
nv-odrulea
force-pushed
the
od/jobs-output-fileset-subpath
branch
from
July 24, 2026 17:42
47bef7c to
f3aa1db
Compare
Signed-off-by: Octavian Drulea <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/nemo-customizer/openapi/openapi.yaml (2)
2365-2367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
output_locationships with no description or constraints.Bare
type: stringgives SDK/doc consumers no hint that the value must be an existing bare fileset name — no workspace prefix, no subpath, non-empty. Same shape at Lines 1328-1330 (AutomodelJobsJobRequest) and Lines 2836-2838 (UnslothJobsJobRequest), so the fix belongs on the shared upstream request model (Field(description=..., min_length=1, pattern=...)), then regenerate.🤖 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-customizer/openapi/openapi.yaml` around lines 2365 - 2367, The shared upstream request model for output_location must define it as a non-empty existing bare fileset name, excluding workspace prefixes and subpaths, with a descriptive schema description and validation constraints for minimum length and pattern. Update that model rather than only the generated OpenAPI entries, then regenerate the corresponding schemas used by AutomodelJobsJobRequest and UnslothJobsJobRequest.
2077-2100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
RlDPOTrainingnumeric hyperparameters are unconstrained.
learning_rate,weight_decay,adam_beta1,adam_beta2accept any number — negative LR orbeta > 1passes contract validation and only fails once training is underway. The siblingAutomodelOptimizerSpec(Lines 1442-1470) constrains all four. Add the bounds upstream on the RL plugin schema and regenerate.♻️ Expected generated shape after adding upstream bounds
learning_rate: type: number + exclusiveMinimum: 0.0 title: Learning Rate description: Peak learning rate. default: 0.0001 @@ weight_decay: type: number + minimum: 0.0 title: Weight Decay description: Weight decay coefficient. default: 0.01 @@ adam_beta1: type: number + exclusiveMaximum: 1.0 + minimum: 0.0 title: Adam Beta1 description: Adam beta1. default: 0.9 @@ adam_beta2: type: number + exclusiveMaximum: 1.0 + minimum: 0.0 title: Adam Beta2 description: Adam beta2. default: 0.999🤖 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-customizer/openapi/openapi.yaml` around lines 2077 - 2100, Constrain the numeric hyperparameters in the RlDPOTraining schema to match AutomodelOptimizerSpec: require learning_rate and weight_decay to be non-negative, and restrict adam_beta1 and adam_beta2 to the valid [0, 1] range. Add these bounds in the upstream schema source, then regenerate the OpenAPI output so the displayed RlDPOTraining properties include the corresponding minimum/maximum validation metadata.
🤖 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 `@openapi/ga/openapi.yaml`:
- Around line 9789-9791: Add schema validation to output_location by retaining
type: string, adding minLength: 1, and applying the existing request-validator
pattern for bare fileset values only. Do not encode fileset existence checks in
the OpenAPI schema; keep those server-side validations unchanged.
In `@openapi/openapi.yaml`:
- Around line 9789-9791: Update the output_location schema property in the
relevant OpenAPI definition to include the same minLength and pattern
constraints enforced by request validation, rejecting empty,
workspace-qualified, and subpath-containing values while preserving server-side
fileset existence validation.
---
Nitpick comments:
In `@plugins/nemo-customizer/openapi/openapi.yaml`:
- Around line 2365-2367: The shared upstream request model for output_location
must define it as a non-empty existing bare fileset name, excluding workspace
prefixes and subpaths, with a descriptive schema description and validation
constraints for minimum length and pattern. Update that model rather than only
the generated OpenAPI entries, then regenerate the corresponding schemas used by
AutomodelJobsJobRequest and UnslothJobsJobRequest.
- Around line 2077-2100: Constrain the numeric hyperparameters in the
RlDPOTraining schema to match AutomodelOptimizerSpec: require learning_rate and
weight_decay to be non-negative, and restrict adam_beta1 and adam_beta2 to the
valid [0, 1] range. Add these bounds in the upstream schema source, then
regenerate the OpenAPI output so the displayed RlDPOTraining properties include
the corresponding minimum/maximum validation metadata.
🪄 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: 49afd0fb-8db2-41f4-b016-020b1fa33e53
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/jobs/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/jobs/jobs.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/inference/container_executor_config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/inference/container_executor_config_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/jobs/job_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_jobs.pyis excluded by!sdk/**
📒 Files selected for processing (13)
docs/cli/reference.mdxopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/jobs/__init__.pyplugins/nemo-agents/openapi/openapi.yamlplugins/nemo-anonymizer/openapi/openapi.yamlplugins/nemo-auditor/openapi/openapi.yamlplugins/nemo-customizer/openapi/openapi.yamlplugins/nemo-data-designer/openapi/openapi.yamlplugins/nemo-deployments/openapi/openapi.yamlplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-safe-synthesizer/openapi/openapi.yaml
Signed-off-by: Octavian Drulea <[email protected]>
Signed-off-by: Octavian Drulea <[email protected]>
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
1 participant
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.
Optional
output_locationfor platform jobsAdds an optional
output_locationfield to job creation so a caller can nest a job'sartifacts (logs + results) under a caller-supplied fileset instead of the auto-created
throwaway
job-fileset-<name>. Backend only; no data migration.Behavior
output_locationunset → auto-createjob-fileset-<name>(unchanged).output_locationset → must be an existing fileset in the workspace; the job'sartifacts nest there via the existing inner path scheme (
results/<attempt_id>/,logs/job=<id>/). Missing fileset → 422 (never auto-created — caller owns the lifecycle).forward-compatible with a future
<fileset>#<subpath>form (no rename needed).Ownership & delete
output_locationis persisted on thePlatformJobentity(migration-free — the entity store stores non-base fields as a dynamic property bag).
output_location is None→ wholedelete_fileset; otherwise the caller's fileset is left intact. Symmetric with create,and consistent with "delete an entity → delete only the storage it owns."
output_location="job-fileset-<their-job>"can't trick delete into destroying their fileset.Validation
field_validator): rejects empty //(workspace-qualified) /#(subpath, reserved).JobOutputLocationError→ 422.Files
nemo_platform_plugin/jobs/api_factory.py— field + validator + create-forwardnemo_platform_plugin/jobs/types.py—CreatePlatformJobRequest.output_locationcore/jobs/.../entities.py—PlatformJob.output_location(persisted provenance)core/jobs/.../app/dispatcher.py— create: validate + persist; delete: own-vs-leavecore/jobs/.../api/v2/jobs/endpoints.py—JobOutputLocationError→ 422 mappingExplain it to me like I'm 5 yrs old
How
output_locationnesting works: the full journeyThe analogy:
qualified-grayis a shared filing cabinet. Every job gets its own drawer inside it (named after the job), and each drawer has two folders: results and logs. The catch: the results clerk and the logs clerk work very differently, so they learn "which drawer" in different ways.The 3 new props
output_locationlog_subpathPlatformJobStepWithContext)<job-name>/drawer." Only set ifoutput_locationwas set.subpath<job-name>drawer name used when saving/finding log files. Same value on both ends.output_location= the cabinet. The drawer name is always just the job's name — nobody passes it; the server already knows it.Route A — Creating the job (where the props get set)
{spec, output_location: "qualified-gray"}→ the plugin buildsBaseJobRequest(api_factory.py). Itsfield_validatorchecks the value isn't empty / no// no#.CreatePlatformJobRequest(output_location=...)and calls core-jobs.JobDispatcher.create_job(dispatcher.py):output_locationset? →files.get_fileset(...)to check the cabinet exists (else 422). Setfileset = "qualified-gray".PlatformJob(fileset="qualified-gray", output_location="qualified-gray", …). ← the cabinet choice is now written on the job, forever.PlatformJobStepWithContext(fileset=job.fileset, log_subpath = job.name if job.output_location else None, …)(dispatcher.py:720). ← the "write logs into the<job-name>/drawer" sticky note.Route B — The RESULTS clerk (easy: he asks)
The results clerk phones the front desk before filing, so he just asks which drawer.
ctx.results.save("ltManager.create_result**(result_manager.py)._fetch_job_metadata()→get.output_location** off the response.base = job.name if output_location else None._result_remote_path(attempt, "summary", base)→"nemo-evaluator.agent-xyz/results/<attempt>/summary""results/<attempt>/summary"job.filesetat that path. ✅ Done — no sticky note needed, he asked.Route C — The LOGS clerk (hard: hen't ask)
The logs clerk is a running subprocess shipping logs out to a drop-box endpoint. He knows nothing
about the job — only the URL he was twer name must be written on theshipping label at launch.
Write side:
subprocess._prepare_runtime(step)readsstep.log_subpathand callsget_logs_endpoint_config_from_fileset(fileset, subpath=step.log_subpath)(base.py) → buildsthe URL:
…/filesets/qualified-gray/otlp/v1/logs?base=nemo-evaluator.agent-xyz← drawer name baked intothe label.
upload_otlp_logs(base=…)reads?base=→insert_logs(subpath=base)→ wrib-name>/logs/…`. ✅Read side (Studio asks for logs):
3.
page_job_logs(name)(jobs/ejob, recomputes **subpath = name ifjob.output_location else None
** → thnt.query_logs→ filesquery_otlp_logs→ **query_logs(subualified-gray/<job-name>/logs/…. ✅Why it works: write side and read side both compute the drawer name as
job.name, gated onoutput_location. Same recipe on both ends → they always look in the same drawer.One-line summary
output_location= **which cabinethe job).job.name(server derives it; never passed by you).?base=→subpath) must be handed to him at launch and re-derived identicaSummary by CodeRabbit
Summary by CodeRabbit
New Features
output_locationto control where job artifacts are written.--output-locationto thenemo jobs createCLI.output_location.Bug Fixes
output_locationvalues are rejected with HTTP 422.Tests