feat(core): add ContinuousSpace action-space primitive#927
Draft
rutayan-nv wants to merge 17 commits into
Draft
Conversation
rutayan-nv
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds env-parameter sampling and validation support, wires env_params into DSE execution and CloudAIGymEnv caching/trajectory writing, adds CLI validation plus failure reporting, and exports a new ContinuousSpace action-space model. ChangesDSE Environment Randomization and Execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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
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
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
4 times, most recently
from
23558cd to
7c3c7b1
Compare
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 17, 2026
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).
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 20, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
017842c to
ea9af74
Compare
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 21, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
ea9af74 to
79854c6
Compare
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 21, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
720b16a to
852569a
Compare
Contributor
Author
|
Parking this PR as draft pending a continuous-tunable use case. The ContinuousSpace primitive and the downstream GymnasiumAdapter plumbing are complete and tested, but the producer side (TestRun.param_space) does not yet surface ContinuousSpace, and no current RL workload needs it: the aiconfigurator action space is fully discrete (parallelism degrees, worker counts, batch sizes). Will revive and complete the param_space wiring when a workload with continuous tunables arrives. |
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 `@tests/test_action_space.py`:
- Line 33: The combined type check in the action space test triggers PT018 and
hides which side fails, so split the isinstance assertion in the test around
space.low and space.high into two separate asserts for clearer failure output.
Update the assertion in the test that references space.low and space.high so
each type check stands alone.
🪄 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: 24222cf4-ba61-4cd1-a40c-f0dd56e67b71
📒 Files selected for processing (5)
src/cloudai/_core/action_space.pysrc/cloudai/core.pysrc/cloudai/models/workload.pytests/test_action_space.pytests/test_env_params.py
| def test_continuous_space_coerces_int_bounds_and_keeps_int_dtype() -> None: | ||
| space = ContinuousSpace(low=0, high=200, dtype="int") | ||
| assert space.dtype == "int" | ||
| assert isinstance(space.low, float) and isinstance(space.high, float) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Split the combined type assertion for clearer failure output.
Line 33 triggers PT018 and makes failures less precise; use two asserts.
Suggested patch
- assert isinstance(space.low, float) and isinstance(space.high, float)
+ assert isinstance(space.low, float)
+ assert isinstance(space.high, float)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert isinstance(space.low, float) and isinstance(space.high, float) | |
| assert isinstance(space.low, float) | |
| assert isinstance(space.high, float) |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 33-33: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 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` at line 33, The combined type check in the action
space test triggers PT018 and hides which side fails, so split the isinstance
assertion in the test around space.low and space.high into two separate asserts
for clearer failure output. Update the assertion in the test that references
space.low and space.high so each type check stands alone.
Source: Linters/SAST tools
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 25, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
852569a to
ba39c51
Compare
Contributor
Author
|
Parking this PR for now. Per a change in release plan, this work isn't needed for the upcoming aiconfigurator release, so I'm moving it to draft to keep it out of the review queue. @podkidyshev — no need to review this one yet; I'll mark it ready again when it's scheduled for release. Thanks! |
Contributor
Author
|
Heads-up for when this is unparked: the GymnasiumAdapter (#930) previously imported |
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 26, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
ba39c51 to
fb2c239
Compare
Make env_params a first-class part of CloudAIGymEnv trial identity so the trajectory cache keys on (action, env_params) rather than action alone, fixing the domain-randomization correctness bug where the same action under a different env_params sample returned a stale reward. - Cache key now includes env_params; cache-key tests pin the contract (formerly the TDD-red specs of NVIDIA#900, folded in here). - Keep env.csv and trajectory.csv 1:1 step-aligned: a single TrajectoryEntry sinks both files coherently, including on constraint failure. - Reject env_params on non-DSE jobs; reject non-finite / negative weights. - Add cache-hit + declared-env_params integration coverage. Folds the test-only PR NVIDIA#900 (cache-key TDD) into this PR so the stack has no permanently-red standalone PR.
…rom search space Make env_params a thin annotation over cmd_args fields instead of a holder of candidate values. Candidate values live in cmd_args (the single source of truth, exactly like an action-space dimension); env_params.<name> only marks a field as env-sampled and carries optional sampling weights, never the values. - EnvParamSpec drops `values`; validates weights (finite, non-negative, sum=1.0). - Sampler/observer resolve candidate lists from cmd_args; scalar knobs are no-ops. - TestDefinition.validate_env_params cross-checks annotations against cmd_args (key must be a real field; weights require a list and must match its length). - Exclude env_params keys from both param_space and is_dse_job: an env-sampled list is not a search dimension, so an env-params-only workload is not a DSE job. - validate_dse_env_params rejects env_params on non-DSE runs and on grid_search (exhaustive search cannot exploit per-trial randomization). - Scrub private-implementation references from public docstrings. - Unit tests use generic Atari Breakout semantics (ball_speed / paddle_width).
…pyright - validate_env_params: reject structured (non-leaf) cmd_args targets. The observer cannot sample them, yet param_space/is_dse_job exclude the whole key, which would silently drop nested action dimensions. - CloudAIGymEnv.write_trajectory: rebind the env.csv sink to the current iteration path before each write, so env.csv stays 1:1 aligned with trajectory.csv when the env instance is reused across iterations. - test_env_params: assert the unknown-field rejection via model_validate so the negative test no longer trips pyright's call-arg check (CI Linting fix); add a structured-target rejection test.
An unweighted env_params spec skipped the candidate-list check, so an empty cmd_args list (e.g. ball_speed = []) passed validation and only failed later in EnvParamsSampler.sample() via rng.choice([]) (IndexError). Guard against an empty candidate list in validate_env_params so the error surfaces at TestDefinition build time. Addresses CodeRabbit feedback.
…ms value objects Replace the EnvParamsSampler class and the StepObserver/EnvParamsObserver indirection with two frozen dataclasses: EnvParam (one resolved knob: candidates, optional weights, single draw) and EnvParams (per-run knobs + seed, built via from_test, sampled per trial). The sampling RNG lives in the env: step() draws this trial's values and hands concrete values to TestRun.apply_params_set(action, env_params=...), which overlays action and sample through one deterministic path. Centralize the cmd_args -> env_params lookup in TestDefinition.is_env_sampled and access current_env_params directly. Expand EnvParam/EnvParams unit tests to cover draw, from_test, sample, and immutability.
Drop the EnvParamsSink Protocol + CsvSink pair (and runtime_checkable) for a single concrete EnvParamsSink, built unconditionally in CloudAIGymEnv. The sink is now stateless: write() takes the record path per call and skips empty samples, so non-DR runs write nothing and write_trajectory needs no branch. Derive both records from a new iteration_dir property and expose the env record via the env_params_record_path property (was _env_csv_path), keeping env.csv and trajectory.csv step-aligned without coupling the name to CSV.
…ty flag Replace the hardcoded `agent == "grid_search"` check with a BaseAgent.samples_env_params capability flag (opt-in, defaults False). Only agents whose search consumes per-trial env_params sampling set it True; enumerating/surrogate agents leave it False, so a config that declares env_params for an agent that would ignore them is rejected up front instead of silently no-op'ing. New agents answer for themselves with no string to maintain. Relocate validate_dse_env_params out of the CLI handlers into configurator/env_params.py next to the logic it guards, looking the agent up via the Registry. Unknown agents are deferred to the dedicated agent-resolution error rather than masked here. Keep all public-facing comments, docstrings, and the error message generic (no internal agent names). Cover the full validator matrix, including the unknown-agent deferral.
Compress multi-line inline comments down to the single non-obvious rationale (or drop them where the code already speaks), per the self-documenting-code principle. Public API docstrings and test intent comments are left intact.
apply_params_set overlays sampled scalar draws onto cmd_args, then reconstructs the TestDefinition to validate the applied action values. That pass re-ran validate_env_params, which rejects a weighted env_param whose cmd_args target is no longer a candidate list - exactly what the overlay produces. env_params is already validated at parse time, so drop it from the validation-only dump. Adds a regression test covering a weighted env_param's scalar draw.
An env_params entry only reclassifies a list-valued cmd_args sweep as env-sampled; a scalar is already fixed, so annotating it is a meaningless label. Previously such an annotation was tolerated as a silent no-op, which let it slip through parse-time validation and inconsistently trip (or not) the downstream "no agent will sample them" check depending on run mode. Reject it where the contract lives - TestDefinition.validate_env_params - so the failure is immediate and mode-independent. EnvParams.from_test's non-list guard becomes defensive (parse-time now guarantees lists); the post-overlay path already drops env_params before re-validating, so concrete scalar draws are unaffected. Extract the per-field checks into a helper to keep the validator under the complexity limit, and update tests: scalar annotations now assert rejection instead of no-op tolerance.
…edicates Drop TestRun.current_env_params and the restore line in CloudAIGymEnv.step; the per-trial draw is now a local threaded explicitly into the cache lookup and TrajectoryEntry, mirroring action. Adds TestDefinition .is_domain_randomization_enabled (declared?) and TestRun .is_domain_randomization_active (will actually sample?), and renames validate_dse_env_params -> validate_domain_randomization_active to flag declared-but-inactive domain randomization. Also collapses EnvParamsSink into a write_env_params function and tidies apply_params_set.
registry.py has no runtime cloudai imports (all under TYPE_CHECKING), so the function-local import in is_domain_randomization_active was unnecessary. Move it to the top with the other _core imports.
A single-element list (cmd_args.<name> == [x]) has the same degenerate semantics as the scalar case already rejected in 8f91664: EnvParams.sample() can only ever return x, while the rest of the flow treats the run as domain-randomized. Move the >= 2 candidate guard ahead of the weights short-circuit so it applies to every env-sampled field, not just weighted ones.
rutayan-nv
added a commit
to rutayan-nv/cloudai
that referenced
this pull request
Jun 29, 2026
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
fb2c239 to
606bf06
Compare
…rts_variable_environment The flag declares that an agent operates over a variable (non-stationary) environment that changes per trial. "sampling" was an implementation detail (env_params domain randomization); curriculum learning also varies the env without sampling. The broader name keeps the same infra usable for both. No behavior change - pure rename of the opt-in capability flag and its references.
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.
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).
rutayan-nv
force-pushed
the
rpatro/action-space-continuous
branch
from
606bf06 to
b1cf51c
Compare
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.