feat: Infer DSE parameter encoding from candidate values at parse time#966
feat: Infer DSE parameter encoding from candidate values at parse time#966KhushviB wants to merge 2 commits into
Conversation
KhushviB
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesEnvironment parameter encoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloudai/configurator/env_params.py`:
- Around line 146-156: Update the arithmetic and geometric-series checks in the
encoding-detection function to use scale-relative tolerances rather than the
fixed 1e-5 absolute comparison, so small values such as [1e-8, 1e-7, 1e-6] reach
LogEncoding() for their constant ×10 ratio. Add a regression test covering this
sequence and preserve the existing arithmetic and ratio direction checks.
- Around line 273-276: The EnvParams resolution path must validate explicit log
encoding candidates before constructing EnvParam, since `spec.encoding`
currently bypasses `_infer_encoding()`. In `EnvParams.from_test()`, reject zero,
negative, non-numeric, and non-finite values for explicit log overrides, while
preserving inferred-encoding behavior for unspecified encodings; add a
regression test covering the explicit override path.
🪄 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: 7dd06e9f-56dc-4ca7-91ae-9f0120ad7203
📒 Files selected for processing (2)
src/cloudai/configurator/env_params.pytests/test_env_params.py
There was a problem hiding this comment.
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/env_params.py (1)
180-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the stale default-value test.
EnvParamSpec.encodingnow correctly defaults toNone, buttests/test_env_params.py:415-417still assertsCategoricalEncoding(). That test will fail immediately; update it to assertis Noneand keep categorical compatibility covered throughEnvParams.from_test()inference.Proposed test fix
- assert EnvParamSpec().encoding == CategoricalEncoding() + assert EnvParamSpec().encoding is None🤖 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/env_params.py` around lines 180 - 182, Update the default-value assertion for EnvParamSpec.encoding in the relevant env-params test to expect None instead of CategoricalEncoding(). Preserve categorical compatibility coverage through EnvParams.from_test() inference rather than the direct default-value test.
🤖 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.
Outside diff comments:
In `@src/cloudai/configurator/env_params.py`:
- Around line 180-182: Update the default-value assertion for
EnvParamSpec.encoding in the relevant env-params test to expect None instead of
CategoricalEncoding(). Preserve categorical compatibility coverage through
EnvParams.from_test() inference rather than the direct default-value test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9f9349aa-b0fb-4e78-bcb1-d86af9d3bc79
📒 Files selected for processing (2)
src/cloudai/configurator/env_params.pytests/test_env_params.py
KhushviB
force-pushed
the
feature/env-params-encoding-inference
branch
from
e976078 to
9a70619
Compare
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 (2)
src/cloudai/configurator/env_params.py (2)
180-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale
EnvParamSpecdocstring.The class docstring still says omitted encoding defaults to a categorical index, but it now defaults to
Noneand is inferred from candidates.Proposed wording
- ``encoding`` (optional) selects how the drawn value is exposed to the policy as an observation leaf, defaulting to a categorical index. + ``encoding`` (optional) selects how the drawn value is exposed to the policy as an observation leaf; when omitted, it is inferred from candidates.Based on learnings, production classes should maintain meaningful and accurate docstrings.
🤖 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/env_params.py` around lines 180 - 182, Update the EnvParamSpec class docstring to state that an omitted encoding defaults to None and is inferred from candidates, replacing the stale categorical-index default description. Keep the rest of the docstring unchanged.Source: Learnings
138-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude
boolfrom inferred log encodings.Because
boolsubclassesint,[True, 2, 4]is classified as geometric and producesLogEncoding. Treat any boolean-containing candidate list as categorical, matching the explicit-log validation path, and add a regression test.Proposed fix
- if not all(isinstance(c, (int, float)) and c > 0 for c in candidates): + if not all( + isinstance(c, (int, float)) + and not isinstance(c, bool) + and c > 0 + for c in candidates + ): return CategoricalEncoding()🤖 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/env_params.py` around lines 138 - 139, Update the candidate validation in the inferred log-encoding logic to explicitly reject bool values before accepting numeric candidates, so any list containing booleans returns CategoricalEncoding. Keep valid int/float candidates eligible for geometric inference, and add a regression test covering a boolean-containing list such as [True, 2, 4].
🤖 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`:
- Around line 278-279: Update the candidate-type validation in the LogEncoding
configuration logic to raise TypeError, rather than ValueError, when a candidate
is a string, boolean, or otherwise non-numeric; update the corresponding
assertions in tests/test_env_params.py to expect TypeError while preserving
ValueError for other invalid-value cases.
---
Outside diff comments:
In `@src/cloudai/configurator/env_params.py`:
- Around line 180-182: Update the EnvParamSpec class docstring to state that an
omitted encoding defaults to None and is inferred from candidates, replacing the
stale categorical-index default description. Keep the rest of the docstring
unchanged.
- Around line 138-139: Update the candidate validation in the inferred
log-encoding logic to explicitly reject bool values before accepting numeric
candidates, so any list containing booleans returns CategoricalEncoding. Keep
valid int/float candidates eligible for geometric inference, and add a
regression test covering a boolean-containing list such as [True, 2, 4].
🪄 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: 7a21b3f7-deca-466a-9f32-be9d3a285d08
📒 Files selected for processing (2)
src/cloudai/configurator/env_params.pytests/test_env_params.py
KhushviB
force-pushed
the
feature/env-params-encoding-inference
branch
from
9a70619 to
6d4e258
Compare
1 participant
Summary (#956)
This pull request migrates DSE parameter encoding inference directly into the framework (
cloudai), moving it out of individual optimizer agents (e.g., BO agents).Previously, selecting a non-default encoding required an explicit
encoding = { type = "log" }in the configuration TOML, and only the BO agent internally detected log scales. This led to agent-specific framework logic that was not reused by GA, MAB, or RL agents.Changes introduced:
LogEncodingclass to encapsulateObsLeafDescriptorand log-scaling logic.AnyEncodingdiscriminated union to seamlessly parse explicit configuration types (categoricalorlog).EnvParamSpecto make theencodingfield optional._infer_encodinginenv_params.pyto automatically evaluatecmd_argscandidates at parse time. Inference rules safely distinguish between categorical values, arithmetic series (linear), and geometric series (log).EnvParams.from_testto apply when no explicit encoding is specified.These changes centralize parameter encoding logic, decouple it from optimizer agents, and make configuration easier for users.
Test Plan
Testing Environment:
Steps followed:
tests/test_env_params.pyto add targeted unit tests for the_infer_encodinglogic.CategoricalEncoding.[1, 10, 100]and[2, 4, 8, 16]) correctly evaluate toLogEncoding.[1, 2, 3]) are correctly detected and fallback toCategoricalEncoding.0.0or negative values correctly default toCategoricalEncoding.{ type = "log" }) safely bypasses the inference logic for edge cases.Results:
The
_infer_encodingheuristic strictly follows the mathematical properties of the candidate values and correctly creates the appropriate encoding object. The framework correctly defaults to inferred logic without crashing or overriding explicitly supplied TOML configurations.Additional Notes
CategoricalEncoding. The infrastructure is now properly set up to easily introduce a dedicatedLinearEncodingorOrdinalEncodingblock in the future if required._detect_log_scaleheuristic and directly consume the framework-decided Encoding.