feat(customizer): add grpo support with gym environments - #1156
Merged
Conversation
Contributor
|
soluwalana
marked this pull request as ready for review
August 7, 2026 23:58
soluwalana
force-pushed
the
aalgo-418/solu
branch
from
August 8, 2026 00:08
16026ea to
f72e6e3
Compare
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:
📝 WalkthroughWalkthroughAdds GRPO training support with environment filesets, Prime Intellect conversion, offline package bootstrap, sandboxed NeMo Gym execution, and OpenSandbox deployment profiles. Updates schemas, APIs, validation, documentation, tests, and storage-path handling. ChangesGRPO environment workflow
Sequence Diagram(s)sequenceDiagram
participant Client
participant JobCompiler
participant EnvironmentFileset
participant GRPODriver
participant NeMoGym
participant Ray
Client->>JobCompiler: submit GRPO job
JobCompiler->>EnvironmentFileset: validate and download environment
JobCompiler->>GRPODriver: create GRPO training step
GRPODriver->>EnvironmentFileset: bootstrap package
GRPODriver->>NeMoGym: initialize environment and dataset
GRPODriver->>Ray: start GRPO training
Ray->>NeMoGym: execute rollouts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.md-171-171 (1)
171-171: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate DPO tuning bullet.
The
ref_policy_kl_penaltyguidance appears twice in adjacent bullets. Keep one authoritative statement.🤖 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/src/nemo_customizer/skills/nemo-customizer/SKILL.md` at line 171, Remove the duplicate adjacent DPO tuning bullet in the skill documentation, keeping one authoritative statement for ref_policy_kl_penalty and its activation_checkpointing guidance.plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md-21-21 (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate DPO-only documentation pointers after adding GRPO.
The PR adds GRPO but leaves related RL references DPO-only. Keep the canonical reference and plugin README aligned.
plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md#L21-L21: update the surrounding RL summary to say DPO + GRPO and mention the environment and conversion references.plugins/nemo-rl/README.md#L63-L64: update the field-reference and related skill pointers to the combined DPO/GRPO content.🤖 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/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md` at line 21, Update the RL documentation references in plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md:21 to describe combined DPO and GRPO coverage, including the environment and conversion references. Also update the field-reference and related skill pointers in plugins/nemo-rl/README.md:63-64 to point to the combined DPO/GRPO content.plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.md-173-173 (1)
173-173: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the contradictory
max_stepsguidance.This bullet permits
max_stepswithepochsfor smoke jobs, while the earlier skill guidance says never combine them andhyperparameters-rl.mddescribes the combination as safe. State the GRPO rule explicitly and make both references match.🤖 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/src/nemo_customizer/skills/nemo-customizer/SKILL.md` at line 173, Update the max_steps and epochs guidance in the SKILL.md GRPO section to state one explicit rule for their combination, then align the corresponding guidance in hyperparameters-rl.md and the referenced smoke fixture documentation. Remove the contradictory permission or prohibition so all references consistently describe the supported behavior.services/rl/tests/test_grpo_config.py-95-95 (1)
95-95: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert the exact egress allowlist.
A truthiness check passes for any non-empty or incorrect rule set. Compare the expected vLLM and broker hosts and ports, and reject unexpected extra rules.
🤖 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/rl/tests/test_grpo_config.py` at line 95, Update the assertion in the GRPO configuration test to compare egress_allow against the exact expected vLLM and broker host/port rules, rather than checking truthiness. Ensure the comparison rejects missing, incorrect, or unexpected extra rules.services/rl/tests/test_sandbox_config.py-79-82 (1)
79-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the no-scratch test assert the intended branch.
The test name says it uses
/tmpwhen no scratch path exists, but the assertion also accepts/scratch. It can pass when the helper chooses the scratch path. Clear the configured scratch input and assert the intended fallback, or rename the test to reflect both allowed roots.🤖 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/rl/tests/test_sandbox_config.py` around lines 79 - 82, Update test_resolve_ephemeral_work_path_uses_tmp_when_no_scratch to clear the configured scratch input before calling resolve_ephemeral_work_path, then assert the result starts with /tmp/ only while preserving the existing path suffix assertion.services/rl/tests/test_sandbox_config.py-21-26 (1)
21-26: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIsolate the default egress test from ambient environment variables.
assemble_master_egress_allow()reads the fourNMP_*_SERVICE_*variables before applying defaults. This test does not clear them, so a developer shell or CI environment can change the values under test or make the assertion fail. Addmonkeypatchand delete all four variables before the call.🤖 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/rl/tests/test_sandbox_config.py` around lines 21 - 26, Update test_assemble_master_egress_allow_defaults to accept pytest’s monkeypatch fixture and delete all four NMP_*_SERVICE_* environment variables before calling assemble_master_egress_allow, ensuring the existing default-rule assertion is isolated from ambient environment configuration.services/rl/tests/test_sandbox_config.py-10-18 (1)
10-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Ruff import-order failure.
CI reports
Ruff I001for this import block. Reorder the imports and runuv run pre-commit run -abefore merging.As per coding guidelines, run Ruff through
uvand run all pre-commit hooks before committing.🤖 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/rl/tests/test_sandbox_config.py` around lines 10 - 18, Reorder the imported symbols in the test module’s sandbox_config import block to satisfy Ruff I001, preserving all existing imports. Validate the change by running Ruff through uv and then execute uv run pre-commit run -a.Sources: Coding guidelines, Pipeline failures
plugins/nemo-rl/src/nemo_rl_plugin/schema.py-60-64 (1)
60-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the GRPO environment requirement in
RlJobInput.When
training.typeisgrpoandenvironmentis absent,RlJobInput.model_validate()accepts the input. The transform rejects it only later. Add a model-level validator and a rejecting contract test inplugins/nemo-rl/tests/test_contract_job_inputs.py.🤖 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-rl/src/nemo_rl_plugin/schema.py` around lines 60 - 64, Update the RlJobInput model validation to reject inputs where training.type is grpo and environment is absent, while preserving acceptance for other training types and provided environments. Add a contract test in test_contract_job_inputs.py that verifies model_validate() raises for the missing-environment GRPO case.Source: Coding guidelines
packages/nmp_customization_common/values/opensandbox/verify/lib.sh-42-44 (1)
42-44: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound every
curlcall.
READY_TIMEOUT_Sdoes not bound an in-flight health, create, status, or delete request. A stalled port-forward or API can hang verification and prevent cleanup. Define shared--connect-timeoutand--max-timearguments, then apply them to every call.Also applies to: 128-128, 154-157, 168-170
🤖 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/nmp_customization_common/values/opensandbox/verify/lib.sh` around lines 42 - 44, Define shared curl timeout arguments using READY_TIMEOUT_S (or the established verification timeout) and apply both --connect-timeout and --max-time to every curl invocation in the verification flow, including health, create, status, and delete calls. Update the calls near the cleanup and referenced locations while preserving their existing request behavior and error handling.services/rl/src/nmp/rl/tasks/environment/__main__.py-99-110 (1)
99-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--vf-env-argsbefore conversion.Invalid JSON raises an uncaught traceback. A valid JSON scalar also passes this line although
vf_env_argsrequires an object. CatchJSONDecodeError, requiredict, and return a CLI usage error.🤖 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/rl/src/nmp/rl/tasks/environment/__main__.py` around lines 99 - 110, Update the `vf_env_args` parsing in the CLI entrypoint before constructing `ConvertEnvironmentSpec`: catch `json.JSONDecodeError`, require the parsed value to be a dictionary, and return the established CLI usage-error response for invalid JSON or non-object JSON values instead of allowing an uncaught traceback.services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py-100-108 (1)
100-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the asserts with explicit errors. Python removes
assertunder-O, so_should_use_nemo_gym(config)and the generation-config check silently pass and training then fails deeper with an opaque error. RaiseValueErrorinstead.🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py` around lines 100 - 108, In the GRPO setup flow, replace the generation-config assertion and the _should_use_nemo_gym(config) assertion with explicit ValueError checks. Preserve the existing validation conditions and generation-required message, and raise clear errors whenever either condition is not satisfied so behavior remains enforced under optimized Python execution.services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py-103-108 (1)
103-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the port parsing. A non-numeric
NMP_VLLM_PORT/NMP_BROKER_PORTvalue raises a bareValueErrorfromint()inside config compilation and inside the driver's egress refresh. Catch it and report which variable is invalid.🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py` around lines 103 - 108, Guard the integer parsing for NMP_VLLM_PORT_ENVVAR and NMP_BROKER_PORT_ENVVAR in the host/port configuration flow, including the driver’s egress refresh path. Catch non-numeric values and raise or report a clear configuration error identifying the specific invalid environment variable, while preserving explicit numeric port arguments and valid environment values.services/rl/src/nmp/rl/config.py-70-76 (1)
70-76: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the misleading description. The code performs no OpenSandbox detection.
_build_grpo_training_step_configfails wheneversandbox_cluster_capable=false, regardless of actual cluster state. State that the operator must set this flag manually.📝 Proposed wording
description=( - "When false and sandboxed_gym_default is true, GRPO jobs with environment " - "filesets fail at compile time unless the cluster has OpenSandbox available." + "Operator-set capability flag. When false and sandboxed_gym_default is true, " + "GRPO jobs with environment filesets fail at compile time. Set to true only " + "after OpenSandbox is installed on the cluster." ),🤖 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/rl/src/nmp/rl/config.py` around lines 70 - 76, Update the description of sandbox_cluster_capable to state that the operator must set it manually and that GRPO jobs with environment filesets fail when it is false; remove the misleading claim about detecting or requiring actual OpenSandbox availability.services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py-50-53 (1)
50-53: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winHonor
TMPDIRinstead of hardcoding/tmp. On a shared node the fallback path is predictable and world-writable, so another process can pre-create or symlink it. Static analysis flags this as CWE-377.🛡️ Proposed fix
def resolve_ephemeral_work_path(job_id: str) -> str: """Prefer node-local ``/scratch`` for lock-heavy Gym/HF work; else ``TMPDIR``.""" - base = Path("/scratch") if Path("/scratch").is_dir() else Path("/tmp") + base = Path("/scratch") if Path("/scratch").is_dir() else Path(tempfile.gettempdir()) return str(base / "nmp-rl" / job_id / "work")🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py` around lines 50 - 53, Update resolve_ephemeral_work_path to use the TMPDIR environment value for its fallback instead of hardcoding /tmp, while preserving the /scratch preference. Apply a safe default only when TMPDIR is unset, then continue constructing the nmp-rl/job_id/work path from the selected base.Source: Linters/SAST tools
services/rl/src/nmp/rl/schemas/job.py-77-88 (1)
77-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose
max_grad_normfor GRPO. Add the field toGRPOTrainingand passt.max_grad_normtoGRPOConfig; the backend reads this value, but the compiler currently always uses its default.🤖 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/rl/src/nmp/rl/schemas/job.py` around lines 77 - 88, Add the missing max_grad_norm field to GRPOTraining using the established training-base type and default, then update the GRPOConfig construction to pass t.max_grad_norm explicitly so compiler configuration matches the backend value.
🧹 Nitpick comments (8)
plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the required reference-page structure.
Add a
## Prerequisitessection before the job layout. State the Kubernetes runtime, full-weight model, dataset fileset, and GRPO environment fileset requirements. Add a## Next Stepssection after troubleshooting with links to dataset formats, environment conversion/upload, and runtime troubleshooting.As per coding guidelines, documentation pages must list prerequisites at the top and include a
Next Stepssection with cross-links at the end.Also applies to: 126-127
🤖 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/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md` around lines 1 - 5, Add a ## Prerequisites section before the job layout covering Kubernetes runtime, full-weight models, the dataset fileset, and GRPO environment fileset requirements. Add a ## Next Steps section after troubleshooting with links to dataset formats, environment conversion/upload, and runtime troubleshooting, preserving the existing reference-page content.Source: Coding guidelines
services/rl/README.md (1)
4-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd the required task documentation structure.
List prerequisites before the overview. Keep this page as an EXPLANATION page. Link to a dedicated HOW-TO with tested CLI and Python conversion examples in tabs. Add a
Next Stepssection.As per coding guidelines, “Always list prerequisites at the top of documentation pages,” “Provide both Python SDK and CLI examples in tab-sets,” and “Include 'Next Steps' section at the end.”
🤖 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/rl/README.md` around lines 4 - 21, Restructure services/rl/README.md as an Explanation page by moving prerequisites to the top, before the overview. Add a link to a dedicated HOW-TO containing tested CLI and Python conversion examples presented in tabs, and finish this page with a “Next Steps” section linking onward guidance. Preserve the existing overview and package descriptions while adding only the required documentation structure.Source: Coding guidelines
packages/nmp_customization_common/values/opensandbox/verify/all.sh (1)
5-6: 🩺 Stability & Availability | 🔵 TrivialRun deployment verification before merge.
The PR summary states that live Mode B E2E validation and pre-commit were not run. Run
uv run pre-commit run -aandverify/all.shin an authorized target-cluster session. Retain the output for both profiles.🤖 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/nmp_customization_common/values/opensandbox/verify/all.sh` around lines 5 - 6, Run uv run pre-commit run -a and verify/all.sh in an authorized target-cluster session, exercising both verification profiles invoked by crun.sh and kata-qemu.sh, and retain the complete output for each profile.Source: Coding guidelines
services/rl/src/nmp/rl/schemas/job.py (1)
149-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate branches. Both checks assert the same condition.
♻️ Proposed simplification
- if self.training_type == TrainingType.DPO and self.output.type != OutputNameType.MODEL: - raise ValueError("DPO produces a full-weight model; output.type must be 'model'.") - if self.training_type == TrainingType.GRPO and self.output.type != OutputNameType.MODEL: - raise ValueError("GRPO produces a full-weight model; output.type must be 'model'.") + if self.output.type != OutputNameType.MODEL: + raise ValueError( + f"{self.training_type.value.upper()} produces a full-weight model; output.type must be 'model'." + )🤖 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/rl/src/nmp/rl/schemas/job.py` around lines 149 - 155, In _output_type_matches_training, combine the duplicate DPO and GRPO branches into one condition that checks whether training_type is either TrainingType.DPO or TrainingType.GRPO while output.type is not OutputNameType.MODEL. Preserve the existing ValueError message and return self behavior.services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py (2)
102-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not default to a
:latesttag. A mutable tag makes runs non-reproducible and can change the sandbox runtime mid-campaign. Pin a digest or version, or fail whengym_runtime_imageis unset.🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py` at line 102, Update the runtime_image initialization in the GRPO configuration to remove the mutable :latest fallback. When gym.gym_runtime_image is unset, either require a pinned digest/version or fail explicitly; preserve explicitly configured immutable runtime images.
131-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import yamlto module scope and handle unreadable manifests. The local import hides the dependency.yaml.safe_loadalso raises on malformed YAML, which aborts compilation with a raw parser error. Wrap it and report the manifest path.🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py` around lines 131 - 141, Move the yaml import to module scope, then update the manifest-loading logic in the gym configuration flow to catch YAML read/parse errors from nemo-environment.yaml and report the manifest path in the resulting error. Preserve the existing config_paths handling for valid dictionary manifests.services/rl/src/nmp/rl/app/jobs/compiler.py (1)
262-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared step-config body.
_build_grpo_training_step_configand_build_dpo_training_step_configduplicate the model, dataset, schedule, batch, optimizer, parallelism, integrations, output, and seed blocks verbatim. The two will drift. Extract a helper that builds the commonTrainingStepConfigfields and let each builder supply onlytrainingandgym.🤖 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/rl/src/nmp/rl/app/jobs/compiler.py` around lines 262 - 340, The GRPO and DPO builders duplicate the shared TrainingStepConfig construction. Extract the model, dataset, schedule, batch, optimizer, parallelism, integrations, output, and seed fields into a shared helper, then update _build_grpo_training_step_config and _build_dpo_training_step_config to provide only their training and gym configurations while preserving their existing backend-specific values.services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py (1)
171-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import rayout of the loop and reuse the module-level logger for startup output. The import inside thetryrepeats per environment and hides the failure mode if ray is unavailable. Lines 82-95 also uselogger, which loses these details in structured job logs.♻️ Proposed change
+import ray + ... finally: for task_name, env in task_to_env.items(): try: - import ray - ray.get(env.shutdown.remote(), timeout=120) except Exception as exc: logger.warning("Error shutting down environment %s: %s", task_name, exc)🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py` around lines 171 - 178, Move the ray import to module scope and reuse that imported symbol in the finally shutdown loop, allowing ray import failures to surface during module initialization rather than per-environment cleanup. Replace the startup print calls in the code around lines 82-95 with the existing module-level logger, preserving their messages and appropriate log levels for structured output.
🤖 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/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yaml`:
- Around line 40-41: Remove the broad Exists toleration from both
packages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yaml
lines 40-41 and
packages/nmp_customization_common/values/opensandbox/batchsandbox-template-kata-qemu.yaml
lines 30-31, unless the selected nodes require taint tolerance; in that case,
configure the specific taint key and effect in both templates.
In `@packages/nmp_customization_common/values/opensandbox/install.sh`:
- Line 13: Remove xtrace from the shell options in install.sh while preserving
errexit, nounset, and pipefail. Ensure tracing is disabled before the API key
generation and kubectl secret creation commands spanning the secret-handling
flow, so generated or supplied keys are not emitted in terminal or CI logs.
- Around line 30-33: Validate the active Kubernetes context against
EXPECTED_CONTEXT before proceeding: in
packages/nmp_customization_common/values/opensandbox/install.sh lines 30-33, add
the check before the first mutation; in
packages/nmp_customization_common/values/opensandbox/verify/lib.sh lines 91-92,
perform the same check before reading Secrets or creating a sandbox. Reject
unexpected contexts and exit without continuing.
In
`@packages/nmp_customization_common/values/opensandbox/opensandbox-server-crun.yaml`:
- Around line 89-94: Set disable_ipv6 to true in the [egress] profiles of
packages/nmp_customization_common/values/opensandbox/opensandbox-server-crun.yaml
(lines 89-94) and
packages/nmp_customization_common/values/opensandbox/opensandbox-server-kata-qemu.yaml
(lines 90-95), removing the current false values while preserving the other
egress settings.
In `@packages/nmp_customization_common/values/opensandbox/README.md`:
- Around line 1-145: Restructure the OpenSandbox README as an EXPLANATION of the
two-profile design: keep the rationale, shared namespace/isolation model, and
profile overview, while moving prerequisites before cluster details. Create
HOW-TO pages for installation and verification, and REFERENCE pages for
releases, Secrets, endpoints, and node selectors; remove those
operational/reference sections from the README. Add a Next Steps section linking
to each new page, using the existing filenames and concepts consistently.
In `@services/rl/src/nmp/rl/app/jobs/compiler.py`:
- Around line 371-379: Update the GRPO environment setup in the compiler so
NMP_VLLM_HOST_ENVVAR/NMP_VLLM_PORT_ENVVAR and
NMP_BROKER_HOST_ENVVAR/NMP_BROKER_PORT_ENVVAR use endpoints reachable from the
sandbox pod rather than loopback values. Resolve the service endpoints before
appending the EnvironmentVariable entries, preserving the existing GRPO-only
behavior.
In `@services/rl/src/nmp/rl/tasks/environment/bootstrap.py`:
- Line 20: Remove the unused EnvironmentFormat import from
services/rl/src/nmp/rl/tasks/environment/bootstrap.py at lines 20-20 and the
unused AdapterWheelsV1Manifest import from
services/rl/src/nmp/rl/tasks/environment/convert.py at lines 17-17; no other
changes are needed.
In `@services/rl/src/nmp/rl/tasks/environment/convert.py`:
- Around line 221-224: Update the split logic in the environment conversion path
around all_rows and spec.validation_fraction to reject values outside 0 <=
validation_fraction < 1, and ensure a positive fraction never consumes the only
available row. Preserve distinct validation and training datasets by limiting
the validation split so at least one training row remains; do not fall back to
all_rows for train_rows when validation is requested.
- Around line 128-138: Update the wheel installation command in the conversion
environment to install the selected wheel and its dependency closure by removing
--no-deps and adding --no-index with --find-links pointing to the downloaded
wheel directory. Preserve --force-reinstall and the existing subprocess.run
invocation.
In `@services/rl/src/nmp/rl/tasks/environment/package.py`:
- Around line 99-104: Ensure repeat runs produce exact package snapshots: in
services/rl/src/nmp/rl/tasks/environment/package.py lines 99-104, clear the
existing wheels directory before copying current wheels; in
services/rl/src/nmp/rl/tasks/environment/upload.py lines 65-87, clear, replace,
or version existing FileSets before uploading so removed wheels and dataset
files cannot persist as stale artifacts.
- Around line 6-22: Reorder the imports in
services/rl/src/nmp/rl/tasks/environment/package.py at lines 6-22 according to
Ruff. In services/rl/src/nmp/rl/tasks/environment/validate.py at lines 6-20,
reorder the imports and remove the unused WheelsV1Manifest import. Run
validation with uv run ruff check before merging.
In `@services/rl/src/nmp/rl/tasks/environment/upload.py`:
- Line 63: Update the FilesClient construction to pass the token via the
inherited NemoClient.__init__ auth parameter, replacing the unsupported api_key
argument with auth while preserving the existing base_url and workspace
arguments.
In `@services/rl/src/nmp/rl/tasks/environment/validate.py`:
- Around line 55-57: Replace the string-prefix check in the config path
validation with path containment using resolved.relative_to(env_root.resolve()).
Catch ValueError and raise EnvironmentPackageValidationError for paths outside
the environment root, including paths escaping through symlinked parents.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py`:
- Around line 156-164: Update the configuration-building logic near batch_size,
micro_batch_size, and num_prompts so policy.train’s train_global_batch_size
matches the effective GRPO batch size: num_prompts_per_step multiplied by
num_generations_per_prompt. If the configured value cannot be updated, validate
it and reject mismatches before training begins.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py`:
- Around line 112-114: Update the configuration handling in the GRPO driver to
treat MasterConfig and GRPOConfig as mappings: cast the
OmegaConf.to_container(...) result to MasterConfig, replace
type(config).model_fields and all config attribute access with mapping/subscript
access, including the val_dataset assignments and lines around the training
configuration, and replace model_dump() with mapping-compatible usage.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py`:
- Around line 62-81: Prevent broker_token from being included in the
bootstrap_env configuration passed to hyperparameter logging: inject it through
a Kubernetes secret reference, or redact bootstrap_env before grpo_driver logs
config.model_dump(). In the environment-variable builder, derive
NMP_MAX_REQUEST_BYTES and NMP_MAX_RESPONSE_BYTES from the corresponding
SandboxConfig defaults instead of duplicating literal values. Preserve the
existing environment variable names and injection behavior.
In `@services/rl/tests/test_environment_package.py`:
- Line 11: Remove the unused yaml import from the test module so Ruff no longer
reports F401; leave the remaining imports and test behavior unchanged.
---
Minor comments:
In `@packages/nmp_customization_common/values/opensandbox/verify/lib.sh`:
- Around line 42-44: Define shared curl timeout arguments using READY_TIMEOUT_S
(or the established verification timeout) and apply both --connect-timeout and
--max-time to every curl invocation in the verification flow, including health,
create, status, and delete calls. Update the calls near the cleanup and
referenced locations while preserving their existing request behavior and error
handling.
In
`@plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md`:
- Line 21: Update the RL documentation references in
plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md:21
to describe combined DPO and GRPO coverage, including the environment and
conversion references. Also update the field-reference and related skill
pointers in plugins/nemo-rl/README.md:63-64 to point to the combined DPO/GRPO
content.
In `@plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.md`:
- Line 171: Remove the duplicate adjacent DPO tuning bullet in the skill
documentation, keeping one authoritative statement for ref_policy_kl_penalty and
its activation_checkpointing guidance.
- Line 173: Update the max_steps and epochs guidance in the SKILL.md GRPO
section to state one explicit rule for their combination, then align the
corresponding guidance in hyperparameters-rl.md and the referenced smoke fixture
documentation. Remove the contradictory permission or prohibition so all
references consistently describe the supported behavior.
In `@plugins/nemo-rl/src/nemo_rl_plugin/schema.py`:
- Around line 60-64: Update the RlJobInput model validation to reject inputs
where training.type is grpo and environment is absent, while preserving
acceptance for other training types and provided environments. Add a contract
test in test_contract_job_inputs.py that verifies model_validate() raises for
the missing-environment GRPO case.
In `@services/rl/src/nmp/rl/config.py`:
- Around line 70-76: Update the description of sandbox_cluster_capable to state
that the operator must set it manually and that GRPO jobs with environment
filesets fail when it is false; remove the misleading claim about detecting or
requiring actual OpenSandbox availability.
In `@services/rl/src/nmp/rl/schemas/job.py`:
- Around line 77-88: Add the missing max_grad_norm field to GRPOTraining using
the established training-base type and default, then update the GRPOConfig
construction to pass t.max_grad_norm explicitly so compiler configuration
matches the backend value.
In `@services/rl/src/nmp/rl/tasks/environment/__main__.py`:
- Around line 99-110: Update the `vf_env_args` parsing in the CLI entrypoint
before constructing `ConvertEnvironmentSpec`: catch `json.JSONDecodeError`,
require the parsed value to be a dictionary, and return the established CLI
usage-error response for invalid JSON or non-object JSON values instead of
allowing an uncaught traceback.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py`:
- Around line 100-108: In the GRPO setup flow, replace the generation-config
assertion and the _should_use_nemo_gym(config) assertion with explicit
ValueError checks. Preserve the existing validation conditions and
generation-required message, and raise clear errors whenever either condition is
not satisfied so behavior remains enforced under optimized Python execution.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py`:
- Around line 103-108: Guard the integer parsing for NMP_VLLM_PORT_ENVVAR and
NMP_BROKER_PORT_ENVVAR in the host/port configuration flow, including the
driver’s egress refresh path. Catch non-numeric values and raise or report a
clear configuration error identifying the specific invalid environment variable,
while preserving explicit numeric port arguments and valid environment values.
- Around line 50-53: Update resolve_ephemeral_work_path to use the TMPDIR
environment value for its fallback instead of hardcoding /tmp, while preserving
the /scratch preference. Apply a safe default only when TMPDIR is unset, then
continue constructing the nmp-rl/job_id/work path from the selected base.
In `@services/rl/tests/test_grpo_config.py`:
- Line 95: Update the assertion in the GRPO configuration test to compare
egress_allow against the exact expected vLLM and broker host/port rules, rather
than checking truthiness. Ensure the comparison rejects missing, incorrect, or
unexpected extra rules.
In `@services/rl/tests/test_sandbox_config.py`:
- Around line 79-82: Update
test_resolve_ephemeral_work_path_uses_tmp_when_no_scratch to clear the
configured scratch input before calling resolve_ephemeral_work_path, then assert
the result starts with /tmp/ only while preserving the existing path suffix
assertion.
- Around line 21-26: Update test_assemble_master_egress_allow_defaults to accept
pytest’s monkeypatch fixture and delete all four NMP_*_SERVICE_* environment
variables before calling assemble_master_egress_allow, ensuring the existing
default-rule assertion is isolated from ambient environment configuration.
- Around line 10-18: Reorder the imported symbols in the test module’s
sandbox_config import block to satisfy Ruff I001, preserving all existing
imports. Validate the change by running Ruff through uv and then execute uv run
pre-commit run -a.
---
Nitpick comments:
In `@packages/nmp_customization_common/values/opensandbox/verify/all.sh`:
- Around line 5-6: Run uv run pre-commit run -a and verify/all.sh in an
authorized target-cluster session, exercising both verification profiles invoked
by crun.sh and kata-qemu.sh, and retain the complete output for each profile.
In
`@plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md`:
- Around line 1-5: Add a ## Prerequisites section before the job layout covering
Kubernetes runtime, full-weight models, the dataset fileset, and GRPO
environment fileset requirements. Add a ## Next Steps section after
troubleshooting with links to dataset formats, environment conversion/upload,
and runtime troubleshooting, preserving the existing reference-page content.
In `@services/rl/README.md`:
- Around line 4-21: Restructure services/rl/README.md as an Explanation page by
moving prerequisites to the top, before the overview. Add a link to a dedicated
HOW-TO containing tested CLI and Python conversion examples presented in tabs,
and finish this page with a “Next Steps” section linking onward guidance.
Preserve the existing overview and package descriptions while adding only the
required documentation structure.
In `@services/rl/src/nmp/rl/app/jobs/compiler.py`:
- Around line 262-340: The GRPO and DPO builders duplicate the shared
TrainingStepConfig construction. Extract the model, dataset, schedule, batch,
optimizer, parallelism, integrations, output, and seed fields into a shared
helper, then update _build_grpo_training_step_config and
_build_dpo_training_step_config to provide only their training and gym
configurations while preserving their existing backend-specific values.
In `@services/rl/src/nmp/rl/schemas/job.py`:
- Around line 149-155: In _output_type_matches_training, combine the duplicate
DPO and GRPO branches into one condition that checks whether training_type is
either TrainingType.DPO or TrainingType.GRPO while output.type is not
OutputNameType.MODEL. Preserve the existing ValueError message and return self
behavior.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py`:
- Line 102: Update the runtime_image initialization in the GRPO configuration to
remove the mutable :latest fallback. When gym.gym_runtime_image is unset, either
require a pinned digest/version or fail explicitly; preserve explicitly
configured immutable runtime images.
- Around line 131-141: Move the yaml import to module scope, then update the
manifest-loading logic in the gym configuration flow to catch YAML read/parse
errors from nemo-environment.yaml and report the manifest path in the resulting
error. Preserve the existing config_paths handling for valid dictionary
manifests.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py`:
- Around line 171-178: Move the ray import to module scope and reuse that
imported symbol in the finally shutdown loop, allowing ray import failures to
surface during module initialization rather than per-environment cleanup.
Replace the startup print calls in the code around lines 82-95 with the existing
module-level logger, preserving their messages and appropriate log levels for
structured output.
🪄 Autofix
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: 1bc2f5bc-7c59-4f32-9da5-89d8240da040
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
docker/rl/README.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.pypackages/nmp_customization_common/src/nmp/customization_common/service/constants.pypackages/nmp_customization_common/src/nmp/customization_common/service/platform_client.pypackages/nmp_customization_common/values/opensandbox/README.mdpackages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yamlpackages/nmp_customization_common/values/opensandbox/batchsandbox-template-kata-qemu.yamlpackages/nmp_customization_common/values/opensandbox/install.shpackages/nmp_customization_common/values/opensandbox/opensandbox-controller.yamlpackages/nmp_customization_common/values/opensandbox/opensandbox-server-crun.yamlpackages/nmp_customization_common/values/opensandbox/opensandbox-server-kata-qemu.yamlpackages/nmp_customization_common/values/opensandbox/verify/all.shpackages/nmp_customization_common/values/opensandbox/verify/crun.shpackages/nmp_customization_common/values/opensandbox/verify/kata-qemu.shpackages/nmp_customization_common/values/opensandbox/verify/lib.shplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.mdplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.mdplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.mdplugins/nemo-rl/README.mdplugins/nemo-rl/src/nemo_rl_plugin/schema.pyplugins/nemo-rl/src/nemo_rl_plugin/transform.pyplugins/nemo-rl/tests/fixtures/minimal_grpo.jsonplugins/nemo-rl/tests/test_contract_job_inputs.pyservices/rl/README.mdservices/rl/pyproject.tomlservices/rl/src/nmp/rl/app/constants.pyservices/rl/src/nmp/rl/app/jobs/compiler.pyservices/rl/src/nmp/rl/app/jobs/training/schemas.pyservices/rl/src/nmp/rl/config.pyservices/rl/src/nmp/rl/schemas.pyservices/rl/src/nmp/rl/schemas/__init__.pyservices/rl/src/nmp/rl/schemas/environment.pyservices/rl/src/nmp/rl/schemas/job.pyservices/rl/src/nmp/rl/tasks/environment/__init__.pyservices/rl/src/nmp/rl/tasks/environment/__main__.pyservices/rl/src/nmp/rl/tasks/environment/allowlist.pyservices/rl/src/nmp/rl/tasks/environment/bootstrap.pyservices/rl/src/nmp/rl/tasks/environment/convert.pyservices/rl/src/nmp/rl/tasks/environment/package.pyservices/rl/src/nmp/rl/tasks/environment/upload.pyservices/rl/src/nmp/rl/tasks/environment/validate.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/backend.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.pyservices/rl/src/nmp/rl/tasks/training/datasets/validation.pyservices/rl/tests/test_compiler.pyservices/rl/tests/test_environment_package.pyservices/rl/tests/test_grpo_config.pyservices/rl/tests/test_sandbox_config.pyservices/rl/tests/test_schemas.py
💤 Files with no reviewable changes (1)
- services/rl/src/nmp/rl/schemas.py
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py (1)
134-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
yamlimport to module scope.The lazy
import yamlinside the branch hides the dependency and adds no benefit;yamlis already required elsewhere in the task image.♻️ Proposed change
- if manifest_path.is_file(): - import yaml - - manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if manifest_path.is_file(): + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8"))Add
import yamlto the module imports.🤖 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/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py` around lines 134 - 140, Move the yaml import from the manifest-handling branch into the module-level imports of grpo_config.py, and remove the nested import while keeping the existing yaml.safe_load behavior unchanged.services/rl/src/nmp/rl/schemas/environment.py (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse parameterized
dictannotations.
vf_env_args,responses_create_params, andinfouse baredict. Pydantic accepts any key/value type here, which weakens validation and type checking. Usedict[str, Any].♻️ Proposed change
- vf_env_args: dict = Field(default_factory=dict) + vf_env_args: dict[str, Any] = Field(default_factory=dict)- responses_create_params: dict + responses_create_params: dict[str, Any] agent_ref: AgentRef answer: str = "" task: str = "" example_id: int | str = 0 - info: dict = Field(default_factory=dict) + info: dict[str, Any] = Field(default_factory=dict)Add
Anyto thetypingimport.Also applies to: 132-132, 137-137
🤖 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/rl/src/nmp/rl/schemas/environment.py` at line 108, Update the annotations for vf_env_args, responses_create_params, and info to use dict[str, Any] instead of bare dict, and add Any to the typing imports. Preserve their existing default values and field behavior.Source: Coding guidelines
services/rl/src/nmp/rl/app/jobs/compiler.py (1)
262-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared step-config sections.
_build_grpo_training_step_configduplicates theschedule,batch,optimizer,parallelism,integrations,output_model, andseedblocks from_build_dpo_training_step_configverbatim. Both functions will drift when a field changes. Extract helpers such as_build_schedule_config(t),_build_optimizer_config(t), and_build_parallelism_config(t, p), then keep only the method-specifictraining/gymblocks in each function.🤖 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/rl/src/nmp/rl/app/jobs/compiler.py` around lines 262 - 334, Extract the duplicated schedule, batch, optimizer, parallelism, integrations, output_model, and seed construction from _build_grpo_training_step_config and _build_dpo_training_step_config into shared helpers, such as _build_schedule_config, _build_batch_config, _build_optimizer_config, and _build_parallelism_config. Update both builders to call these helpers while retaining only their method-specific training and gym configuration, and preserve all existing field mappings and defaults.services/rl/src/nmp/rl/tasks/environment/__main__.py (2)
116-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a concrete type for
payload.Replace
dictwithdict[str, object].As per coding guidelines, “Prefer concrete type hints over string-based annotations.”
🤖 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/rl/src/nmp/rl/tasks/environment/__main__.py` around lines 116 - 122, Update the payload annotation in the result serialization block to use the concrete type dict[str, object] instead of the unparameterized dict, while preserving the existing payload fields and values.Source: Coding guidelines
153-154: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required hook suite before merge.
Run
uv run pre-commit run -aand fix all failures.🤖 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/rl/src/nmp/rl/tasks/environment/__main__.py` around lines 153 - 154, Run the required hook suite with `uv run pre-commit run -a` and resolve every reported failure, including any issues in the `main` entry point or surrounding changes, before completing the merge.Source: Coding guidelines
🤖 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-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md`:
- Around line 3-7: Move the Kubernetes runtime prerequisites and GRPO
conversion/upload requirements to the top of the hyperparameters-rl
documentation, before the DPO and GRPO job layouts. Include the existing GRPO
Internet-capable host requirement there, and avoid duplicating it later in the
page.
- Around line 126-127: Add a “Next Steps” section at the end of the
hyperparameters-rl documentation, including cross-links to hyperparameters.md
and dataset-formats.md. Keep the existing troubleshooting content unchanged and
use the repository’s standard relative documentation-link style.
In `@services/rl/src/nmp/rl/tasks/environment/__main__.py`:
- Around line 92-99: Update the CLI flow around validate_package_layout and
json.loads(args.vf_env_args) to catch package-validation exceptions and JSON
decoding errors, log a single error message, and return a nonzero status. After
decoding vf_env_args, validate that the result is a JSON object and treat other
JSON types as invalid CLI input through the same error path. Preserve the
existing successful validation and environment-argument behavior.
- Around line 131-133: Update the default dataset-name derivation near env_name
and ds_name so it uses the effective env_name value, producing
“<environment-name>-dataset” when --environment-name is provided while
preserving the hub-based default when it is omitted. Ensure the resulting
ds_name is the value passed to upload_converted_packages.
- Around line 29-35: Update _build_parser() so --hub-id and --out-dir are not
required during argument parsing, then enforce both arguments only in the
non---validate-only execution path after the validation branch. Preserve their
existing types and behavior when conversion runs, while allowing --validate-only
to execute without either argument.
In `@services/rl/src/nmp/rl/tasks/environment/convert.py`:
- Around line 202-205: Update the dataset conversion flow around the
dataset_size == 0 branch to produce package layout only: do not call
write_dataset_jsonl, return optional or absent dataset paths, and bypass dataset
creation and upload when no rows are requested. Propagate the optional-path
behavior through the CLI, and add a regression test covering package-only mode.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py`:
- Line 244: Update the GRPOConfig construction to pass the user-provided
gradient clipping value through its max_grad_norm parameter, using
grpo_hp.max_grad_norm, so the configured value overrides the schema default.
---
Nitpick comments:
In `@services/rl/src/nmp/rl/app/jobs/compiler.py`:
- Around line 262-334: Extract the duplicated schedule, batch, optimizer,
parallelism, integrations, output_model, and seed construction from
_build_grpo_training_step_config and _build_dpo_training_step_config into shared
helpers, such as _build_schedule_config, _build_batch_config,
_build_optimizer_config, and _build_parallelism_config. Update both builders to
call these helpers while retaining only their method-specific training and gym
configuration, and preserve all existing field mappings and defaults.
In `@services/rl/src/nmp/rl/schemas/environment.py`:
- Line 108: Update the annotations for vf_env_args, responses_create_params, and
info to use dict[str, Any] instead of bare dict, and add Any to the typing
imports. Preserve their existing default values and field behavior.
In `@services/rl/src/nmp/rl/tasks/environment/__main__.py`:
- Around line 116-122: Update the payload annotation in the result serialization
block to use the concrete type dict[str, object] instead of the unparameterized
dict, while preserving the existing payload fields and values.
- Around line 153-154: Run the required hook suite with `uv run pre-commit run
-a` and resolve every reported failure, including any issues in the `main` entry
point or surrounding changes, before completing the merge.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.py`:
- Around line 134-140: Move the yaml import from the manifest-handling branch
into the module-level imports of grpo_config.py, and remove the nested import
while keeping the existing yaml.safe_load behavior unchanged.
🪄 Autofix
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: b889f87a-5693-4a91-ad7a-de7fee0f95b8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
docker/rl/README.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.pypackages/nmp_customization_common/src/nmp/customization_common/service/constants.pypackages/nmp_customization_common/src/nmp/customization_common/service/platform_client.pypackages/nmp_customization_common/values/opensandbox/README.mdpackages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yamlpackages/nmp_customization_common/values/opensandbox/batchsandbox-template-kata-qemu.yamlpackages/nmp_customization_common/values/opensandbox/install.shpackages/nmp_customization_common/values/opensandbox/opensandbox-controller.yamlpackages/nmp_customization_common/values/opensandbox/opensandbox-server-crun.yamlpackages/nmp_customization_common/values/opensandbox/opensandbox-server-kata-qemu.yamlpackages/nmp_customization_common/values/opensandbox/verify/all.shpackages/nmp_customization_common/values/opensandbox/verify/crun.shpackages/nmp_customization_common/values/opensandbox/verify/kata-qemu.shpackages/nmp_customization_common/values/opensandbox/verify/lib.shplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.mdplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.mdplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.mdplugins/nemo-rl/README.mdplugins/nemo-rl/src/nemo_rl_plugin/schema.pyplugins/nemo-rl/src/nemo_rl_plugin/transform.pyplugins/nemo-rl/tests/fixtures/minimal_grpo.jsonplugins/nemo-rl/tests/test_contract_job_inputs.pyservices/rl/README.mdservices/rl/pyproject.tomlservices/rl/src/nmp/rl/app/constants.pyservices/rl/src/nmp/rl/app/jobs/compiler.pyservices/rl/src/nmp/rl/app/jobs/training/schemas.pyservices/rl/src/nmp/rl/config.pyservices/rl/src/nmp/rl/schemas.pyservices/rl/src/nmp/rl/schemas/__init__.pyservices/rl/src/nmp/rl/schemas/environment.pyservices/rl/src/nmp/rl/schemas/job.pyservices/rl/src/nmp/rl/tasks/environment/__init__.pyservices/rl/src/nmp/rl/tasks/environment/__main__.pyservices/rl/src/nmp/rl/tasks/environment/allowlist.pyservices/rl/src/nmp/rl/tasks/environment/bootstrap.pyservices/rl/src/nmp/rl/tasks/environment/convert.pyservices/rl/src/nmp/rl/tasks/environment/package.pyservices/rl/src/nmp/rl/tasks/environment/upload.pyservices/rl/src/nmp/rl/tasks/environment/validate.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/backend.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.pyservices/rl/src/nmp/rl/tasks/training/datasets/validation.pyservices/rl/tests/test_compiler.pyservices/rl/tests/test_environment_package.pyservices/rl/tests/test_grpo_config.pyservices/rl/tests/test_sandbox_config.pyservices/rl/tests/test_schemas.py
💤 Files with no reviewable changes (1)
- services/rl/src/nmp/rl/schemas.py
🚧 Files skipped from review as they are similar to previous changes (37)
- plugins/nemo-rl/tests/test_contract_job_inputs.py
- packages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yaml
- services/rl/tests/test_schemas.py
- plugins/nemo-rl/src/nemo_rl_plugin/transform.py
- services/rl/README.md
- packages/nmp_customization_common/values/opensandbox/opensandbox-server-kata-qemu.yaml
- packages/nmp_customization_common/values/opensandbox/verify/all.sh
- services/rl/src/nmp/rl/config.py
- plugins/nemo-rl/tests/fixtures/minimal_grpo.json
- services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/backend.py
- packages/nmp_customization_common/values/opensandbox/install.sh
- packages/nemo_platform_plugin/src/nemo_platform_plugin/files/types.py
- packages/nmp_customization_common/values/opensandbox/opensandbox-controller.yaml
- services/rl/src/nmp/rl/schemas/init.py
- services/rl/tests/test_compiler.py
- services/rl/src/nmp/rl/tasks/training/datasets/validation.py
- packages/nmp_customization_common/values/opensandbox/README.md
- docker/rl/README.md
- services/rl/src/nmp/rl/tasks/environment/allowlist.py
- services/rl/src/nmp/rl/tasks/environment/upload.py
- services/rl/src/nmp/rl/tasks/environment/init.py
- services/rl/tests/test_environment_package.py
- packages/nmp_customization_common/values/opensandbox/batchsandbox-template-kata-qemu.yaml
- packages/nmp_customization_common/src/nmp/customization_common/service/platform_client.py
- plugins/nemo-rl/src/nemo_rl_plugin/schema.py
- packages/nmp_customization_common/values/opensandbox/opensandbox-server-crun.yaml
- plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters.md
- services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py
- services/rl/src/nmp/rl/schemas/job.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.py
- plugins/nemo-rl/README.md
- services/rl/pyproject.toml
- services/rl/src/nmp/rl/app/jobs/training/schemas.py
- plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/SKILL.md
- services/rl/src/nmp/rl/app/constants.py
- packages/nmp_customization_common/src/nmp/customization_common/service/constants.py
- services/rl/src/nmp/rl/tasks/environment/validate.py
15 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
services/rl/src/nmp/rl/tasks/environment/convert.py (2)
123-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill installs with
--no-deps.
_install_hub_package_from_wheelsinstalls the selected wheel with--no-deps. Dataset generation fails when the hub package's dependencies aren't already present. Install with--no-index --find-links={wheels_dir}instead, so the closure resolves from the vendored wheels.🤖 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/rl/src/nmp/rl/tasks/environment/convert.py` around lines 123 - 151, Update _install_hub_package_from_wheels to remove --no-deps and add --no-index plus --find-links pointing to wheels_dir, allowing pip to resolve the hub package dependency closure exclusively from the vendored wheels.
221-278: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPackage-only mode still writes an empty training dataset.
When
spec.dataset_size == 0,all_rowsis[], butwrite_dataset_jsonlstill runs and createstraining.jsonlindataset_out. Package-only conversions should skip dataset creation and upload entirely, not persist an empty dataset artifact.🤖 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/rl/src/nmp/rl/tasks/environment/convert.py` around lines 221 - 278, Update convert_prime_environment so the spec.dataset_size == 0 package-only path returns after creating the adapter package, without calling split_train_validation or write_dataset_jsonl. Preserve the existing dataset generation, validation, and ConvertedPackage return flow for positive dataset sizes, and ensure the package-only result does not create or upload dataset artifacts.
🧹 Nitpick comments (2)
plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md (1)
35-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the GRPO procedure to a dedicated HOW-TO page.
This reference page now includes environment conversion, upload, and job submission steps. Move those steps to a GRPO HOW-TO. Add Python SDK and CLI examples in tab sets, then link to that page here.
As per coding guidelines, each documentation page must fit one Diataxis quadrant and provide Python SDK and CLI examples.
🤖 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/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md` around lines 35 - 62, Move the environment conversion, FileSet upload, and GRPO submission procedure from the hyperparameters reference into a dedicated GRPO HOW-TO page. Add equivalent Python SDK and CLI examples there using tab sets, while keeping this reference focused on hyperparameter information and linking to the new HOW-TO. Preserve the documented environment, dataset, and sandbox configuration requirements.Source: Coding guidelines
openapi/ga/individual/platform.openapi.yaml (1)
10770-10799: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
EnvironmentMetadataContent.formatand regenerate the OpenAPI and SDK artifacts.The source model is
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.py, whereformat: straccepts values outside the three documented formats.🤖 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 `@openapi/ga/individual/platform.openapi.yaml` around lines 10770 - 10799, Update the EnvironmentMetadataContent model’s format field in metadata.py to constrain values to native-v1, wheels-v1, and adapter-wheels-v1, then regenerate the OpenAPI specification and SDK artifacts so the schema exposes the corresponding enum constraint.Source: Coding guidelines
🤖 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 `@k8s/helm/values.yaml`:
- Around line 502-507: Add an rl.sandbox_cluster_capable Helm value alongside
job_storage_pvc_claim, defaulting to false and documenting that operators should
enable it when OpenSandbox is installed and sandboxed GRPO is used. Ensure the
value is exposed through the rendered configuration consumed by RlConfig.
In
`@plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md`:
- Around line 127-129: Rename the “DPO tuning guide” heading in the
hyperparameter tuning documentation to “Tuning guide” so it accurately covers
both DPO and GRPO guidance; leave the table content unchanged.
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py`:
- Line 17: Update build_permissions_init_container to replace the local
f"jobs/{workspace}/{job_id}" path construction with
build_job_storage_subpath(workspace, job_id), matching the existing helper usage
elsewhere and keeping the permissions directory aligned with the job mount
subpath.
In `@services/rl/src/nmp/rl/app/constants.py`:
- Around line 59-63: Update the GRPO error message to reference
NMP_JOB_STORAGE_PVC_CLAIM instead of NMP_RL_JOB_STORAGE_PVC_CLAIM, matching the
constant defined by NMP_JOB_STORAGE_PVC_ENVVAR and the compiler/training
container contract.
In `@services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.py`:
- Around line 179-182: The endpoint resolution in the sandbox configuration
around host_vllm and host_broker must not pass loopback addresses into
OpenSandbox egress rules. Replace compiler-injected 127.0.0.1 values with
routable service endpoints while preserving explicit reachable host overrides,
and add coverage that rejects loopback egress for sandboxed jobs.
---
Duplicate comments:
In `@services/rl/src/nmp/rl/tasks/environment/convert.py`:
- Around line 123-151: Update _install_hub_package_from_wheels to remove
--no-deps and add --no-index plus --find-links pointing to wheels_dir, allowing
pip to resolve the hub package dependency closure exclusively from the vendored
wheels.
- Around line 221-278: Update convert_prime_environment so the spec.dataset_size
== 0 package-only path returns after creating the adapter package, without
calling split_train_validation or write_dataset_jsonl. Preserve the existing
dataset generation, validation, and ConvertedPackage return flow for positive
dataset sizes, and ensure the package-only result does not create or upload
dataset artifacts.
---
Nitpick comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 10770-10799: Update the EnvironmentMetadataContent model’s format
field in metadata.py to constrain values to native-v1, wheels-v1, and
adapter-wheels-v1, then regenerate the OpenAPI specification and SDK artifacts
so the schema exposes the corresponding enum constraint.
In
`@plugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.md`:
- Around line 35-62: Move the environment conversion, FileSet upload, and GRPO
submission procedure from the hyperparameters reference into a dedicated GRPO
HOW-TO page. Add equivalent Python SDK and CLI examples there using tab sets,
while keeping this reference focused on hyperparameter information and linking
to the new HOW-TO. Preserve the documented environment, dataset, and sandbox
configuration requirements.
🪄 Autofix
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: 3687ec7b-d009-4acd-ba8c-78cb782bb73a
⛔ Files ignored due to path filters (15)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/python/nemo-platform/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/files/filesets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/files/fileset_purpose.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/environment_metadata_content.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared/fileset_metadata.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared_params/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared_params/environment_metadata_content.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/shared_params/fileset_metadata.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/files/test_filesets.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
k8s/helm/values.yamlopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/files/filesets.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/constants.pypackages/nmp_customization_common/src/nmp/customization_common/service/platform_client.pypackages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yamlpackages/nmp_customization_common/values/opensandbox/install.shpackages/nmp_customization_common/values/opensandbox/verify/lib.shplugins/nemo-customizer/openapi/openapi.yamlplugins/nemo-customizer/src/nemo_customizer/skills/nemo-customizer/references/hyperparameters-rl.mdplugins/nemo-rl/README.mdservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/rl/README.mdservices/rl/pyproject.tomlservices/rl/src/nmp/rl/app/constants.pyservices/rl/src/nmp/rl/app/jobs/compiler.pyservices/rl/src/nmp/rl/config.pyservices/rl/src/nmp/rl/schemas/job.pyservices/rl/src/nmp/rl/tasks/environment/__main__.pyservices/rl/src/nmp/rl/tasks/environment/bootstrap.pyservices/rl/src/nmp/rl/tasks/environment/convert.pyservices/rl/src/nmp/rl/tasks/environment/package.pyservices/rl/src/nmp/rl/tasks/environment/upload.pyservices/rl/src/nmp/rl/tasks/environment/validate.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_config.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.pyservices/rl/src/nmp/rl/tasks/training/backends/nemo_rl/sandbox_config.pyservices/rl/tests/test_compiler.pyservices/rl/tests/test_environment_package.pyservices/rl/tests/test_grpo_config.pyservices/rl/tests/test_sandbox_config.pyservices/rl/tests/test_schemas.pyweb/packages/studio/src/components/DatasetsTable/constants.ts
💤 Files with no reviewable changes (1)
- services/rl/src/nmp/rl/tasks/environment/package.py
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/nmp_customization_common/values/opensandbox/batchsandbox-template-crun.yaml
- services/rl/pyproject.toml
- services/rl/tests/test_sandbox_config.py
- services/rl/src/nmp/rl/app/jobs/compiler.py
- services/rl/src/nmp/rl/tasks/training/backends/nemo_rl/grpo_driver.py
- packages/nmp_customization_common/src/nmp/customization_common/service/platform_client.py
- services/rl/tests/test_compiler.py
- packages/nmp_customization_common/values/opensandbox/install.sh
- services/rl/src/nmp/rl/tasks/environment/main.py
- plugins/nemo-rl/README.md
- packages/nmp_customization_common/values/opensandbox/verify/lib.sh
- services/rl/src/nmp/rl/tasks/environment/validate.py
- services/rl/README.md
- packages/nemo_platform_plugin/src/nemo_platform_plugin/files/metadata.py
- services/rl/src/nmp/rl/tasks/environment/upload.py
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-1156-aalgo-418-solu.docs.buildwithfern.com/nemo-platform |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
third_party/osv-licenses.json (1)
2848-3106: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUpdate vulnerable
ragas.Line 2851 records
ragas==0.4.3. Both affected ranges include version 0.4.3. Upgrade to an unaffected version or removeragas, then regenerate this metadata. The current version contains an SSRF vulnerability.🤖 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 `@third_party/osv-licenses.json` around lines 2848 - 3106, The vulnerable ragas package entry is pinned to version 0.4.3, which is affected by both listed advisories. Upgrade ragas to an unaffected release or remove the package, then regenerate third_party/osv-licenses.json so both vulnerability records and their affected-version metadata reflect the updated dependency.
🤖 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 `@docs/cli/reference.mdx`:
- Line 1314: Update the CLI source that defines the --purpose choices to include
environment, then regenerate docs/cli/reference.mdx using the designated
documentation generation command; do not edit the generated reference page
directly.
In `@third_party/requirements-main.txt`:
- Around line 296-299: Run uv run pre-commit run -a for the updated dependency
entry and resolve every hook modification and type-checking error it reports,
leaving the repository with all pre-commit checks passing before merge.
---
Outside diff comments:
In `@third_party/osv-licenses.json`:
- Around line 2848-3106: The vulnerable ragas package entry is pinned to version
0.4.3, which is affected by both listed advisories. Upgrade ragas to an
unaffected release or remove the package, then regenerate
third_party/osv-licenses.json so both vulnerability records and their
affected-version metadata reflect the updated dependency.
🪄 Autofix
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: 816c5679-0bf9-4d40-af5f-89b5535d99ee
📒 Files selected for processing (4)
docs/cli/reference.mdxthird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txt
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 11, 2026 23:08
1f9c342 to
53f7449
Compare
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 12, 2026 23:23
a1d2daf to
b6031e2
Compare
albcui
reviewed
Aug 18, 2026
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 19, 2026 19:37
429152d to
2e92492
Compare
gabwow
requested changes
Aug 19, 2026
gabwow
left a comment
gabwow
left a comment
Contributor
There was a problem hiding this comment.
Right now I think users will be at a loss when trying use environments, or defining their own custom environments. Some of this is solvable in AALGO-439, but I've suggested some changes that might help here. Also can we move the validation logic of environments to plugins/nemo-rl/src/nemo_rl_plugin/transform.py to get signal earlier?
Contributor
|
LGTM |
gabwow
self-requested a review
August 20, 2026 15:52
gabwow
approved these changes
Aug 20, 2026
albcui
approved these changes
Aug 20, 2026
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 20, 2026 22:54
523eaaf to
3033b88
Compare
15 tasks
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 21, 2026 18:22
7b130e0 to
bb456e4
Compare
Signed-off-by: Sam Oluwalana <soluwalana@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
…fixes Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
anubhutivyas
force-pushed
the
aalgo-418/solu
branch
from
August 21, 2026 18:31
bb456e4 to
710d2e8
Compare
anubhutivyas
enabled auto-merge
August 21, 2026 18:36
anubhutivyas
disabled auto-merge
August 21, 2026 18:36
anubhutivyas
enabled auto-merge
August 21, 2026 18:37
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
4 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
Adds NeMo Platform GRPO training support with NeMo Gym environments (Mode B / OpenSandbox). Operators convert Prime Intellect hub envs offline via
pi-to-gym-conversionintoadapter-wheels-v1FileSets + Gym JSONL, upload them, and submit GRPO jobs; the compiler downloads the environment FileSet, emits sandboxed Gym host config with master-time egress allowlists, and runs the real GRPO driver instead of the Penguin stub.Related Issue
Changes
packages/nmp_customization_common/values/opensandbox/FilesetPurpose.ENVIRONMENT+EnvironmentMetadataContentfor environment packagespi-to-gym-conversion(nmp-rl): hub →adapter-wheels-v1+ Gym JSONL (requirespip+verifiers; fails closed if dataset generation yields 0 rows)GRPOTraining+ optionalenvironmenton RL job specs; environment manifests innmp.rl.schemas.environment/ job types innmp.rl.schemas.jobminimal_grpo.jsonfixture; contract testssandboxed_gym_default/sandbox_cluster_capablefail-closed, egress env placeholderssandbox_confighelpers, GRPO config compilation, replaced Penguingrpo_driverwith Gym GRPO + optional Mode A bootstrap / Mode B master egress fillservices/rlREADME, customizer hyperparams referencesType of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Not yet run in this PR validation pass:
uv run pre-commit run -a(not executed for this description update)nmp-test1(convert → upload → one-step GRPO with OpenSandbox); blocked on clusterNMP_BASE_URL/NMP_RL_SANDBOX_CLUSTER_CAPABLE=trueSummary by CodeRabbit