| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Template Schemas 2023-09 §3.4.1.1 (L1110) makes an <IntRangeList> element
`<integer> | <intstring>`, and §3.4.1.2 (L1184) makes a <FloatRangeList>
element `<float> | <floatstring>`, neither behind an extension gate. §2.3 and
§2.4 define <intstring>/<floatstring> as "a string whose value is the string
representation of" a number, so such an element denotes the number rather than
its source text.
Python was keeping the source text. A `range: ['1', '02', '003']` on an INT
parameter rendered FRAME:02 and FRAME:003, and a `range: ['1.5', '02.50']` on
a FLOAT parameter rendered W:02.50. These values reach a task command line, so
a renderer was invoked with `--frame 02`.
The pre-validator on the template models did compute the value -- the element
validator evaluates int('02') == 2 -- but validate_list_field returns the
original list object, discarding every coercion, and nothing downstream
recovered it.
Normalize on RangeListTaskParameterDefinition instead, the instantiation
target that all three inbound range paths funnel through: a literal list, a
range-expression expansion, and an RFC 0006 typed whole-field resolution.
Keying off the already-validated `type` field, an INT or CHUNK[INT] element
becomes an int and a FLOAT element becomes a normalized Decimal. Decimal's
normalize() rewrites Decimal('100') as Decimal('1E+2'), so a positive exponent
is quantized away -- exponent notation must never reach a task command line.
Only string-form elements are touched. A <float> literal keeps the scale it was
written with, so a FLOAT `range: [1.0]` still renders 1.0; openjd-rs renders an
integral float the same way and the conformance suite pins it
(2023-09/base/jobs/3.4--float-parameter). STRING and PATH ranges are text by
definition and are left alone. Job parameter defaults are unaffected: they live
on the job-parameter definitions, not the task-parameter definitions, so the
verbatim FLOAT default behaviour that
2023-09/EXPR/jobs/expr1.3.4--float-passthrough pins is untouched.
Normalizing the stored elements exposed a latent bug in
StepParameterSpaceIterator containment. RangeListIdentifierNode compared
ParameterValue.value -- the rendered form of an element -- against a set built
from the raw elements, so it only ever matched when a range happened to be
written as strings. An INT range written as `[1, 2, 3]` already reported every
one of its own values as not contained, before this change. The containment set
now holds rendered elements.
openjd-rs already behaves this way and passes both conformance fixtures; it
parses every range element into an i64/f64 and so has no source text to carry.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| # them as text rather than changing how they render. | ||
| return elem | ||
| exponent = value.as_tuple().exponent | ||
| if isinstance(exponent, int) and exponent > 0: |
There was a problem hiding this comment.
The guard only undoes exponent notation for positive exponents, but Decimal.__str__ also uses scientific notation for sufficiently negative exponents (adjusted exponent below -6). So small <floatstring> elements now render in exponent notation on a task command line -- the exact thing the comment two lines below says must never happen:
This is a behaviour regression for these values, not merely a missed normalization: before this validator the raw string was carried through unchanged.
The parametrized cases only cover exponents that shift upward ("100", "1E+2"), which is why it is not caught. Consider handling both directions: when the normalized exponent is negative and value.adjusted() < -6, return Decimal(format(value, "f")) to force plain notation -- and add a "0.0000001" case to test_floatstring_elements_carry_their_value.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed and fixed. Measured before/after on "0.0000001": the landed code rendered 1E-7 (str(Decimal) switches to exponent notation once the adjusted exponent drops below -6, which the positive-exponent guard never covered), and it is a regression, since keeping the source text rendered 0.0000001. It now renders 0.0000001 via format(value, "f"), which is plain at every magnitude — note this is a deliberate divergence from openjd-rs at the extremes, whose f64 Display emits 1e-07 here, because exponent notation must not reach a task command line.
Sorry, something went wrong.
| try: | ||
| if to_int: | ||
| return int(elem) | ||
| value = Decimal(elem).normalize() |
There was a problem hiding this comment.
Decimal.normalize() runs under the ambient decimal context, so it rounds to getcontext().prec significant digits (28 by default). That makes this a silent, lossy rewrite of a task parameter value:
Before this change the source text reached the renderer unchanged, so no precision could be lost. Since the only goal here is to strip redundant leading/trailing zeros, the operation does not need the default context: consider value.normalize(context=Context(prec=...)) with an explicitly large precision, or drop normalize() and strip the zeros without a context-sensitive arithmetic operation.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed and fixed. Measured: normalize() cut a 41-significant-digit element to 1.234567890123456789012345679 at the default prec=28, and with an embedding app setting getcontext().prec = 5 the same template rendered 1.2346 — library output was a function of host process state. Replaced with format(value, "f") plus text-level zero stripping, neither of which consults the context; verified byte-identical output under prec=5, and pinned by a new localcontext() regression test.
Sorry, something went wrong.
| # Numeric elements are left exactly as parsed. A FLOAT range of [1.0] | ||
| # renders 1.0, which openjd-rs also does and the conformance suite pins | ||
| # (base/jobs/3.4--float-parameter). STRING and PATH ranges are text by | ||
| # definition and are never touched. |
There was a problem hiding this comment.
This leaves the model rendering the same denoted number two different ways on a FLOAT parameter, depending only on how the element was spelled in the template:
But §2.4 defines a <floatstring> as "a string whose value is the string representation of" a float, and "1.0" is the string representation of the float 1.0. By the PR's own reasoning the element denotes 1.0, so a FLOAT parameter emitting --frame 0 where the template said "0.0" looks like the same class of bug being fixed here, in the opposite direction. It also means the two forms disagree even though the spec treats them as alternate spellings of one value.
Worth confirming against openjd-rs / the conformance suite before pinning "0.0" -> "0" in a test: if openjd-rs renders 0.0 for a "0.0" floatstring, then trailing-zero stripping should be limited to zeros beyond the first fractional digit (i.e. keep at least one), rather than reducing an integral float to bare digits.
Sorry, something went wrong.
There was a problem hiding this comment.
You are right, and I probed openjd-rs to settle it rather than argue from the fixture, which does not cover an integral float (3.4.1.2 uses ['1.5', '02.50']). The Rust CLI renders "1.0" as 1.0, "0.0" as 0.0, "007" as 7.0 and "100" as 100.0, so my normalize() was too aggressive and produced a spelling-dependent split against the <float> literal 1.0. FLOAT elements now always keep one fractional digit, "-0.0" renders 0.0 since the denoted number is unsigned (matching Rust), and the test that pinned "0.0" -> "0" is updated along with 007, 100 and 1E+2.
Sorry, something went wrong.
| if isinstance(exponent, int) and exponent > 0: | ||
| # normalize() rewrites Decimal('100') as Decimal('1E+2'); undo the | ||
| # shift so a range element never renders in exponent notation. | ||
| value = value.quantize(Decimal(1)) |
There was a problem hiding this comment.
quantize(Decimal(1)) raises InvalidOperation whenever the result would need more than getcontext().prec digits, and that exception is swallowed by the except (ValueError, ArithmeticError) below — which returns the original string. So for a large-exponent element the "never renders in exponent notation" guarantee falls back to whatever the template wrote:
Not a regression (the raw text was passed through before), but the fallback silently defeats the stated invariant rather than reporting anything. If exponent notation genuinely must never reach a task command line, formatting with format(value, "f") avoids the precision-bounded quantize entirely and handles arbitrary exponents in one step.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed exactly as described, and fixed. Measured: Decimal("1E+30").normalize().quantize(Decimal(1)) raises InvalidOperation because the result needs 31 digits against prec=28, the except (ValueError, ArithmeticError) swallowed it, and range: ["1E+30"] fell back to the raw string 1E+30 — defeating the invariant the guard existed to hold; "123456.75" under a narrowed context hit the same path. quantize is gone entirely: format(value, "f") has no precision bound, so "1E+30" now renders 1000000000000000000000000000000.0 and there is no fallback path left for a finite value.
Sorry, something went wrong.
|
CI note: the THIRD-PARTY-LICENSES check fails on upstream dependency drift, not on anything in this change. The diff is four Python source and test files (git diff --name-only upstream/mainline..HEAD) and touches no dependency manifest, so it cannot move a dependency version. The report diff is a pydantic patch bump that landed between #341's run and this one: -** pydantic; version 2.13.4 +** pydantic; version 2.13.5 -** pydantic_core; version 2.46.4 +** pydantic_core; version 2.46.5 Regenerating THIRD-PARTY-LICENSES.txt belongs in its own commit, since it will re-drift on the next pydantic release. Every other check is green: all 21 Python matrix jobs (macOS/Ubuntu/Windows, 3.9-3.14), all Rust jobs, cargo-deny, CodeQL and both Analyze legs. |
Sorry, something went wrong.
| the element unchanged when it does not denote one.""" | ||
| try: | ||
| if to_int: | ||
| return int(elem) |
There was a problem hiding this comment.
The docstring says the element is returned unchanged when it "does not denote" a number, but int() / Decimal() accept a strictly wider grammar than <intstring> / <floatstring>, so some non-conforming elements are silently rewritten instead of passed through:
These are reachable from a literal template range: the template-layer check (validate_int_fmtstring_field via validate_list_field) also uses int(), and validate_list_field discards its validator's return value, so today the raw text is what reaches the renderer. After this change the rendered task value differs for these inputs.
Edge-case severity, but since the point of the change is to make the rendered value match what the element denotes per the spec, it may be worth matching against the spec's grammar (e.g. a regex for <intstring> / <floatstring>) rather than delegating to Python's more permissive parsers.
Sorry, something went wrong.
There was a problem hiding this comment.
Accurate on the facts — int() and Decimal() do accept more than <intstring>/<floatstring>, and I reproduced "1_0" -> 10, " 5 " -> 5 and "+7" -> 7 — but I am declining this one. Every input that reaches it is a template already malformed under the spec grammar, so the only behaviour change would be swapping one undefined outcome for another, and a regex gate on both grammars is disproportionate to that. If you want the grammar enforced I would rather do it as validation at the template layer where the element type is already checked, as its own change with its own conformance fixture.
Sorry, something went wrong.
Regenerated versions for pydantic (2.13.4 -> 2.13.5) and pydantic_core (2.46.4 -> 2.46.5) to match the resolved dependency set, clearing the THIRD-PARTY-LICENSES check. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| return value | ||
| to_int = param_type != TaskParameterType.FLOAT | ||
| return [ | ||
| _normalized_range_element(elem, to_int) if isinstance(elem, str) else elem |
There was a problem hiding this comment.
The else elem branch skips every non-str element, so the "a range element never renders in exponent notation" invariant that _normalized_range_element establishes only holds for the <intstring>/<floatstring> spelling. A numeric FLOAT element takes this branch and can still render in exponent notation:
So the guard covers range: ["1E+2"] (there is a test for it) but not the numerically-typed forms of the same value, which is the asymmetry the quantize branch two functions up exists to prevent.
If exponent notation must never reach a task command line, the guard belongs after the str/non-str split — e.g. normalize the exponent for any Decimal/float element as well, rather than only for elements that arrived as text.
Sorry, something went wrong.
There was a problem hiding this comment.
Correct on the facts, but pre-existing rather than a regression, so I am declining it here rather than ignoring it. Checked on mainline (f7ca58f): a Decimal("1.0e+16") element already rendered 1.0E+16 there, a float 1e16 rendered 1e+16 and 1e-7 rendered 1e-07, and this branch leaves all three byte-identical — numeric elements were passed through untouched before it and still are. Normalizing them is deliberately out of scope for this PR because that pass-through is what keeps 3.4--float-parameter passing (it pins TASK:Scale=1.0 from a literal 1.0), so it needs its own conformance decision about what a numerically-typed <float> element renders as; worth its own issue.
Sorry, something went wrong.
Review found three defects that share one root cause: Decimal.normalize() plus a positive-exponent quantize() guard is both context-sensitive and notation-unstable. - normalize() rounds to getcontext().prec. A 41-significant-digit element came back as 1.234567890123456789012345679, and an embedding application that sets getcontext().prec = 5 changed what this library rendered for the same template (measured: '1.2345678901234567890123456789012345678901' -> '1.2346'). Template output must not depend on host process state. - str(Decimal) switches to exponent notation once the adjusted exponent is below -6, which the positive-exponent guard did not cover, so the element '0.0000001' rendered 1E-7. That was a regression: before normalization was introduced the source text was kept and it rendered 0.0000001. - quantize(Decimal(1)) raises InvalidOperation once the result needs more digits than getcontext().prec, and the except clause swallowed it and returned the raw string. '1E+30' therefore rendered as 1E+30, silently defeating the very invariant the guard existed to hold. '123456.75' under a narrowed context hit the same path. Render with format(value, 'f') instead. With no precision in the format spec it is exact and plain at every magnitude, so it needs neither the rounding of normalize() nor the precision-bounded quantize(): verified identical output under getcontext().prec = 5. Redundant leading and trailing zeros are then stripped from the text, which is not an arithmetic operation and cannot consult the context. An integral <floatstring> now keeps one fractional digit, so '1.0' renders 1.0 and not 1. openjd-rs was measured as the reference for this: it renders '1.0' as 1.0, '0.0' as 0.0, '007' as 7.0 and '100' as 100.0. Dropping the fraction made the same denoted number render two ways depending on whether it was spelled as a <float> or a <floatstring>, since a <float> literal 1.0 renders 1.0. The conformance fixture does not settle it -- 3.4.1.2 uses ['1.5', '02.50'], neither of them integral -- so the Rust implementation was probed directly. '-0.0' renders 0.0, since the number it denotes is zero, which openjd-rs also does. Plain notation is a deliberate divergence from openjd-rs at the extremes, where its f64 Display gives 1e-07 and 1e+30. Exponent notation must not reach a task command line at any magnitude, and Decimal carries the written value exactly where f64 cannot. Scope is unchanged. Numeric literals are still untouched, STRING and PATH ranges are still text, and job parameter defaults are not involved. 2023-09/base/jobs/3.4--float-parameter and 2023-09/EXPR/jobs/expr1.3.4--float-passthrough both still pass, as do both proposed 3.4.1.1/3.4.1.2 normalization fixtures; the full 2023-09 conformance suite is 1172 passed, 0 failed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| # ambient precision, str() switches to exponent notation below 1e-6, | ||
| # and quantize() raises InvalidOperation once the result needs more | ||
| # digits than the ambient precision allows. | ||
| text = format(value, "f") |
There was a problem hiding this comment.
format(value, "f") is unbounded in output size, and the input is attacker-controlled template text, so this looks like a memory-exhaustion hazard on job creation.
Decimal("1e999999999") constructs fine — Emax/Emin from the context constrain arithmetic results, not construction from a string — and is_finite() is True, so the guard above does not stop it. format(d, "f") then has to materialize the full plain-notation expansion, i.e. on the order of 10^9 characters for that exponent (and likewise for a large negative exponent such as "1e-999999999"). That is a multi-gigabyte allocation, or an effectively unbounded CPU/memory spike well before that.
It is reachable from a plain template: a FLOAT task parameter range: ["1e999999999"] passes the template-layer check, since validate_float_fmtstring_field only does Decimal(str(value)) and imposes no exponent bound. The element then arrives here as a str on the instantiation target and gets expanded. The previous str()/normalize() formulations did not have this property — str(Decimal) keeps exponent notation and is O(digits) — so this is specific to the format(..., "f") approach.
Worth bounding the exponent before expanding, e.g. reject (or leave as text) when value.adjusted() or -value.as_tuple().exponent exceeds some sane limit, so the plain-notation length is capped by construction rather than by the input.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed and fixed. Measured: Decimal("1e999999999") is finite exactly as you say, and the expansion is exponent+1 characters — parsing range: ["1e100000000"] on the instantiation target cost 0.37s and 266 MiB peak RSS from 11 characters of template text, and the 1e999999999 case killed the probe process outright. The length is now computed from adjusted()/as_tuple().exponent before anything is materialized, bounded at the 1024-character cap TaskParameterStringValueAsJob already enforces on this field, and an element over it keeps its source text — "1e999999999" now parses in 0.01 ms at 42 MiB.
Sorry, something went wrong.
| # The number denoted by '-0.0' is zero, which has no sign; openjd-rs | ||
| # renders it `0.0`. | ||
| negative = False | ||
| return f"{'-' if negative else ''}{integer}.{fraction}" |
There was a problem hiding this comment.
Normalization can make an element longer than the 1024-char cap the field type enforces, so an input that validated before this change can now fail (or silently change type) at the same point.
Elements of TaskRangeList are Union[TaskParameterStringValueAsJob, int, float, Decimal] and TaskParameterStringValueAsJob carries StringConstraints(max_length=1024). format(value, "f") turns a short source text into its full plain-notation expansion, so a FLOAT element written as "1E+1030" — 7 characters, well under every existing limit and accepted by the template-layer check — becomes a 1033-character string here. That fails the str member of the union, and rather than erroring cleanly pydantic then falls through to the numeric members in lax mode, where float("1000…0.0") overflows to inf. Either way the result is wrong for a template that was previously valid: an error on a conforming template, or a rendered task value of inf.
This is distinct from the unbounded-expansion concern on the format(..., "f") line above (which needs an exponent in the billions to hurt): here the threshold is around 1e1024, so it is well within what a template can plausibly contain and it costs nothing to trigger. A single exponent/length bound before expanding would cover both.
Also worth a test: the existing coverage stops at "1E+30", which stays comfortably under the cap.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed and fixed, with one correction to the predicted mechanism. Measured on the instantiation target: the threshold is a 1025-character expansion, and "1E+1022" fell through the str member to int and rendered 1023 digits rather than inf, while "1E-1023" fell through to float, underflowed and rendered 0.0 — silently losing the value; mainline rendered both as their source text, so both were previously valid. Both directions are now bounded by that same 1024-character cap before expansion, "1E+1021" and "1E-1022" expand to exactly 1024 and are still normalized, and there are tests one past the cap and at it.
Sorry, something went wrong.
Rendering a <floatstring> range element with format(value, 'f') is unbounded
in the element's exponent, which review found has two reachable faces.
Unbounded expansion. Decimal construction from a string is not bounded by the
context -- Emax/Emin constrain arithmetic results, not construction -- so
Decimal('1e999999999') is finite and the is_finite() guard did not stop it.
The plain-notation length is exponent + 1, measured: 10**7 gives 10,000,001
characters. Parsing a FLOAT range of ['1e100000000'] on the instantiation
target took 0.37s and 266 MiB peak RSS from 11 characters of template text;
the 1e999999999 case is 10x that and killed the probe process. It is reachable
from an ordinary template, because the template layer accepts the element and
keeps it as a TaskParameterStringValue.
Silently exceeding the field cap, far lower down. A TaskRangeList element's str
member is TaskParameterStringValueAsJob, capped at 1024 characters. Measured:
'1E+1022' -- 7 characters, accepted before this branch -- expands to 1025 and
so fails that member, and pydantic then falls through to the numeric members,
rendering the element as a 1023-digit int. In the other direction '1E-1023'
expands to 1025, fails the same way, and renders 0.0, silently losing the
value. Both were valid text-rendering elements on mainline, which rendered
'1E+1030' as 1E+1030.
Bound the length from value.adjusted() and the exponent before materializing
anything. The limit is the field's own 1024-character cap rather than a new
number, because an expansion past it cannot be carried here as text at all, so
there is nothing to gain by producing one. An element over the bound keeps its
source text -- what this function already does for anything it cannot
normalize, and how such an element rendered before normalization was
introduced -- so there is no new rejection path and no new error type. The
bound is exact at the boundary: '1E+1021' and '1E-1022' expand to exactly 1024
and are still normalized.
After the fix '1e999999999' parses in 0.01ms at 42 MiB peak RSS.
Model suite 5521 -> 5527 passed (6 new tests, 24 skipped, 3 xfailed). The two
boundary tests fail against 2109e2f, rendering 1000...000 and 0.0. Conformance
3.4.1.1, 3.4.1.2, 3.4--float-parameter and expr1.3.4--float-passthrough all
pass.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
What
A task parameter range element written in the <intstring> / <floatstring> string form now carries the number it denotes instead of its source text.
The spec reading being adopted
Template Schemas 2023-09 §3.4.1.1 (L1110) makes an <IntRangeList> element <integer> | <intstring>, and §3.4.1.2 (L1184) makes a <FloatRangeList> element <float> | <floatstring>. Neither is behind an extension gate. §2.3 (L318) and §2.4 (L375) define <intstring> / <floatstring> as "a string whose value is the string representation of" a number, so such an element denotes the number, not the text it was written with.
The reading is scoped to the string forms, and deliberately so:
openjd-rs already behaves this way and passes both conformance fixtures. It parses every range element into an i64/f64 at create_job (crates/openjd-model/src/job/create_job/ranges.rs), so it has no source text to carry in the first place.
Why Python was keeping the text
The pre-validator on the template models already computes the value — the element validator evaluates int('02') == 2 in validate_int_fmtstring_field — but validate_list_field returns the original list object, discarding every coercion. Nothing downstream recovered it.
The fix is applied one layer later, on RangeListTaskParameterDefinition, the instantiation target that all three inbound range paths funnel through: a literal list, a range-expression expansion, and an RFC 0006 typed whole-field resolution. Keying off the already-validated type field, an INT or CHUNK[INT] element becomes an int and a FLOAT element becomes a normalised Decimal.
Decimal.normalize() needs one guard: it rewrites Decimal('100') as Decimal('1E+2'), so a positive exponent is quantized away. Exponent notation must never reach a task command line.
An element that does not parse as a number is carried through unchanged rather than rejected here. A literal range is already checked against its element type at template parse time, and adding a rejection path for values arriving from a resolved format string is a separate change.
A latent containment bug this exposed
Normalising the stored elements turned six existing tests red, and the cause was already a defect on mainline.
RangeListIdentifierNode.validate_containment compared ParameterValue.value — the rendered form of a range element — against a set built from the raw elements. It only matched when a range happened to be written as strings. An INT range written as [1, 2, 3] reported every one of its own values as not contained, before this change:
The containment set now holds rendered elements. TestRangeListElementValues::test_containment_matches_the_rendered_values pins both forms.
Verification
Model test suite: 5483 passed before, 5505 passed after (22 added), coverage 94.10% against the 94% gate. Every new assertion was checked for falsifiability by reverting each half of the fix separately: 8 of the 15 model-layer tests fail without the normalisation, and both containment cases fail without the range_set repair.
Conformance fixtures, run by direct file path against a venv built from this branch:
These two fixtures were the only conformance failures the Python implementation had. A full 2023-09/* run on this branch is 1173 passed, 1 failed, and that one failure is 3.6--let-step-bindings-in-step-env, which is unrelated and waits on #341.
Sweep before writing the fix. Across the whole suite (1,193 files) there are 1,046 FloatRangeList elements and 2,420 IntRangeList elements. Exactly two FLOAT elements and three INT elements use the string form, and all five are in the two fixtures above — so nothing else in the suite asserts a rendering that this change moves.
The sweep also found the constraint that scoped the fix: 1,030 FLOAT elements are numeric literals whose rendering would change if numeric literals were normalised too, and one of them is asserted by an executing fixture (3.4--float-parameter asserts TASK:Scale=1.0 from a literal 1.0). Restricting normalisation to the string forms avoids that conflict without touching any existing assertion.
ruff, black and mypy clean at the CI-pinned versions.
Related
Known remaining divergence
An integral <floatstring> still renders differently from openjd-rs: range: ['1.0'] on a FLOAT parameter renders 1 here and 1.0 there, because openjd-rs formats every integral f64 with a trailing .0 while this implementation preserves the scale a Decimal carries. No conformance fixture covers it, and closing it would change how numeric FLOAT literals render, which 3.4--float-parameter pins. Left alone deliberately.