feat(core): add ContinuousSpace action-space primitive#927
Open
rutayan-nv wants to merge 15 commits into
Open
Conversation
rutayan-nv
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
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 DSE environment-randomization support: a ChangesDSE Environment Randomization and Agent Execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cloudai/configurator/cloudai_gym.py (1)
145-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstraint-failure early return breaks
env.csv↔trajectory.csvstep alignment.Line 145 triggers
before_stepobservers (which persistenv.csv), but Line 168/Line 170 can return beforewrite_trajectory(...). With declaredenv_params, this creates one-sided env rows for failed trials and violates the step-alignment contract.Proposed fix
if not self.test_run.test.constraint_check(self.test_run, self.runner.system): logging.info("Constraint check failed. Skipping step.") - return [-1.0], self.rewards.constraint_failure, True, {} + observation = [-1.0] + reward = self.rewards.constraint_failure + self.write_trajectory( + TrajectoryEntry( + step=self.test_run.step, + action=action, + reward=reward, + observation=observation, + env_params=dict(self.test_run.current_env_params), + ) + ) + for observer in self.observers: + observer.after_step(self.test_run, observation, reward) + return observation, reward, True, {}🤖 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 `@src/cloudai/configurator/cloudai_gym.py` around lines 145 - 170, The issue is that the `before_step` observer (line 145) writes to env.csv, but when the constraint check fails (line 168), the method returns early without calling `write_trajectory`, creating a mismatch between env.csv and trajectory.csv rows. To fix this, when the constraint check fails, you must write a corresponding trajectory entry before returning. Create a TrajectoryEntry with the current step, action, the constraint failure reward, and current env_params (similar to how it's done in the cached result case), pass it to `write_trajectory`, and only then return the constraint failure response. This ensures every `before_step` call has a matching trajectory entry.
🤖 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 `@src/cloudai/configurator/env_params.py`:
- Line 2: The copyright headers in both files use a ranged year format
(2024-2026) that does not match the repository's CI expectations. Update the
copyright year in src/cloudai/configurator/env_params.py at line 2 by changing
the year range to a single year format of 2026. Apply the identical copyright
year correction to tests/test_env_params.py at line 2, also changing to the
single year 2026.
In `@src/cloudai/models/workload.py`:
- Around line 115-122: The new env_params field added to the Workload class is
not being checked in the TestDefinition.is_dse_job method, which means test
scenarios that use only environment parameter randomization could be incorrectly
classified as non-DSE jobs and skip the CloudAIGymEnv sampling flow. Update the
is_dse_job method to include a check for env_params alongside any existing DSE
parameter checks, ensuring that a job is classified as a DSE job if env_params
is non-empty.
In `@tests/test_action_space.py`:
- Around line 43-50: The test functions
test_continuous_space_rejects_unknown_dtype and
test_continuous_space_forbids_extra_fields are calling the ContinuousSpace
constructor directly with invalid arguments (dtype="double" and step=0.1), which
violates the type signature and blocks CI type checking. Refactor both test
functions to use the model_validate() class method instead, passing dictionaries
containing the invalid payloads as arguments. This approach will exercise the
runtime validation logic while maintaining static type safety, allowing the
tests to verify that ValidationError is raised for invalid inputs without
triggering pyright type errors.
In `@tests/test_cloudaigym.py`:
- Around line 567-615: Create a new test case (either as a separate test
function or within the existing test structure) that validates step alignment
between env.csv and trajectory.csv when a constraint_check failure occurs. Set
up a test scenario similar to test_env_csv_is_step_aligned_with_trajectory with
declared env_params, but configure or mock the environment so that
constraint_check fails during execution. After the steps complete, verify that
both env_steps and traj_steps remain aligned (same step values in the same
order) even after the constraint failure, ensuring the step alignment contract
holds for trials that hit constraint violations.
---
Outside diff comments:
In `@src/cloudai/configurator/cloudai_gym.py`:
- Around line 145-170: The issue is that the `before_step` observer (line 145)
writes to env.csv, but when the constraint check fails (line 168), the method
returns early without calling `write_trajectory`, creating a mismatch between
env.csv and trajectory.csv rows. To fix this, when the constraint check fails,
you must write a corresponding trajectory entry before returning. Create a
TrajectoryEntry with the current step, action, the constraint failure reward,
and current env_params (similar to how it's done in the cached result case),
pass it to `write_trajectory`, and only then return the constraint failure
response. This ensures every `before_step` call has a matching trajectory entry.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: b6c39d36-ea91-40c3-ac4f-5345ef1e77f6
📒 Files selected for processing (13)
src/cloudai/_core/action_space.pysrc/cloudai/_core/test_scenario.pysrc/cloudai/cli/handlers.pysrc/cloudai/configurator/base_agent.pysrc/cloudai/configurator/cloudai_gym.pysrc/cloudai/configurator/env_params.pysrc/cloudai/core.pysrc/cloudai/models/workload.pytests/test_action_space.pytests/test_cloudaigym.pytests/test_env_params.pytests/test_handlers.pytests/test_test_scenario.py
This was referenced Jun 16, 2026
…ole mutator The trial index lives on TestRun and is advanced exactly once per CloudAIGymEnv.step(), before output_path and the trajectory row are computed. Step artifacts and trajectory rows are therefore tagged with the advanced index rather than the pre-step value.
handle_dse_job calls agent.run() uniformly; agents (e.g. RLlib-based) implement their own training loop in run(). Failure handling follows a hybrid contract: a non-zero rc accumulates via `err |= rc` and the sweep continues, while an unexpected exception hard-fails the scenario after reports are generated and the aborting error is documented.
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
c291ba4 to
4e52bc0
Compare
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)
src/cloudai/configurator/base_agent.py (1)
90-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType contract mismatch:
select_actionsignature doesn't allowNonereturn.The abstract method signature declares
tuple[int, dict[str, Any]]as the return type (line 91), butrun()handlesNoneas an early-termination signal (lines 143-145). This breaks the type contract and will cause static analysis issues for subclasses that want to returnNone.Update the return type to reflect this:
🔧 Proposed fix
`@abstractmethod` -def select_action(self, observation: list[float] | None = None) -> tuple[int, dict[str, Any]]: +def select_action(self, observation: list[float] | None = None) -> tuple[int, dict[str, Any]] | None: """ Select an action from the action space. Args: observation: Latest observation produced by the environment (``env.reset()`` on the first call, then the result of the prior ``env.step()``). Stateless agents such as grid search or Bayesian optimization may ignore this; observation-conditioned agents (RL, contextual bandits) should use it. Returns: - Tuple[int, Dict[str, Any]]: The current step index and a dictionary mapping action keys to selected values. + Tuple[int, Dict[str, Any]] | None: The current step index and a dictionary mapping + action keys to selected values, or None to signal early termination. """ passAlso applies to: 143-146
🤖 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 `@src/cloudai/configurator/base_agent.py` around lines 90 - 104, The abstract method `select_action` declares a return type of `tuple[int, dict[str, Any]]` but the `run()` method handles `None` as a valid return value for early termination. Update the return type annotation of the `select_action` method to allow `None` by changing it to `tuple[int, dict[str, Any]] | None` to correctly reflect the actual contract and prevent type checking errors in subclasses.
♻️ Duplicate comments (2)
src/cloudai/models/workload.py (1)
115-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
env_paramsis not integrated intois_dse_jobdetection.The
is_dse_jobproperty (lines 148-159) only checkscmd_args_dictandextra_env_varsfor list values. A workload that declares onlyenv_params(no list-valued cmd_args) would returnis_dse_job = False, potentially bypassing theCloudAIGymEnvstep/sampling flow.🔧 Proposed fix
`@property` def is_dse_job(self) -> bool: def check_dict(d: dict, parent_key: str = "") -> bool: if isinstance(d, dict): for key, value in d.items(): path = f"{parent_key}.{key}" if parent_key else key if self.is_dse_excluded_arg(path): continue if isinstance(value, list) or (isinstance(value, dict) and check_dict(value, path)): return True return False - return check_dict(self.cmd_args_dict) or check_dict(self.extra_env_vars) + return check_dict(self.cmd_args_dict) or check_dict(self.extra_env_vars) or bool(self.env_params)🤖 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 `@src/cloudai/models/workload.py` around lines 115 - 122, The `is_dse_job` property does not check for values in the newly added `env_params` field when determining whether a workload is a DSE job. Update the `is_dse_job` property (which currently checks only `cmd_args_dict` and `extra_env_vars` for list values) to also include a check for whether `env_params` is non-empty. This ensures that workloads declaring only `env_params` without list-valued `cmd_args` will be correctly identified as DSE jobs and properly routed through the `CloudAIGymEnv` sampling flow.tests/test_action_space.py (1)
43-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch invalid-input tests to
model_validate()payloads to unblock pyright.Line 45 and Line 50 currently call
ContinuousSpace(...)with arguments that intentionally violate the constructor signature; this is causing the CI pyright failure. Keep the negative-runtime validation intent, but feed invalid payloads throughmodel_validate({...})instead.✅ Minimal patch
def test_continuous_space_rejects_unknown_dtype() -> None: with pytest.raises(ValidationError): - ContinuousSpace(low=0.0, high=1.0, dtype="double") + ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "dtype": "double"}) @@ def test_continuous_space_forbids_extra_fields() -> None: with pytest.raises(ValidationError): - ContinuousSpace(low=0.0, high=1.0, step=0.1) + ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "step": 0.1})#!/bin/bash # Read-only verification: confirm the two constructor calls that trigger pyright. rg -n 'ContinuousSpace\(low=0\.0,\s*high=1\.0,\s*dtype="double"\)|ContinuousSpace\(low=0\.0,\s*high=1\.0,\s*step=0\.1\)' tests/test_action_space.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 `@tests/test_action_space.py` around lines 43 - 50, Replace the direct constructor calls to ContinuousSpace that use invalid arguments with model_validate() calls instead. In test_continuous_space_rejects_unknown_dtype() at line 45, change ContinuousSpace(low=0.0, high=1.0, dtype="double") to ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "dtype": "double"}). In test_continuous_space_forbids_extra_fields() at line 50, change ContinuousSpace(low=0.0, high=1.0, step=0.1) to ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "step": 0.1}). This preserves the negative-input validation testing intent while avoiding pyright type-checking errors.Source: Pipeline failures
🤖 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 `@tests/test_env_params.py`:
- Line 2: The copyright header in tests/test_env_params.py on line 2 uses a year
range format (2025-2026) which fails the repository's copyright header
validation. Update the copyright year to use the single-year format (2026) as
expected by the tests/test_check_copyright_headers.py validation rules. Change
the year range in the copyright line from "2025-2026" to just "2026".
---
Outside diff comments:
In `@src/cloudai/configurator/base_agent.py`:
- Around line 90-104: The abstract method `select_action` declares a return type
of `tuple[int, dict[str, Any]]` but the `run()` method handles `None` as a valid
return value for early termination. Update the return type annotation of the
`select_action` method to allow `None` by changing it to `tuple[int, dict[str,
Any]] | None` to correctly reflect the actual contract and prevent type checking
errors in subclasses.
---
Duplicate comments:
In `@src/cloudai/models/workload.py`:
- Around line 115-122: The `is_dse_job` property does not check for values in
the newly added `env_params` field when determining whether a workload is a DSE
job. Update the `is_dse_job` property (which currently checks only
`cmd_args_dict` and `extra_env_vars` for list values) to also include a check
for whether `env_params` is non-empty. This ensures that workloads declaring
only `env_params` without list-valued `cmd_args` will be correctly identified as
DSE jobs and properly routed through the `CloudAIGymEnv` sampling flow.
In `@tests/test_action_space.py`:
- Around line 43-50: Replace the direct constructor calls to ContinuousSpace
that use invalid arguments with model_validate() calls instead. In
test_continuous_space_rejects_unknown_dtype() at line 45, change
ContinuousSpace(low=0.0, high=1.0, dtype="double") to
ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "dtype": "double"}). In
test_continuous_space_forbids_extra_fields() at line 50, change
ContinuousSpace(low=0.0, high=1.0, step=0.1) to
ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "step": 0.1}). This
preserves the negative-input validation testing intent while avoiding pyright
type-checking errors.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 1bc10229-4b90-4cdf-84d0-690819822eea
📒 Files selected for processing (13)
src/cloudai/_core/action_space.pysrc/cloudai/_core/test_scenario.pysrc/cloudai/cli/handlers.pysrc/cloudai/configurator/base_agent.pysrc/cloudai/configurator/cloudai_gym.pysrc/cloudai/configurator/env_params.pysrc/cloudai/core.pysrc/cloudai/models/workload.pytests/test_action_space.pytests/test_cloudaigym.pytests/test_env_params.pytests/test_handlers.pytests/test_test_scenario.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
4e52bc0 to
5245d81
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_action_space.py (1)
43-50: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace
# type: ignoreconstructor negatives with payload-based validation calls.Line 45 and Line 50 currently bypass static typing. Use
ContinuousSpace.model_validate({...})to keep the negative-runtime checks while preserving type safety in tests.Suggested patch
def test_continuous_space_rejects_unknown_dtype() -> None: with pytest.raises(ValidationError): - ContinuousSpace(low=0.0, high=1.0, dtype="double") # type: ignore + ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "dtype": "double"}) def test_continuous_space_forbids_extra_fields() -> None: with pytest.raises(ValidationError): - ContinuousSpace(low=0.0, high=1.0, step=0.1) # type: ignore + ContinuousSpace.model_validate({"low": 0.0, "high": 1.0, "step": 0.1})🤖 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 `@tests/test_action_space.py` around lines 43 - 50, Replace the direct ContinuousSpace constructor calls that use `# type: ignore` comments with `ContinuousSpace.model_validate({...})` calls in both test_continuous_space_rejects_unknown_dtype and test_continuous_space_forbids_extra_fields functions. Instead of passing parameters directly to the constructor with type ignore comments, pass them as a dictionary to model_validate to maintain type safety while still allowing the negative test cases to validate the runtime behavior.tests/test_cloudaigym.py (1)
567-615: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a constraint-failure alignment regression beside this step-alignment test.
This block validates alignment for successful/cache trials, but it still doesn’t pin the declared-
env_paramspath whenconstraint_checkfails. Add a case assertingenv.csvandtrajectory.csvremain step-aligned in that failure branch as well.🤖 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 `@tests/test_cloudaigym.py` around lines 567 - 615, Add a regression test that validates env.csv and trajectory.csv remain step-aligned when constraint_check fails. Create a new test case (or extend the existing test_env_csv_is_step_aligned_with_trajectory) that simulates a constraint check failure scenario, then verify that the step alignment between env.csv and trajectory.csv is preserved even in this failure branch. This ensures the declared env_params contract holds for both successful execution paths and constraint-failure paths.
🤖 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 `@tests/test_env_params.py`:
- Around line 86-87: The pytest.raises(ValueError) assertion is too broad and
may mask unrelated failures. Replace the broad ValueError check with a more
specific assertion that matches the exact step-validation error message that
should be raised when sink.write is called with step 0 and drop_rate 0.0. Use
pytest.raises with the match parameter to verify the specific error message
expected from step validation.
---
Duplicate comments:
In `@tests/test_action_space.py`:
- Around line 43-50: Replace the direct ContinuousSpace constructor calls that
use `# type: ignore` comments with `ContinuousSpace.model_validate({...})` calls
in both test_continuous_space_rejects_unknown_dtype and
test_continuous_space_forbids_extra_fields functions. Instead of passing
parameters directly to the constructor with type ignore comments, pass them as a
dictionary to model_validate to maintain type safety while still allowing the
negative test cases to validate the runtime behavior.
In `@tests/test_cloudaigym.py`:
- Around line 567-615: Add a regression test that validates env.csv and
trajectory.csv remain step-aligned when constraint_check fails. Create a new
test case (or extend the existing test_env_csv_is_step_aligned_with_trajectory)
that simulates a constraint check failure scenario, then verify that the step
alignment between env.csv and trajectory.csv is preserved even in this failure
branch. This ensures the declared env_params contract holds for both successful
execution paths and constraint-failure paths.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 1af5e41b-bb3a-409b-a00f-58669b8a911c
📒 Files selected for processing (9)
src/cloudai/_core/action_space.pysrc/cloudai/_core/test_scenario.pysrc/cloudai/configurator/cloudai_gym.pysrc/cloudai/configurator/env_params.pysrc/cloudai/core.pysrc/cloudai/models/workload.pytests/test_action_space.pytests/test_cloudaigym.pytests/test_env_params.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
5245d81 to
8a0e03d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloudai/cli/handlers.py`:
- Around line 324-328: The validate_dse_env_params function call in the try
block currently validates env_params only against tr.is_dse_job, but does not
account for the --single-sbatch flag which routes execution through
handle_non_dse_job where CloudAIGymEnv sampling is not used. This allows
env_params to pass validation and then be silently ignored at runtime. Enhance
the validation logic to also check if --single-sbatch mode is enabled, and if
so, reject any env_params specification by raising a TestScenarioParsingError to
explicitly block this unsupported combination at validation time rather than
failing silently.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 3934bf03-5c1d-4d10-a50e-1ec080b51156
📒 Files selected for processing (5)
src/cloudai/_core/action_space.pysrc/cloudai/cli/handlers.pysrc/cloudai/core.pytests/test_action_space.pytests/test_handlers.py
run() already treats a None return from select_action() as a stop signal, but the abstract method declared only tuple[int, dict[str, Any]]. Widen the return annotation to tuple[int, dict[str, Any]] | None and document the termination semantics so the declared contract matches run()'s usage.
CloudAIGymEnv.get_cached_trajectory_result(action) keys the trajectory cache on action alone. When a workload declares env_params (e.g. drop_rate) and the agent re-selects the same action under a different env_params sample, the cache returns a stale reward measured under a different env, silently invalidating any domain-randomization workflow. Four tests pin the contract; today two FAIL (bug-exposing), two PASS (back-compat sanity). All 27 pre-existing tests are untouched. FAIL test_cache_miss_when_env_params_differ FAIL test_step_reruns_workload_when_env_params_change PASS test_cache_hit_when_action_and_env_params_match PASS test_cache_hit_when_neither_has_env_params Driver for the follow-up PR that adds env_params as a first-class field on TestDefinition + TrajectoryEntry and rewrites the cache key as (action, env_params). Both unit and integration shapes are covered so the fix can be validated end-to-end through env.step().
…ge signature Satisfy ruff-format on the env_params cache-key TDD test.
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
8a0e03d to
65929ca
Compare
CodeRabbit (Ruff PT018): `assert a and b` obscures which clause failed. Split the two `assert result is not None and result.step == 1` checks in the env_params cache-key tests into separate assertions for clearer pytest introspection. Test-only change, no behavior impact.
Domain randomization broke under the trajectory cache because the cache key was action-only: an agent revisiting the same action under a different env_params sample (e.g. drop_rate) got the *first* trial's reward served back, silently corrupting the reward signal. env.csv was also workload-owned and skipped on cache hits, so it was sparse and mis-aligned with trajectory.csv. This change makes env_params first-class in cloudai so the design works the same way for every agent (PPO, BO, GA, MAB) and every workload: - TestDefinition.env_params: dict[str, EnvParamSpec] - sibling to cmd_args, not part of the action space. - TestRun.current_env_params: dict[str, Any] - per-trial sample attached by the env so the cache key and downstream sinks see it. - TrajectoryEntry gains env_params; the cache key is now (action, current_env_params). - CloudAIGymEnv builds an EnvParamsObserver when env_params is declared. before_step() samples deterministically by (seed, name, trial), stashes the sample on test_run, and appends to env.csv *before* the cache lookup - so env.csv is written on every trial (cache hit or miss) and aligns 1:1 with trajectory.csv by step. - Workloads with no [env_params.*] block pay zero overhead: no observer, no env.csv, identical behaviour to today. Tests: turns the four TDD-red tests from the parent PR green and adds nine unit tests for EnvParamSpec / EnvParamsSampler / CsvSink / EnvParamsObserver plus two integration tests pinning the env.csv ↔ trajectory.csv contract.
Closes the most important gap in the env_params test matrix: the
cache-HIT path under an observer-driven TestDefinition.env_params.
The previous tests covered each axis separately - cache key unit tests
for env_params, a step()-level cache-miss integration with manually
set current_env_params, and a step-alignment test where all trials
were cache misses. None of them proved that a cache HIT still drives
the observer (and therefore env.csv) - which is precisely the
sparsity contract the fix was about.
The new test:
- declares env_params on the TestDefinition (real observer wired in),
- pre-seeds the trajectory with an entry whose action AND deterministic
env_params sample match what the observer will produce for step 1,
- calls env.step() once and asserts:
(a) runner.run is NOT called (cache short-circuit honored),
(b) env.csv gains a row (observer fired before the cache lookup),
(c) the new trajectory entry carries the per-trial env_params.
This is the end-to-end proof that the env.csv sparsity bug cannot
recur, and that env.csv ↔ trajectory.csv stay step-aligned on the
cache-hit path too.
…s files env_params.py and test_env_params.py were first authored in 2026; the copyright-header check derives the expected year from git history.
env_params are sampled only during DSE (by CloudAIGymEnv); on a non-DSE run they would be silently ignored. is_dse_job is a property of the fully prepped config, so validate it there rather than at parse time. Add validate_dse_env_params(test_scenario), which raises when a test run declares env_params while is_dse_job is False. Enforce it at run/dry-run dispatch (handle_dry_run_and_run, fail fast after setup) and mandatorily in verify_test_scenarios, so a config declaring env_params without a DSE sweep (list-valued cmd_args/extra_env_vars or num_nodes) is rejected instead of silently dropped.
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
65929ca to
d60ad62
Compare
Sink env.csv from the same TrajectoryEntry as trajectory.csv inside
write_trajectory, instead of eagerly in the observer's before_step. A
constraint-failed step returns before write_trajectory, so it now records
no row in either file and the two stay step-aligned (a downstream
pd.merge(traj, env, on="step") loses no rows).
Also restore current_env_params after the run-result fallback in step():
that branch rebuilt test_run from the pristine copy and reset the sampled
env_params to {}, recording empty env_params in the trajectory entry and
weakening the (action, env_params) cache key.
EnvParamsObserver is reduced to sample-and-stash; CloudAIGymEnv owns
persistence of both files.
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
d60ad62 to
23558cd
Compare
random.choices treats inf/nan weights inconsistently (they slip past the existing non-negative + positive-sum checks), so validate finiteness in EnvParamSpec and reject inf/nan/-inf. Also add a match= to the CsvSink zero-step pytest.raises for clarity (CodeRabbit, NVIDIA#901/NVIDIA#928).
Introduce ContinuousSpace, a closed-interval [low, high] action-space
dimension with a dtype ("int" | "float") that declares how consumers
should quantize decoded samples. CloudAI action spaces can now express
continuous tunables alongside discrete (list) candidate domains; agents
and adapters read low/high/dtype to build their own representation.
Validated low < high; exported via cloudai.core.
…ejection tests These negative tests pass invalid dtype/extra kwargs to assert pydantic ValidationError; mark the deliberate type violations with type: ignore.
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
23558cd to
7c3c7b1
Compare
GymnasiumAdapter._decode_continuous clamps to [low, high] then rounds, so a dtype="int" space with non-integer bounds (e.g. [0.2, 0.8]) can decode to a value outside the declared range (1). Validate that int-quantized spaces declare integer bounds (CodeRabbit, NVIDIA#932/NVIDIA#927).
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.
Issue
Fix
ContinuousSpace(low, high, dtype="int"|"float")incloudai/_core/action_space.py(validateslow < high), exported viacloudai.core. Consumers (agents,GymnasiumAdapter) readlow/high/dtypeto build their representation and quantize decoded samples.Testing
tests/test_action_space.py(5 tests): defaults, int-bound coercion,low < highrejection, unknown-dtype rejection, extra-field rejection. ruff + pyright + vulture clean.Stack:
main← #893 ← #900 ← #901 ← this (first of the gymnasium-adapter upstreaming). Independent of #901 in content; stacked for linear history.