feat: support fractional CPU limits in operation resources - #34
Merged
Conversation
- Updated the `cpu_limit` parameter to accept fractional values in configuration files and code, enhancing flexibility for resource allocation. - Revised related documentation to clarify the usage of fractional CPU limits. - Added unit tests to validate acceptance and rejection of various CPU limit inputs, ensuring robust error handling and compliance with new specifications.
Reviewer's GuideAdds support for fractional CPU limits in operation resources by relaxing cpu_limit validation to positive numbers (including floats), updating config parsing to use floats, extending tests to cover fractional and invalid values, and updating documentation to describe the new behavior. Sequence diagram for fractional cpu_limit parsing and validationsequenceDiagram
actor User
participant Config
participant Common as operations_common
participant Resources as OperationResources
participant Validator as _require_positive_cpu_limit
User->>Common: extract_operation_resources
Common->>Config: resources_config["cpu_limit"]
Common->>Common: _get_config_value_with_default
Common->>Common: _float_from_config_value
Common->>Resources: OperationResources(cpu_limit)
Resources->>Resources: __post_init__
Resources->>Validator: _require_positive_cpu_limit(cpu_limit)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
OperationResourcesdocstring now states that a non-positivecpu_limitraisesValueError, but_require_positive_cpu_limitraisesTypeErrorfor non-numeric (e.g., bool) inputs; consider aligning the documented and actual exception types or clarifying this distinction in the docstring. - Since
_require_positive_cpu_limitexplicitly guards against non-finite values withmath.isfinite, it would be useful to add tests coveringfloat('inf'),float('-inf'), andfloat('nan')to validate the intended behavior for these edge cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `OperationResources` docstring now states that a non-positive `cpu_limit` raises `ValueError`, but `_require_positive_cpu_limit` raises `TypeError` for non-numeric (e.g., bool) inputs; consider aligning the documented and actual exception types or clarifying this distinction in the docstring.
- Since `_require_positive_cpu_limit` explicitly guards against non-finite values with `math.isfinite`, it would be useful to add tests covering `float('inf')`, `float('-inf')`, and `float('nan')` to validate the intended behavior for these edge cases.
## Individual Comments
### Comment 1
<location path="yt_framework/operations/common.py" line_range="41-42" />
<code_context>
return int(cast("Any", value))
+def _float_from_config_value(value: object) -> float:
+ return float(cast("Any", value))
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Boolean config values are now silently accepted as cpu_limit, bypassing the bool check in `_require_positive_cpu_limit`.
With `_float_from_config_value`, `true`/`false` from config become `1.0`/`0.0` and then pass `_require_positive_cpu_limit`, since the bool check only happens after conversion. Direct construction `OperationResources(cpu_limit=True)` still raises, so config vs constructor behavior diverges. To keep semantics consistent and reject bools in both paths, consider explicitly disallowing bools in `_float_from_config_value` (or using a helper that preserves `_require_positive_cpu_limit`’s type check).
</issue_to_address>
### Comment 2
<location path="tests/test_common.py" line_range="47-53" />
<code_context>
assert res.cpu_limit == 8
+def test_extract_operation_resources_parses_fractional_cpu_limit() -> None:
+ cfg = OmegaConf.create({"cpu_limit": 0.5})
+ res = extract_operation_resources(cfg, _LOG)
+ assert res.cpu_limit == 0.5
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for fractional cpu_limit inside the nested `resources` block as well.
Since `extract_operation_resources` also reads `resources.cpu_limit`, please add a sibling test using `OmegaConf.create({"resources": {"cpu_limit": 0.5}})` and asserting the extracted `cpu_limit` is `0.5`, to confirm nested configs follow the same semantics.
```suggestion
def test_extract_operation_resources_parses_fractional_cpu_limit() -> None:
cfg = OmegaConf.create({"cpu_limit": 0.5})
res = extract_operation_resources(cfg, _LOG)
assert res.cpu_limit == 0.5
def test_extract_operation_resources_parses_fractional_cpu_limit_in_nested_resources() -> None:
cfg = OmegaConf.create({"resources": {"cpu_limit": 0.5}})
res = extract_operation_resources(cfg, _LOG)
assert res.cpu_limit == 0.5
def test_extract_operation_resources_reads_nested_resources_block() -> None:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+41
to
+42
| def _float_from_config_value(value: object) -> float: | ||
| return float(cast("Any", value)) |
There was a problem hiding this comment.
issue (bug_risk): Boolean config values are now silently accepted as cpu_limit, bypassing the bool check in _require_positive_cpu_limit.
With _float_from_config_value, true/false from config become 1.0/0.0 and then pass _require_positive_cpu_limit, since the bool check only happens after conversion. Direct construction OperationResources(cpu_limit=True) still raises, so config vs constructor behavior diverges. To keep semantics consistent and reject bools in both paths, consider explicitly disallowing bools in _float_from_config_value (or using a helper that preserves _require_positive_cpu_limit’s type check).
Comment on lines
+47
to
53
| def test_extract_operation_resources_parses_fractional_cpu_limit() -> None: | ||
| cfg = OmegaConf.create({"cpu_limit": 0.5}) | ||
| res = extract_operation_resources(cfg, _LOG) | ||
| assert res.cpu_limit == 0.5 | ||
|
|
||
|
|
||
| def test_extract_operation_resources_reads_nested_resources_block() -> None: |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for fractional cpu_limit inside the nested resources block as well.
Since extract_operation_resources also reads resources.cpu_limit, please add a sibling test using OmegaConf.create({"resources": {"cpu_limit": 0.5}}) and asserting the extracted cpu_limit is 0.5, to confirm nested configs follow the same semantics.
Suggested change
| def test_extract_operation_resources_parses_fractional_cpu_limit() -> None: | |
| cfg = OmegaConf.create({"cpu_limit": 0.5}) | |
| res = extract_operation_resources(cfg, _LOG) | |
| assert res.cpu_limit == 0.5 | |
| def test_extract_operation_resources_reads_nested_resources_block() -> None: | |
| def test_extract_operation_resources_parses_fractional_cpu_limit() -> None: | |
| cfg = OmegaConf.create({"cpu_limit": 0.5}) | |
| res = extract_operation_resources(cfg, _LOG) | |
| assert res.cpu_limit == 0.5 | |
| def test_extract_operation_resources_parses_fractional_cpu_limit_in_nested_resources() -> None: | |
| cfg = OmegaConf.create({"resources": {"cpu_limit": 0.5}}) | |
| res = extract_operation_resources(cfg, _LOG) | |
| assert res.cpu_limit == 0.5 | |
| def test_extract_operation_resources_reads_nested_resources_block() -> None: |
- Introduced a new `validate_cpu_limit` function to ensure `cpu_limit` values are positive finite numbers and not of type `bool`. - Added unit tests to validate rejection of non-finite and boolean CPU limit values, improving error handling and robustness. - Updated existing tests to reflect changes in CPU limit validation logic, ensuring comprehensive coverage.
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.
cpu_limitparameter to accept fractional values in configuration files and code, enhancing flexibility for resource allocation.Summary by Sourcery
Support fractional CPU limits in operation resources and configuration.
New Features:
Enhancements:
Documentation:
Tests: