Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/operations/map.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ resources:
pool: default # YT pool name
pool_tree: null # Optional: pool tree
memory_limit_gb: 4 # Memory per job (GB)
cpu_limit: 2 # CPU cores per job
cpu_limit: 2 # CPU cores per job (fractional, e.g. 0.5)
job_count: 2 # Number of parallel jobs
gpu_limit: 0 # GPU count (default: 0)
user_slots: null # Optional: user slots limit
Expand All @@ -247,7 +247,7 @@ resources:
**Resources (rule of thumb):**

- Raise `memory_limit_gb` when a single row plus model weights no longer fits.
- `cpu_limit` helps per-task throughput; `job_count` spreads rows across tasks.
- `cpu_limit` helps per-task throughput; fractional values (e.g. `0.5`, `0.1`) are passed through to YTsaurus; `job_count` spreads rows across tasks.
- `gpu_limit` > 0 only works with an image that actually exposes GPUs to Python.

### Advanced Configuration
Expand Down
4 changes: 2 additions & 2 deletions docs/operations/vanilla.md
Original file line number Diff line number Diff line change
Expand Up @@ -221,15 +221,15 @@ resources:
pool: default # YT pool name
pool_tree: null # Optional: pool tree
memory_limit_gb: 2 # Memory (GB)
cpu_limit: 1 # CPU cores
cpu_limit: 1 # CPU cores (fractional, e.g. 0.5)
gpu_limit: 0 # GPU count (default: 0)
user_slots: null # Optional: user slots limit
```

**Resource guidelines:**

- **Memory**: Allocate based on task needs
- **CPU**: More CPUs = faster execution
- **CPU**: More cores = faster execution; fractional limits (e.g. `0.5`) are supported
- **GPU**: Set to 1+ for GPU workloads (requires custom Docker)

### Advanced Configuration
Expand Down
22 changes: 21 additions & 1 deletion tests/test_client_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,30 @@ def test_operation_resources_rejects_non_positive_memory_gb() -> None:


def test_operation_resources_rejects_non_positive_cpu_limit() -> None:
with pytest.raises(ValueError, match="cpu_limit must be set to a positive integer"):
with pytest.raises(ValueError, match="cpu_limit must be a positive number"):
OperationResources(cpu_limit=0)


def test_operation_resources_accepts_fractional_cpu_limit() -> None:
resources = OperationResources(cpu_limit=0.5)
assert resources.cpu_limit == 0.5


def test_operation_resources_rejects_zero_fractional_cpu_limit() -> None:
with pytest.raises(ValueError, match="cpu_limit must be a positive number"):
OperationResources(cpu_limit=0.0)


def test_operation_resources_rejects_negative_fractional_cpu_limit() -> None:
with pytest.raises(ValueError, match="cpu_limit must be a positive number"):
OperationResources(cpu_limit=-0.5)


def test_operation_resources_rejects_bool_cpu_limit() -> None:
with pytest.raises(TypeError, match="cpu_limit must be a positive number"):
OperationResources(cpu_limit=True) # type: ignore[arg-type]


def test_operation_resources_rejects_negative_gpu_limit() -> None:
with pytest.raises(
ValueError, match="gpu_limit must be set to a non-negative integer"
Expand Down
6 changes: 6 additions & 0 deletions tests/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ def test_extract_operation_resources_reads_top_level_keys() -> None:
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


def test_extract_operation_resources_reads_nested_resources_block() -> None:
Comment on lines +48 to 67

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

cfg = OmegaConf.create({"resources": {"pool": "nest", "job_count": 3}})
res = extract_operation_resources(cfg, _LOG)
Expand Down
6 changes: 5 additions & 1 deletion yt_framework/operations/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ def _int_from_config_value(value: object) -> int:
return int(cast("Any", value))


def _float_from_config_value(value: object) -> float:
return float(cast("Any", value))
Comment on lines +42 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).



def _get_config_value_with_default(
config: DictConfig,
key: str,
Expand Down Expand Up @@ -161,7 +165,7 @@ def extract_operation_resources(
logger,
),
)
cpu_limit = _int_from_config_value(
cpu_limit = _float_from_config_value(
_get_config_value_with_default(resources_config, "cpu_limit", 2, logger),
)
gpu_limit = _int_from_config_value(
Expand Down
20 changes: 15 additions & 5 deletions yt_framework/yt/support/operation_resources.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Operation resource limits for YT jobs."""

import math
from dataclasses import dataclass


Expand All @@ -9,6 +10,15 @@ def _require_positive_resource(name: str, value: int) -> None:
raise ValueError(msg)


def _require_positive_cpu_limit(name: str, value: object) -> None:
if isinstance(value, bool) or not isinstance(value, (int, float)):
msg = f"{name} must be a positive number, got {value!r}"
raise TypeError(msg)
if value <= 0 or not math.isfinite(float(value)):
msg = f"{name} must be a positive number, got {value}"
raise ValueError(msg)


def _require_non_negative_gpu(gpu_limit: int) -> None:
if gpu_limit < 0:
msg = f"gpu_limit must be set to a non-negative integer, got {gpu_limit}"
Expand All @@ -28,29 +38,29 @@ class OperationResources:
pool_tree: Optional pool tree name (default: None).
docker_image: Optional Docker image name for containerized execution (default: None).
memory_gb: Memory allocation in GB (default: 4). In config files, use `memory_limit_gb`.
cpu_limit: CPU cores allocated (default: 2).
cpu_limit: CPU cores allocated per job; fractional values allowed (default: 2).
gpu_limit: Number of GPUs allocated (default: 0).
job_count: Number of parallel jobs (default: 1).
user_slots: Optional user slots limit (default: None).

Raises:
ValueError: If memory_gb, cpu_limit, or job_count are not positive integers,
or if gpu_limit is negative.
ValueError: If memory_gb or job_count are not positive integers, if cpu_limit
is not a positive number, or if gpu_limit is negative.

"""

pool: str = "default"
pool_tree: str | None = None
docker_image: str | None = None
memory_gb: int = 4
cpu_limit: int = 2
cpu_limit: float = 2
gpu_limit: int = 0
job_count: int = 1
user_slots: int | None = None

def __post_init__(self) -> None:
"""Validate resource fields after initialization."""
_require_positive_resource("memory_gb", self.memory_gb)
_require_positive_resource("cpu_limit", self.cpu_limit)
_require_positive_cpu_limit("cpu_limit", self.cpu_limit)
_require_non_negative_gpu(self.gpu_limit)
_require_positive_resource("job_count", self.job_count)
Loading