diff --git a/Cargo.lock b/Cargo.lock index 14bdc543..9deba2a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -679,9 +679,9 @@ checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" [[package]] name = "openjd-expr" -version = "0.5.0" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e20c50bf19d788b8b491e87cecf7e3e0ea312ffcd17cfb2e2240ff1e64c49887" +checksum = "a05a6e060a42d2b5f681ade212f38482c5fadd460fd73dbc12ebf964828bc290" dependencies = [ "regex", "regex-syntax", @@ -696,9 +696,9 @@ dependencies = [ [[package]] name = "openjd-model" -version = "0.5.4" +version = "0.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29cc8faeffd5da33c6e47c7eaff87169d9ed275292c894f950d215d99d5bfb97" +checksum = "62f9668a4b5663ce7897d34c4d3294b214b499c84360dc88ce2d2dedc444fad3" dependencies = [ "indexmap", "openjd-expr", @@ -728,9 +728,9 @@ dependencies = [ [[package]] name = "openjd-sessions" -version = "0.5.4" +version = "0.5.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4fe33608faf28b7ef2449814ad9040605775cbf5a5f7c3d30e53fcd2842b1aff" +checksum = "ef2f96d8fe1e21d255c34cd466d4619d4f809e5105e01fc6f03463a34194fe2c" dependencies = [ "bitflags", "futures-util", diff --git a/THIRD-PARTY-LICENSES.txt b/THIRD-PARTY-LICENSES.txt index 10c7af66..1137dde5 100644 --- a/THIRD-PARTY-LICENSES.txt +++ b/THIRD-PARTY-LICENSES.txt @@ -2534,9 +2534,9 @@ limitations under the License. ** itoa; version 1.0.18 -- https://crates.io/crates/itoa ** libc; version 0.2.189 -- https://crates.io/crates/libc ** manyhow-macros; version 0.11.4 -- https://crates.io/crates/manyhow-macros -** openjd-expr; version 0.5.0 -- https://crates.io/crates/openjd-expr -** openjd-model; version 0.5.4 -- https://crates.io/crates/openjd-model -** openjd-sessions; version 0.5.4 -- https://crates.io/crates/openjd-sessions +** openjd-expr; version 0.6.0 -- https://crates.io/crates/openjd-expr +** openjd-model; version 0.6.0 -- https://crates.io/crates/openjd-model +** openjd-sessions; version 0.5.5 -- https://crates.io/crates/openjd-sessions ** pin-project-lite; version 0.2.17 -- https://crates.io/crates/pin-project-lite ** portable-atomic; version 1.15.0 -- https://crates.io/crates/portable-atomic ** proc-macro2; version 1.0.107 -- https://crates.io/crates/proc-macro2 diff --git a/rust-bindings/Cargo.toml b/rust-bindings/Cargo.toml index e87cad1b..3142af02 100644 --- a/rust-bindings/Cargo.toml +++ b/rust-bindings/Cargo.toml @@ -12,9 +12,9 @@ name = "_openjd_rs" crate-type = ["cdylib", "rlib"] [dependencies] -openjd-expr = "0.5.0" -openjd-model = "0.5.4" -openjd-sessions = "0.5.4" +openjd-expr = "0.6.0" +openjd-model = "0.6.0" +openjd-sessions = "0.5.5" tokio = { version = "1", features = ["rt-multi-thread"] } uuid = { version = "1", features = ["v4"] } serde_json = "1" diff --git a/rust-bindings/src/model/job.rs b/rust-bindings/src/model/job.rs index c956e3f1..0894754f 100644 --- a/rust-bindings/src/model/job.rs +++ b/rust-bindings/src/model/job.rs @@ -899,15 +899,25 @@ fn task_param_def_from_dict( // List of values — coerce each to the variant's element type. let mut items: Vec = Vec::with_capacity(list.len()); for v in list.iter() { - let s: String = match v.extract::() { - Ok(s) => s, - Err(_) => v.str()?.extract()?, + // Whether the element arrived as a Python `str` decides which member of + // the template's own ` | ` union it is, and a + // `` has to keep the spelling it was written with (§7.5). + // `Float64` reads a JSON string as that spelling and a JSON number as a + // bare value, so the distinction has to survive to here -- `v.str()` + // collapses it. + let (s, is_text): (String, bool) = match v.extract::() { + Ok(s) => (s, true), + Err(_) => (v.str()?.extract()?, false), }; items.push(match variant { "int" | "chunkInt" => match s.parse::() { Ok(n) => serde_json::Value::Number(n.into()), Err(_) => serde_json::Value::String(s), }, + // The text carries as-is. `Float64`'s deserializer parses it, and + // rejects an unparseable one there rather than here -- which is also + // what happened when this re-emitted a number. + "float" if is_text => serde_json::Value::String(s), "float" => match s.parse::() { Ok(n) => serde_json::Number::from_f64(n) .map(serde_json::Value::Number) diff --git a/rust-bindings/src/model/step_param_space.rs b/rust-bindings/src/model/step_param_space.rs index a764c807..5f0df8e7 100644 --- a/rust-bindings/src/model/step_param_space.rs +++ b/rust-bindings/src/model/step_param_space.rs @@ -214,6 +214,26 @@ impl PyStepParameterSpaceIterator { } fn __getitem__(&self, py: Python<'_>, index: isize) -> PyResult> { + // Random access uses a fresh iterator — don't disturb the + // 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)?; + // Rejected here rather than left to `get`, because the negative-index + // arithmetic below resolves against `self.len` — the count `__len__` + // refuses to report for an adaptive space. Without this, `it[-1]` declines + // only incidentally, and would start answering against that hidden count + // if `get` were ever extended to adaptive spaces the way 0.6.0 extended it + // to contiguous ones. The message is the pure-Python reference's; the type + // stays `IndexError`, which is a `LookupError` as v0 raises, so `except` + // clauses for either keep working. + if iter.chunks_adaptive() { + return Err(pyo3::exceptions::PyIndexError::new_err( + "Items cannot be retrieved by index because the parameter space uses adaptive chunking.", + )); + } let idx = if index < 0 { let adjusted = self.len as isize + index; if adjusted < 0 { @@ -225,13 +245,6 @@ impl PyStepParameterSpaceIterator { } else { index as usize }; - // Random access uses a fresh iterator — don't disturb the - // 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/rust-bindings/src/model/task_parameter.rs b/rust-bindings/src/model/task_parameter.rs index 28ff4c75..57cc1868 100644 --- a/rust-bindings/src/model/task_parameter.rs +++ b/rust-bindings/src/model/task_parameter.rs @@ -548,8 +548,12 @@ pub(crate) fn task_parameter_to_py<'py>( } .into_bound_py_any(py) } + // `Float64` carries the spelling a `` range element was + // written with (§7.5). It reaches a command line through + // `TaskParameterValue`, which renders it verbatim; this getter is the + // numeric introspection view, so take the value and drop the spelling. TaskParameter::Float { range } => PyFloatTaskParameter { - range: range.clone(), + range: range.iter().map(|f| f.value()).collect(), } .into_bound_py_any(py), TaskParameter::String { range } => PyStringTaskParameter { diff --git a/scripts/check_third_party_licenses.sh b/scripts/check_third_party_licenses.sh index 2e2f8bd9..19fb3f33 100755 --- a/scripts/check_third_party_licenses.sh +++ b/scripts/check_third_party_licenses.sh @@ -150,16 +150,15 @@ awk -v re="^[*][*] ($workspace_pattern); version " ' # ── Combine ─────────────────────────────────────────────────────────── +# Piped through `sed` on the way in, so the EOL strip needs neither a second temp +# file nor `sed -i`, which wants a backup suffix on BSD sed and refuses one on GNU. { echo "" echo "" cat "$python_section" echo "" cat "$rust_section" -} > "$generated" - -# Ensure consistent EOL. -sed -i 's/\r//' "$generated" +} | sed 's/\r//' > "$generated" if [[ "$mode" == "update" ]]; then cp "$generated" "$OUTPUT_FILE" diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 3150975b..96e587f2 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -718,6 +718,24 @@ does. (The underlying Rust struct has `chunks: Option` on the `Int` variant for shape reasons, but no resolver path ever populates it; the binding mirrors the runtime *behaviour*.) +`FloatTaskParameter.range` is numeric, so it does not show the decimal +places a `` range element was written with. A range element +`'2.50'` reports `2.5` here and renders `2.50` as the task parameter +value, which is the form that reaches a command line (Template Schemas +§7.5). Two `FloatTaskParameter`s that compare equal by `range` can +therefore render different task values. Read +`StepParameterSpaceIterator` for the rendered form. + +Constructing a space directly follows the same rule as the template: a +range element given as a `str` is a `` and keeps its +spelling, and one given as a `float` is a `` and renders as the +number. `StepParameterSpace(taskParameterDefinitions={"F": {"type": +"FLOAT", "range": ["1.50"]}})` renders `1.50`, and `range=[1.5]` renders +`1.5`. Stripping a redundant leading zero is a `create_job` +normalization rather than part of reading a resolved value, so `'02.50'` +given directly to the constructor keeps its zero where the same element +in a template does not. + ### `ChunkIntTaskParameter` Available only when the `TASK_CHUNKING` extension is enabled. @@ -1176,20 +1194,30 @@ 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`. +Indexing observes the override. `openjd-model` 0.6.0 gave a `CONTIGUOUS` +chunked space random access, so `it[i]` answers there as it does for a +`NONCONTIGUOUS` one, and reports the overridden granularity rather than +the template's. + +One current-implementation limitation remains. An *adaptive* space has no +knowable count until it is walked, so `len(it)` raises `ValueError` and +`it[i]` is refused for every index, negative included. Iteration still +yields, which is what distinguishes an unknown count from an empty space. +Supplying the override makes the space static and lifts both. + +That refusal is enforced rather than incidental: `__getitem__` rejects an +adaptive space before it resolves a negative index, because the +arithmetic for a negative index would otherwise run against the length +`__len__` declines to report. + +One divergence from the v0 reference here, in the exception *type*. v0 +raises a bare `LookupError` for an adaptive `it[i]`; v1 raises +`IndexError`, which is a `LookupError` subclass, so a caller catching +either type is served by both implementations. The messages agree. +Because `IndexError` also means "past the end" in v1, the two conditions +are told apart by the message rather than the type — the adaptive refusal +says `Items cannot be retrieved by index because the parameter space uses +adaptive chunking.` where a real overrun says `index out of range`. ### `StepDependencyGraph` diff --git a/test/openjd/model_v1/test_known_gaps.py b/test/openjd/model_v1/test_known_gaps.py index bbc50c6b..3cb9c745 100644 --- a/test/openjd/model_v1/test_known_gaps.py +++ b/test/openjd/model_v1/test_known_gaps.py @@ -19,8 +19,6 @@ 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``, @@ -43,121 +41,3 @@ 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_parse.py b/test/openjd/model_v1/test_parse.py index 482a9bca..35267aef 100644 --- a/test/openjd/model_v1/test_parse.py +++ b/test/openjd/model_v1/test_parse.py @@ -232,6 +232,69 @@ def test_template_extensions_list(template, template_type, decode_function) -> N decode_function(template=template, supported_extensions=["TASK_CHUNKING", "EXPR"]) +class TestLetIdentifierLengthCap(object): + """A ``let`` binding's ```` is capped at 512 characters + (Template Schemas §3.6.1). Enforced by openjd-model 0.6.0 (openjd-rs#358); + before that a 513-character name was accepted here. The v0 side has its own + coverage in ``test/openjd/model_v0/v2023_09/test_let_bindings.py``; this is the + v1 path, which had none. + + The cap is flat, not the §7.1 ```` cap of 64 rising to 512 with + ``FEATURE_BUNDLE_1``. The 512-character accept with ``EXPR`` alone is what makes + the difference observable, and is why the fix upstream did not reuse + ``EffectiveLimits::max_identifier_len``. + """ + + @staticmethod + def _template(name: str, extensions: list[str]) -> dict[str, Any]: + return { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": extensions, + "steps": [ + { + "name": "Step1", + "let": [f"{name} = 42"], + "script": {"actions": {"onRun": {"command": "echo", "args": ["hi"]}}}, + } + ], + } + + @pytest.mark.parametrize( + "extensions", + [ + pytest.param(["EXPR"], id="EXPR only"), + pytest.param(["EXPR", "FEATURE_BUNDLE_1"], id="with FEATURE_BUNDLE_1"), + ], + ) + def test_512_characters_is_accepted(self, extensions: list[str]) -> None: + """Both extension sets accept the boundary. Without ``FEATURE_BUNDLE_1`` the + §7.1 cap would be 64, so this is the case that pins the cap as flat.""" + template = self._template("a" * 512, extensions) + assert decode_job_template(template=template, supported_extensions=extensions) + + @pytest.mark.parametrize( + "extensions", + [ + pytest.param(["EXPR"], id="EXPR only"), + pytest.param(["EXPR", "FEATURE_BUNDLE_1"], id="with FEATURE_BUNDLE_1"), + ], + ) + def test_513_characters_is_rejected(self, extensions: list[str]) -> None: + template = self._template("a" * 513, extensions) + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template(template=template, supported_extensions=extensions) + message = str(excinfo.value) + assert "steps[0] -> let[0]" in message + assert "exceeds 512 characters" in message + + def test_a_short_name_is_unaffected(self) -> None: + """Control. 65 characters is over the §7.1 cap of 64 and under §3.6.1's, so it + must be accepted -- a regression to the wrong constant would reject it.""" + template = self._template("a" * 65, ["EXPR"]) + assert decode_job_template(template=template, supported_extensions=["EXPR"]) + + class TestDecodeJobTemplateStr: """``decode_job_template_str`` wrapper: parses YAML or JSON directly, no intermediate dict. The wrapper lives on 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 acb233a0..9101322b 100644 --- a/test/openjd/model_v1/test_step_param_space_iter.py +++ b/test/openjd/model_v1/test_step_param_space_iter.py @@ -14,7 +14,7 @@ ``{type, range, [chunks]}`` matching the YAML/JSON template syntax. """ -from typing import Any, Callable +from typing import Any, Callable, Optional import pytest @@ -563,6 +563,101 @@ def test_len_raises_on_adaptive_chunking(self) -> None: len(it) +class TestFloatRangeSpellingIsPreserved: + """A FLOAT range element given as a ``str`` keeps the decimal places it was + written with, so the value reaching a command line is the one the caller asked + for (Template Schemas §7.5, openjd-rs#354). + + ``StepParameterSpace`` builds the *resolved* space directly, without the + ``decode_job_template`` + ``create_job`` path, so it is its own route to a + ``Float64`` and has to make the same choice: the template's range element is a + `` | `` union, and only the string member carries a + spelling. Before openjd-model 0.6.0 there was no spelling to carry -- the + resolved range was ``Vec`` -- so this could not be observed. + """ + + @staticmethod + def _rendered(elements: list[Any]) -> list[str]: + space = StepParameterSpace( + taskParameterDefinitions={"F": {"type": "FLOAT", "range": elements}} + ) + return [params["F"].value for params in StepParameterSpaceIterator(space=space)] + + @pytest.mark.parametrize( + "element,expected", + [ + ("1.50", "1.50"), # the trailing zero is the requested scale + ("3.500", "3.500"), + ("1e3", "1e3"), # exponent notation is not expanded + ("1E+2", "1E+2"), + ("5.", "5."), # no digit is invented after the point + (".5", ".5"), # nor before it + ("0.50", "0.50"), + ("-0.00", "0.00"), # zero has no sign, but keeps its places + # The last two do not pin the fix -- both render the same without it, and + # both survive the mutant that reverts it. Kept as controls: canonical text + # must not change, and padded text must still be trimmed (it took the + # string path already, because Rust's `f64` parse rejects the spaces). + ("2.5", "2.5"), + (" 2.50 ", "2.50"), + ], + ) + def test_a_string_element_keeps_its_spelling(self, element: str, expected: str) -> None: + assert self._rendered([element]) == [expected] + + @pytest.mark.parametrize("element,expected", [(1.5, "1.5"), (1000.0, "1000.0"), (0.5, "0.5")]) + def test_a_float_element_renders_as_the_number(self, element: float, expected: str) -> None: + """The negative control. A Python ``float`` is the union's ```` member and + has no spelling to keep, so it must not acquire one from ``str()`` -- these are + the values where preserving the text would have been indistinguishable from + rendering the number, and they must stay that way.""" + assert self._rendered([element]) == [expected] + + def test_it_matches_what_the_template_path_renders(self) -> None: + """The two routes to a resolved space agree, which is the point of the fix: + before it, this space rendered ``1.5`` where the template rendered ``1.50``.""" + elements = ["1.50", "1e3", "5.", ".5", "2.5"] + template = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [ + {"name": "F", "type": "FLOAT", "range": elements} + ] + }, + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{Task.Param.F}}"]}} + }, + } + ], + } + step = create_job( + job_template=decode_job_template(template=template), job_parameter_values={} + ).steps[0] + from_template = [params["F"].value for params in StepParameterSpaceIterator(step=step)] + assert self._rendered(elements) == from_template == elements + + def test_a_redundant_leading_zero_is_not_stripped_here(self) -> None: + """The one place the two routes still differ, and deliberately. Stripping a + redundant leading zero is a ``create_job`` normalization (openjd-model + ``create_job/ranges.rs``), not part of reading a resolved value, so the + template path renders ``2.50`` while a value handed straight to the resolved + type keeps the ``0``. Constructing the Rust ``TaskParameter`` directly behaves + the same way, because ``Float64``'s deserializer trims whitespace and unsigns + zero but does not strip. + """ + assert self._rendered(["02.50"]) == ["02.50"] + + def test_an_unparseable_element_is_still_rejected(self) -> None: + """Forwarding the text rather than a number must not turn a bad element into an + accepted one; it fails in ``Float64``'s deserializer instead of here.""" + with pytest.raises(ValueError, match="not-a-float"): + self._rendered(["not-a-float"]) + + class TestChunkIntContains: """``__contains__`` must round-trip values yielded by a chunked iterator. The iterator yields ``TaskParameterValue`` instances @@ -722,8 +817,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). + NONCONTIGUOUS because that is what the assertions below depend on: it renders a + single-task chunk as a bare ``"1"``, where CONTIGUOUS renders ``"1-1"``. Both + support random access — see + ``test_a_contiguous_space_supports_indexing_with_or_without_the_override``. """ it = StepParameterSpaceIterator( step=self._step({"defaultTaskCount": 5, "rangeConstraint": "NONCONTIGUOUS"}), @@ -741,21 +838,95 @@ def test_indexing_observes_the_override(self) -> None: 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 - ) + def test_a_contiguous_space_supports_indexing_with_or_without_the_override(self) -> None: + """openjd-model 0.6.0 (openjd-rs#355) gave a contiguous chunked space random + access, so ``get`` now answers where it used to decline. Promoted from + ``test_known_gaps.py::test_a_contiguous_chunked_space_supports_indexing``, + which pinned the v0 reading as a strict xfail. + + Indexing observes the override, same as the NONCONTIGUOUS case above, and the + end of the space is still an ``IndexError``. + """ + it = StepParameterSpaceIterator(step=self._step(self._STATIC)) + assert [it[0]["Frame"].value, it[1]["Frame"].value, it[-1]["Frame"].value] == [ + "1-5", + "6-10", + "6-10", + ] + with pytest.raises(IndexError) as excinfo: + _ = it[2] + assert str(excinfo.value) == "index out of range" + + overridden = StepParameterSpaceIterator( + step=self._step(self._STATIC), chunks_task_count_override=1 + ) + assert [overridden[0]["Frame"].value, overridden[-1]["Frame"].value] == ["1-1", "10-10"] + + def test_an_adaptive_space_still_refuses_indexing(self) -> None: + """The negative control for the test above, and the remaining limitation + recorded in ``specs/python-model-interface.md``: an adaptive space has no + knowable count, so ``len()`` raises and every index is refused. Iteration + still yields, which is what distinguishes "unknown count" from "empty". + + The refusal is enforced rather than incidental. ``__getitem__`` resolves a + negative index against the length ``__len__`` declines to report, so leaving + the rejection to ``get`` would let ``it[-1]`` answer against that hidden count + if ``get`` were extended to adaptive spaces, as 0.6.0 extended it to + contiguous ones. The message therefore names the reason instead of saying + "index out of range", and it is the pure-Python reference's wording. + """ + it = StepParameterSpaceIterator(step=self._step(self._ADAPTIVE)) + assert it.chunks_adaptive is True + for index in (0, 1, -1, -100): with pytest.raises(IndexError) as excinfo: - _ = it[0] - assert str(excinfo.value) == "index out of range" + _ = it[index] + assert str(excinfo.value) == ( + "Items cannot be retrieved by index because the parameter space " + "uses adaptive chunking." + ) + # v0 raises a bare LookupError here, not an IndexError. IndexError is a + # LookupError subclass, so a caller catching either is served by both + # implementations; the type itself still differs, which the spec records. + with pytest.raises(LookupError): + _ = it[0] + assert self._frames(StepParameterSpaceIterator(step=self._step(self._ADAPTIVE))) == [ + "1-5", + "6-10", + ] + + @pytest.mark.parametrize( + "chunks,override,expected_count", + [ + # Statically chunked: no override involved, the template's own 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, which becomes 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( + self, chunks: dict[str, Any], override: Optional[int], expected_count: int + ) -> None: + """openjd-model 0.6.0 (openjd-rs#355) reports ``chunks_parameter_name`` and + ``chunks_default_task_count`` for any chunked space, not only an adaptive one, + which is what the v0 reference has always done. Promoted from + ``test_known_gaps.py``, where it was a strict xfail.""" + it = StepParameterSpaceIterator( + step=self._step(chunks), 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 def test_an_intermediate_override_regroups_the_chunks(self) -> None: """Not just 1: any positive size regroups the space."""