diff --git a/rust-bindings/src/model/step_param_space.rs b/rust-bindings/src/model/step_param_space.rs index c17cc468..a764c807 100644 --- a/rust-bindings/src/model/step_param_space.rs +++ b/rust-bindings/src/model/step_param_space.rs @@ -123,14 +123,52 @@ pub(crate) struct PyStepParameterSpaceIterator { /// every `NodeIterator` impl is `Send + Sync` (enforced at the /// trait bound in `openjd-model`). iter: Mutex, + /// The `chunks_task_count_override` this was constructed with, kept + /// so `__getitem__` can rebuild an equivalent iterator. Without it, + /// random access would silently fall back to the template's own + /// `defaultTaskCount` and disagree with iteration. + chunk_override: Option, } #[cfg_attr(feature = "stub-gen", gen_stub_pymethods)] #[pymethods] impl PyStepParameterSpaceIterator { + /// Construct an iterator over a step's parameter space. + /// + /// `chunks_task_count_override` overrides the `defaultTaskCount` of a + /// `CHUNK[INT]` parameter and turns adaptive chunking off, so a chunked space + /// can be walked at a caller-chosen granularity. Pass `1` to iterate individual + /// tasks. Ignored when the space has no chunked parameter, matching the + /// pure-Python reference — though a non-positive value is still rejected in + /// that case, since validating an argument is cheaper to reason about than + /// silently discarding a bad one. + /// + /// Without this, a statically chunked space could only be walked at the + /// template's own chunk size: `chunks_default_task_count` is settable for + /// adaptive spaces only. #[new] - #[pyo3(signature = (*, step=None, space=None))] - fn new(step: Option<&PyStep>, space: Option<&PyStepParameterSpace>) -> PyResult { + #[pyo3(signature = (*, step=None, space=None, chunks_task_count_override=None))] + fn new( + step: Option<&PyStep>, + space: Option<&PyStepParameterSpace>, + chunks_task_count_override: Option, + ) -> PyResult { + // Taken as i64, not usize, so a negative value reaches this check instead of + // failing pyo3's unsigned extraction with OverflowError. Every non-positive + // value should report the same ValueError the message and the spec promise. + // + // Rejected rather than clamped: openjd-model applies `.max(1)` to the override, + // so 0 would silently mean 1. The `chunks_default_task_count` setter already + // refuses 0, and the two should not disagree. + let chunks_task_count_override: Option = match chunks_task_count_override { + Some(n) if n <= 0 => { + return Err(pyo3::exceptions::PyValueError::new_err( + "chunks_task_count_override must be a positive integer.", + )); + } + Some(n) => Some(n as usize), + None => None, + }; let ps = if let Some(s) = space { s.inner.clone() } else if let Some(st) = step { @@ -148,7 +186,9 @@ impl PyStepParameterSpaceIterator { combination: None, } }; - let iter = StepParameterSpaceIterator::new(&ps).map_err(model_err_to_py)?; + let iter = + StepParameterSpaceIterator::new_with_chunk_override(&ps, chunks_task_count_override) + .map_err(model_err_to_py)?; let len = iter.len(); let names = iter.names().clone(); Ok(Self { @@ -156,6 +196,7 @@ impl PyStepParameterSpaceIterator { len, names, iter: Mutex::new(iter), + chunk_override: chunks_task_count_override, }) } @@ -185,8 +226,12 @@ impl PyStepParameterSpaceIterator { index as usize }; // Random access uses a fresh iterator — don't disturb the - // persistent iter's cursor or its adaptive Arc. - let iter = StepParameterSpaceIterator::new(&self.space).map_err(model_err_to_py)?; + // persistent iter's cursor or its adaptive Arc. It must carry the + // same chunk override, or indexing would report chunks that + // iteration never yields. + let iter = + StepParameterSpaceIterator::new_with_chunk_override(&self.space, self.chunk_override) + .map_err(model_err_to_py)?; match iter.get(idx) { Some(params) => task_param_set_to_py(py, ¶ms), None => Err(pyo3::exceptions::PyIndexError::new_err( diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index aa8097c7..3150975b 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -1127,6 +1127,8 @@ from openjd.model._v1.job import StepParameterSpaceIterator it = StepParameterSpaceIterator(step=job.steps[0]) # or: it = StepParameterSpaceIterator(space=step.parameterSpace) +# or, to walk a chunked space one task at a time: +# it = StepParameterSpaceIterator(space=..., chunks_task_count_override=1) len(it) # total task count, e.g. 10 it[0] # {"Frame": 1} @@ -1150,6 +1152,45 @@ non-trivial state for chunked spaces). Indexing (``it[i]``) is unaffected by iteration position; ``__contains__`` is also non-mutating. +`chunks_task_count_override` replaces the `defaultTaskCount` of a +`CHUNK[INT]` parameter and turns adaptive chunking off, so a chunked +space can be walked at a granularity the caller picks. Pass `1` to +iterate individual tasks — a `1-20` range chunked five at a time then +yields `1-1`, `2-2`, … instead of `1-5`, `6-10`, …. It is ignored when +the space has no chunked parameter, matching the pure-Python reference, +although a non-positive value is still rejected in that case. +Iteration observes it, and `len()` counts the overridden granularity. + +The rendered form of a chunk depends on `rangeConstraint`, which matters +because these are strings a consumer parses and may feed back through +`__contains__`. A single-task chunk is `"1-1"` under `CONTIGUOUS` and a +bare `"1"` under `NONCONTIGUOUS`. + +It is currently the only way to change the chunk size of a *static* +chunked space: the `chunks_default_task_count` setter accepts adaptive +spaces only, and raises `ValueError` otherwise. + +Any non-positive value raises `ValueError`, so one `except ValueError` +covers `0` and negatives alike. The Rust layer clamps the override to at +least 1, so 0 would otherwise silently mean 1, and the +`chunks_default_task_count` setter already rejects it. The pure-Python +reference does not validate this argument. + +Two current-implementation limitations, both divergences from the v0 +reference rather than intended behaviour. Each has a `strict` xfail in +`test/openjd/model_v1/test_known_gaps.py`, so clearing either will fail +CI until this text is updated with it. + +- Indexing observes the override only for a space that supports random + access. A `CONTIGUOUS` chunked space requires sequential iteration, so + `it[i]` raises `IndexError` for any index — with or without the + override — even though `len(it)` reports a count. See + `test_a_contiguous_chunked_space_supports_indexing`. +- `chunks_parameter_name` and `chunks_default_task_count` both return + `None` once the space is non-adaptive, which supplying the override + makes it. v0 reports the parameter name and the override value. See + `test_chunk_metadata_is_reported_for_a_non_adaptive_space`. + ### `StepDependencyGraph` Step dependency graph for topological ordering. diff --git a/src/openjd/_openjd_rs.pyi b/src/openjd/_openjd_rs.pyi index 205166a2..c05192bb 100644 --- a/src/openjd/_openjd_rs.pyi +++ b/src/openjd/_openjd_rs.pyi @@ -2381,7 +2381,23 @@ class StepParameterSpaceIterator: *, step: typing.Optional[Step] = None, space: typing.Optional[StepParameterSpace] = None, - ) -> StepParameterSpaceIterator: ... + chunks_task_count_override: typing.Optional[builtins.int] = None, + ) -> StepParameterSpaceIterator: + r""" + Construct an iterator over a step's parameter space. + + `chunks_task_count_override` overrides the `defaultTaskCount` of a + `CHUNK[INT]` parameter and turns adaptive chunking off, so a chunked space + can be walked at a caller-chosen granularity. Pass `1` to iterate individual + tasks. Ignored when the space has no chunked parameter, matching the + pure-Python reference -- though a non-positive value is still rejected in + that case. + + Without this, a statically chunked space could only be walked at the + template's own chunk size: `chunks_default_task_count` is settable for + adaptive spaces only. + """ + def __len__(self) -> builtins.int: ... def __getitem__(self, index: builtins.int) -> dict: ... def __iter__(self) -> StepParameterSpaceIterator: diff --git a/test/openjd/model_v1/test_known_gaps.py b/test/openjd/model_v1/test_known_gaps.py index 3cb9c745..bbc50c6b 100644 --- a/test/openjd/model_v1/test_known_gaps.py +++ b/test/openjd/model_v1/test_known_gaps.py @@ -19,6 +19,8 @@ from __future__ import annotations +from typing import Any + # ── Top-level package no longer leaks typing imports ── # # An earlier draft of ``openjd.model._v1`` imported ``Any``, @@ -41,3 +43,121 @@ def test_no_internal_imports_leak_at_top_level(name: str) -> None: import openjd.model._v1 as v1 assert not hasattr(v1, name), f"{name} leaks as a public attribute on openjd.model._v1" + + +# ── Chunked parameter spaces: two divergences from the v0 reference ── +# +# Found while adding `chunks_task_count_override` to +# `StepParameterSpaceIterator`. Neither is caused by that argument — both +# reproduce without it — so they are recorded here rather than fixed in +# passing. + + +def _chunked_step(constraint: str) -> Any: + from openjd.model._v1 import create_job, decode_job_template + + template = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": ["TASK_CHUNKING"], + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [ + { + "name": "Frame", + "type": "CHUNK[INT]", + "range": "1-10", + "chunks": {"defaultTaskCount": 5, "rangeConstraint": constraint}, + } + ] + }, + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.Frame}}"]}} + }, + } + ], + } + job_template = decode_job_template(template=template, supported_extensions=["TASK_CHUNKING"]) + return create_job(job_template=job_template, job_parameter_values={}).steps[0] + + +@pytest.mark.xfail( + reason="v1 derives chunks_parameter_name and chunks_default_task_count from adaptive " + "detection, so both are None for any non-adaptive chunked space. v0 reports them for " + "any chunked space. See openjd-model step_param_space.rs: chunks_param_name and " + "adaptive_chunk_size are both built from adaptive_info.", + strict=True, +) +@pytest.mark.parametrize( + "chunks,override,expected_count", + [ + # Statically chunked: no override involved, v0 reports the template's size. + ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, None, 5), + # Statically chunked, re-chunked by the override. + ({"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"}, 1, 1), + # Adaptive, turned static by the override. v0 reports the override as the size. + ( + {"defaultTaskCount": 5, "targetRuntimeSeconds": 60, "rangeConstraint": "CONTIGUOUS"}, + 1, + 1, + ), + ], + ids=["static", "static-overridden", "adaptive-overridden"], +) +def test_chunk_metadata_is_reported_for_a_non_adaptive_space( + chunks: dict, override: int | None, expected_count: int +) -> None: + """v0 returns ``"Frame"`` and the chunk size for each of these. v1 returns ``None``. + + Neither value is unknowable — both are in the template, or are the override the caller + just passed — so a consumer inspecting a non-adaptive chunked space through v1 cannot + learn which parameter chunks, or at what size. One root cause, three ways to reach it: + anything that leaves the space non-adaptive drops both getters. + """ + from openjd.model._v1 import create_job, decode_job_template + from openjd.model._v1.job import StepParameterSpaceIterator + + template = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": ["TASK_CHUNKING"], + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [ + {"name": "Frame", "type": "CHUNK[INT]", "range": "1-10", "chunks": chunks} + ] + }, + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.Frame}}"]}} + }, + } + ], + } + job_template = decode_job_template(template=template, supported_extensions=["TASK_CHUNKING"]) + step = create_job(job_template=job_template, job_parameter_values={}).steps[0] + + it = StepParameterSpaceIterator(step=step, chunks_task_count_override=override) + assert it.chunks_adaptive is False + assert it.chunks_parameter_name == "Frame" + assert it.chunks_default_task_count == expected_count + + +@pytest.mark.xfail( + reason="v1 refuses random access whenever the space needs sequential iteration, and " + "contiguous chunking always does. v0 supports indexing the same space.", + strict=True, +) +def test_a_contiguous_chunked_space_supports_indexing() -> None: + """v0 answers ``it[0]`` with ``1-5``. v1 raises ``IndexError``. + + ``len()`` works on this space, so the count is known; only ``get`` declines. + """ + from openjd.model._v1.job import StepParameterSpaceIterator + + it = StepParameterSpaceIterator(step=_chunked_step("CONTIGUOUS")) + assert len(it) == 2 + assert it[0]["Frame"].value == "1-5" diff --git a/test/openjd/model_v1/test_step_param_space_iter.py b/test/openjd/model_v1/test_step_param_space_iter.py index b87f292c..acb233a0 100644 --- a/test/openjd/model_v1/test_step_param_space_iter.py +++ b/test/openjd/model_v1/test_step_param_space_iter.py @@ -648,3 +648,192 @@ def test_explicit_chunk_value_round_trip(self) -> None: # caller wants to check whether a known chunk is in the space. existing = {"Frame": TaskParameterValue(type=TaskParameterType.CHUNK_INT, value="1-5")} assert existing in it + + +class TestChunksTaskCountOverride: + """``chunks_task_count_override`` re-chunks a space at the caller's granularity. + + Before this existed, a *statically* chunked space could only be walked at the + template's own ``defaultTaskCount``: the ``chunks_default_task_count`` setter + accepts adaptive spaces only. Consumers that store one task per chunk value — + the reason the pure-Python reference has this argument — had no way to expand a + static space through the binding. + """ + + @staticmethod + def _step(chunks: dict[str, Any], *, range: str = "1-10") -> Any: + """A single CHUNK[INT] step, built through decode + create_job so the test + exercises the same path a consumer hits at runtime.""" + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": ["TASK_CHUNKING"], + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [ + { + "name": "Frame", + "type": "CHUNK[INT]", + "range": range, + "chunks": chunks, + } + ] + }, + "script": { + "actions": { + "onRun": {"command": "echo", "args": ["{{Task.Param.Frame}}"]} + } + }, + } + ], + }, + supported_extensions=["TASK_CHUNKING"], + ) + return create_job(job_template=t, job_parameter_values={}).steps[0] + + _STATIC = {"defaultTaskCount": 5, "rangeConstraint": "CONTIGUOUS"} + _ADAPTIVE = {"defaultTaskCount": 5, "targetRuntimeSeconds": 60, "rangeConstraint": "CONTIGUOUS"} + + @staticmethod + def _frames(it: Any) -> list: + return [params["Frame"].value for params in it] + + def test_static_space_without_override_yields_template_chunks(self) -> None: + """The baseline the override changes: 1-10 at 5 per chunk is two chunks.""" + it = StepParameterSpaceIterator(step=self._step(self._STATIC)) + assert self._frames(it) == ["1-5", "6-10"] + + def test_static_space_with_override_1_yields_individual_tasks(self) -> None: + """The case the argument exists for, and the one that was unreachable.""" + it = StepParameterSpaceIterator(step=self._step(self._STATIC), chunks_task_count_override=1) + assert self._frames(it) == [f"{n}-{n}" for n in range(1, 11)] + + def test_len_counts_the_overridden_granularity(self) -> None: + """``len()`` is the observable proof the override reached the space: the same + template counts 2 without it and 10 with it.""" + step = self._step(self._STATIC) + assert len(StepParameterSpaceIterator(step=step)) == 2 + assert len(StepParameterSpaceIterator(step=step, chunks_task_count_override=1)) == 10 + + def test_indexing_observes_the_override(self) -> None: + """``__getitem__`` builds a fresh iterator, so it has to carry the override too, + or indexing reports chunks iteration never yields. + + Uses NONCONTIGUOUS because random access needs a non-sequential space, and a + CONTIGUOUS chunked space is always sequential (see the test below). + """ + it = StepParameterSpaceIterator( + step=self._step({"defaultTaskCount": 5, "rangeConstraint": "NONCONTIGUOUS"}), + chunks_task_count_override=1, + ) + yielded = self._frames(it) + assert yielded == [str(n) for n in range(1, 11)] + # Without the override carried through, index 0 would be the template's first + # chunk, "1-5", and disagree with what iteration produced. + fresh = StepParameterSpaceIterator( + step=self._step({"defaultTaskCount": 5, "rangeConstraint": "NONCONTIGUOUS"}), + chunks_task_count_override=1, + ) + assert fresh[0]["Frame"].value == yielded[0] + assert fresh[9]["Frame"].value == yielded[-1] + assert fresh[-1]["Frame"].value == yielded[-1] + + def test_a_contiguous_space_refuses_indexing_with_or_without_the_override(self) -> None: + """Pre-existing behaviour the override does not change: openjd-model requires + sequential iteration for contiguous chunking, so ``get`` always declines. + Pinned here so a future change to random access is a deliberate one. Its + counterpart is ``test_known_gaps.py::test_a_contiguous_chunked_space_supports_indexing``, + a strict xfail asserting the opposite: closing that gap fails there as an xpass + *and* here as a hard assertion, so both move together, along with the + limitation noted in ``specs/python-model-interface.md``.""" + for override in (None, 1): + it = StepParameterSpaceIterator( + step=self._step(self._STATIC), chunks_task_count_override=override + ) + with pytest.raises(IndexError) as excinfo: + _ = it[0] + assert str(excinfo.value) == "index out of range" + + def test_an_intermediate_override_regroups_the_chunks(self) -> None: + """Not just 1: any positive size regroups the space.""" + it = StepParameterSpaceIterator(step=self._step(self._STATIC), chunks_task_count_override=2) + assert self._frames(it) == ["1-2", "3-4", "5-6", "7-8", "9-10"] + + def test_override_turns_adaptive_chunking_off(self) -> None: + """Matches the pure-Python reference: supplying the override makes the + parameter static, which is also what makes ``len()`` answerable — an + adaptive space raises on ``len()`` because the count is not yet knowable.""" + adaptive = StepParameterSpaceIterator(step=self._step(self._ADAPTIVE)) + assert adaptive.chunks_adaptive is True + with pytest.raises(ValueError) as excinfo: + len(adaptive) + assert ( + str(excinfo.value) + == "Length is not available because the parameter space uses adaptive chunking." + ) + + overridden = StepParameterSpaceIterator( + step=self._step(self._ADAPTIVE), chunks_task_count_override=1 + ) + assert overridden.chunks_adaptive is False + assert len(overridden) == 10 + assert self._frames(overridden) == [f"{n}-{n}" for n in range(1, 11)] + + def test_a_non_positive_override_is_rejected_even_when_it_would_be_ignored(self) -> None: + """The validation runs before the space is inspected, so an unchunked space still + rejects 0 rather than silently discarding it. The reference does not validate at + all; this is the documented divergence, pinned so it stays deliberate.""" + space = StepParameterSpace( + taskParameterDefinitions={"Frame": {"type": "INT", "range": [1, 2, 3]}} + ) + with pytest.raises(ValueError) as excinfo: + StepParameterSpaceIterator(space=space, chunks_task_count_override=0) + assert str(excinfo.value) == "chunks_task_count_override must be a positive integer." + + def test_override_is_ignored_when_the_space_has_no_chunked_parameter(self) -> None: + """Reference behaviour: the override only applies to a space that chunks.""" + space = StepParameterSpace( + taskParameterDefinitions={"Frame": {"type": "INT", "range": [1, 2, 3]}} + ) + it = StepParameterSpaceIterator(space=space, chunks_task_count_override=1) + assert [params["Frame"].value for params in it] == ["1", "2", "3"] + assert it.chunks_default_task_count is None + assert it.chunks_parameter_name is None + + @pytest.mark.parametrize("override", [0, -1, -5]) + def test_a_non_positive_override_is_rejected_as_a_value_error(self, override: int) -> None: + """openjd-model clamps the override to at least 1, so 0 would silently mean 1, and + the ``chunks_default_task_count`` setter already refuses 0. + + Negatives report the same ``ValueError`` rather than the ``OverflowError`` an + unsigned extraction would raise, so one ``except ValueError`` covers every + non-positive input. + """ + with pytest.raises(ValueError) as excinfo: + StepParameterSpaceIterator( + step=self._step(self._STATIC), chunks_task_count_override=override + ) + assert str(excinfo.value) == "chunks_task_count_override must be a positive integer." + + def test_the_setter_still_refuses_a_static_space(self) -> None: + """The override does not replace the setter, and must not loosen it: the + setter mutates a live adaptive iterator, which a static space cannot do.""" + it = StepParameterSpaceIterator(step=self._step(self._STATIC)) + with pytest.raises(ValueError) as excinfo: + it.chunks_default_task_count = 1 + assert str(excinfo.value) == ( + "The parameter space does not use adaptive chunking, " + "so cannot modify chunks_default_task_count." + ) + + def test_yielded_values_round_trip_through_contains(self) -> None: + """An overridden chunk must still satisfy containment, so a consumer can + validate a task it was handed.""" + step = self._step(self._STATIC) + it = StepParameterSpaceIterator(step=step, chunks_task_count_override=1) + fresh = StepParameterSpaceIterator(step=step, chunks_task_count_override=1) + for params in list(it): + assert params in fresh