-
Notifications
You must be signed in to change notification settings - Fork 22
fix: do not cap <IntRangeExpr> expansion at the list-form limit #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -980,9 +980,14 @@ def validate_let_field(value: Any, info: ValidationInfo, *, simple_action: bool | |
| return value | ||
|
|
||
|
|
||
| # §3.4: the maximum number of values a task parameter's range may take on. | ||
| # Not raised by FEATURE_BUNDLE_1 in 2023-09 (matches openjd-rs's | ||
| # EffectiveLimits.max_task_param_range_len). | ||
| # §3.4: the maximum number of elements in a task parameter's *list*-form range | ||
| # — `<IntRangeList>` (§3.4.1.1), `<FloatRangeList>` (§3.4.1.2) and | ||
| # `<StringRangeList>` (§3.4.1.3). Not raised by FEATURE_BUNDLE_1 in 2023-09. | ||
| # | ||
| # 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. | ||
| _MAX_TASK_PARAM_RANGE_LEN = 1024 | ||
|
|
||
|
|
||
|
|
@@ -1289,26 +1294,6 @@ class RangeExpressionTaskParameterDefinition(OpenJDModel_v2023_09): | |
| # has a value when type is CHUNK[INT], which is only possible from the TASK_CHUNKING extension | ||
| chunks: Optional[TaskChunksDefinition] = None | ||
|
|
||
| @field_validator("range") | ||
| @classmethod | ||
| def _validate_range_len(cls, value: Any) -> Any: | ||
| # §3.4: a range expression that arrives via format-string resolution | ||
| # (e.g. `range: "{{RawParam.Frames}}"` with a RANGE_EXPR parameter) is | ||
| # only parsed at instantiation, so the expansion cap must be enforced | ||
| # here too — matching openjd-rs's resolve-time checks in create_job. | ||
| if isinstance(value, IntRangeExpr): | ||
| _check_range_expr_len(value) | ||
| return value | ||
|
|
||
|
|
||
| def _check_range_expr_len(parsed_range: IntRangeExpr) -> None: | ||
| """§3.4: a range expression may expand to at most 1024 values.""" | ||
| if len(parsed_range) > _MAX_TASK_PARAM_RANGE_LEN: | ||
| raise ValueError( | ||
| f"range expression expands to {len(parsed_range)} elements " | ||
| f"(max {_MAX_TASK_PARAM_RANGE_LEN})." | ||
| ) | ||
|
|
||
|
|
||
| def _range_task_param_target(model: Any, typed_values: dict) -> Type[OpenJDModel]: | ||
| """``create_as`` target-model selector shared by the INT and CHUNK[INT] | ||
|
|
@@ -1412,21 +1397,19 @@ def _native_element_type_name(elem: Any) -> str: | |
| def _validate_int_range_elements(value: Any) -> Any: | ||
| """Shared ``range`` post-validator for the INT and CHUNK[INT] | ||
| task-parameter definitions: a literal range-expression string must parse | ||
| and may expand to at most 1024 values (§3.4); a list-form range is | ||
| length-capped. Ranges containing format expressions defer to the | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-implementation parity note. The two comments this PR deletes both asserted that the cap matched openjd-rs ( 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rs changes in progress |
||
| """ | ||
| if isinstance(value, FormatString): | ||
| # If there are no format expressions, we can validate the range expression. | ||
| # otherwise we defer to the RangeExressionTaskParameter model when | ||
| # they've all been evaluated | ||
| if len(value.expressions) == 0: | ||
| try: | ||
| parsed_range = IntRangeExpr.from_str(value) | ||
| IntRangeExpr.from_str(value) | ||
| except Exception as e: | ||
| raise ValueError(str(e)) | ||
| # §3.4: the range may take on at most 1024 values. | ||
| _check_range_expr_len(parsed_range) | ||
| else: | ||
| validate_task_param_range_list_len(value) | ||
| return value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the expansion cap leaves an unbounded materialization on the
CHUNK[INT]+ range-expression path._MAX_TASK_PARAM_RANGE_LENwas the only thing bounding it._step_param_space_iter.py:291-294expands aCHUNK[INT]range into a real Python list, and thenset(...)of it (lines 333, 968, 1049):So a template with
now parses cleanly (
IntRangeExpr.from_stris O(1) — it storesrangeobjects), and any consumer that later builds aStepParameterSpaceIteratorallocates ~100M ints in a list plus a set — multiple GB — before it can reject anything. Previously_check_range_expr_lenrejected 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
INTrange-expression path is fine —RangeExpressionIdentifierNodekeeps theIntRangeExprand iterates lazily, andlen()is O(1). The problem is specific to the chunk path (and to therange_set/_range_setconstruction).If uncapping the expression form is the intended spec reading, the chunk path probably needs to either stop materializing (chunk boundaries over an
IntRangeExprare computable without expanding it, since__getitem__is O(log n)) or carry its own explicit, documented bound so the failure is aValidationErrorand not an OOM.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec confirms CHUNK[INT] can absolutely use
<IntRangeExpr>