fix: do not cap <IntRangeExpr> expansion at the list-form limit - #327
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
| # Do not apply this to an `<IntRangeExpr>` expansion. §3.4.1.1.1 constrains that | ||
| # form only by "no two ranges may overlap" and states its purpose is to express | ||
| # frame ranges succinctly, so capping the expansion rejects the form's primary | ||
| # use case and pre-empts the host service's own task-count limits. |
There was a problem hiding this comment.
Removing the expansion cap leaves an unbounded materialization on the CHUNK[INT] + range-expression path. _MAX_TASK_PARAM_RANGE_LEN was the only thing bounding it.
_step_param_space_iter.py:291-294 expands a CHUNK[INT] range into a real Python list, and then set(...) of it (lines 333, 968, 1049):
if isinstance(parameter.range, list):
parameter_range: list[int] = [int(v) for v in parameter.range]
else:
parameter_range = list[int](parameter.range) # IntRangeExpr -> full expansionSo a template with
- name: Frames
type: "CHUNK[INT]"
range: "1-100000000"
chunks: { defaultTaskCount: 100 }now parses cleanly (IntRangeExpr.from_str is O(1) — it stores range objects), and any consumer that later builds a StepParameterSpaceIterator allocates ~100M ints in a list plus a set — multiple GB — before it can reject anything. Previously _check_range_expr_len rejected this at parse time. For a library that validates untrusted job templates, that is a memory-exhaustion vector rather than a validation error.
Note the plain INT range-expression path is fine — RangeExpressionIdentifierNode keeps the IntRangeExpr and iterates lazily, and len() is O(1). The problem is specific to the chunk path (and to the range_set/_range_set construction).
If uncapping the expression form is the intended spec reading, the chunk path probably needs to either stop materializing (chunk boundaries over an IntRangeExpr are computable without expanding it, since __getitem__ is O(log n)) or carry its own explicit, documented bound so the failure is a ValidationError and not an OOM.
There was a problem hiding this comment.
The spec confirms CHUNK[INT] can absolutely use <IntRangeExpr>
| RangeExpressionTaskParameterDefinition model once they are resolved. | ||
| against the ``<IntRangeExpr>`` grammar; a list-form range is length-capped | ||
| (§3.4). The expansion of a range expression is deliberately not capped — | ||
| see ``_MAX_TASK_PARAM_RANGE_LEN``. |
There was a problem hiding this comment.
Cross-implementation parity note. The two comments this PR deletes both asserted that the cap matched openjd-rs (EffectiveLimits.max_task_param_range_len, and the resolve-time checks in create_job/ranges.rs). Dropping the check on the Python side without a corresponding change in openjd-rs means a template with range: "1-5000" now validates here but would still be rejected at create_job, so callers using this library as a pre-submission validator get a late, surprising failure instead of an early one.
Worth confirming the openjd-rs side is being changed in step (or that the earlier comments were simply wrong about what it enforces) — otherwise the two implementations disagree on which templates are valid.
Signed-off-by: David Leong <leongdl@amazon.com>
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Problem
0.11.1began enforcing a 1024-element cap on the expansion of an<IntRangeExpr>task parameter range. §3.4 does not specify that cap for thatform.
The 1024 element limit is stated for the list forms only:
<IntRangeList>: "Maximum number of elements: The list must not contain more than 1024 elements."<FloatRangeList>: same<StringRangeList>: same§3.4.1.1.1
<IntRangeExpr>gives the grammar and exactly one constraint —"no two ranges in the expression are allowed to overlap" — and no element count.
It also states the form's purpose outright: "The motivating use-case for this
form is providing a succinct way to describe a frame range." Capping the
expansion at 1024 rejects that use case;
range: "1-5000"is ordinarysubmission traffic for a render farm.
The spec is also explicit elsewhere when it wants to grant implementations
latitude to constrain a count — e.g. for step dependencies: "There is no maximum
defined, though implementations may choose to constrain the number of
dependencies." No such allowance is given for
<IntRangeExpr>, and none isneeded: a host service that wants to bound task counts can do so itself, and
CallerLimits::max_task_countexists for exactly that.Concretely, this is what regressed:
Downstream, this pre-empts a host service's own task-count limits. AWS Deadline
Cloud resolves
max-tasks-per-stepper customer account and can raise it; thatmechanism becomes unreachable above 1024 for a single-parameter range, because
decode now fails before the service's own check runs.
Change
Narrow the cap to the list forms, matching §3.4 as written.
_check_range_expr_lenand its two call sites: theRangeExpressionTaskParameterDefinition._validate_range_lenvalidator (whoseonly job was that check) and the expansion check in
_validate_int_range_elements.<IntRangeExpr>grammar validation in_validate_int_range_elements— a malformed expression is still rejected._MAX_TASK_PARAM_RANGE_LENandvalidate_task_param_range_list_lenunchanged; the list-form cap is correct and predates 0.11.1 (it was already
enforced in 0.11.0 as
Field(max_length=1024)onIntRangeList/FloatRangeList/StringRangeList).expression form is excluded, so the cap does not get re-broadened.
The Rust side carries the same over-broad cap (
max_task_param_range_lenapplied to
IntRange::Expressioninvalidate_v2023_09/structure.rsand to theRangeExprbranches ofjob/create_job/ranges.rs). That lives in theopenjd-modelcrate this repo consumes from crates.io, so it is fixedseparately; this PR is the pure-Python
v0/v2023_09half.Testing
The expression cap had no test coverage, so nothing needed deleting. Added
TestTaskParameterRangeLengthpinning the §3.4 boundary from both directions:1-100000:2(50000 elements) parseat the template layer and the instantiation layer, and the range expands in
full (asserts
len(instantiated.range) == expected_len, so a future silenttruncation fails the test);
Also added a
1-5000case toTestRangeExpressionTaskParameterDefinition.Full suite:
5471 passed, 24 skipped, 3 xfailed.