fix: Normalize intstring/floatstring task parameter range elements - #342
Conversation
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>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # 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:
"0.0000001"becomesDecimal("1E-7")and renders as1E-7(previously the source text0.0000001reached the renderer verbatim)"0.00000025"becomesDecimal("2.5E-7")and renders as2.5E-7
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.
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.
| 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:
"1.1234567890123456789012345678901234567890"is rounded to 28 significant digits instead of being carried through exactly, and- because the context is process-global mutable state, an embedding application that has done
getcontext().prec = 5anywhere changes what range values this library produces —"1.234567"would become1.2346.
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.
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.
| 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:
range: ["1E+30"]on a FLOAT parameter ->normalize()givesDecimal("1E+30"), exponent 30 > 0,quantizeneeds 31 digits > prec 28 ->InvalidOperation-> the function returns the string"1E+30", which renders as1E+30.
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.
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.
|
CI note: the Regenerating |
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>
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>
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>
Review on OpenJobDescription#342 pointed out that the string form of a <FloatRangeList> element exists in order to carry the scale it was written with: a range element written '2.50' is asking for `2.50` on a command line, and a renderer handed `2.5` instead has been given a different string. It is also the only way a template can ask for a fixed number of decimal places, because a <float> literal of 2.50 is the same literal as 2.5 once parsed. OpenJobDescription#342 normalized the element to the number it denotes and threw that away. Split the two kinds of zero, which are not the same thing: - Leading zeros are not part of the number and still go. '02' on an INT range is the task value 2, and forwarding the text renders `--frame 02`. - Trailing zeros are the author's chosen scale and are kept. '02.50' renders `2.50`, not `2.5`. The already-landed conformance fixture EXPR/jobs/expr1.3.4--float-passthrough pins the same rule for a FLOAT parameter default, so both it and base/jobs/3.4.1.2 can now be right at once. openjd-specifications#180 states the rule as Template Schemas §7.5. A <floatstring> therefore renders as its own text less any redundant leading zeros, which is a substring operation rather than a numeric one. That is what removes the machinery the two preceding commits added, because every problem they were solving came from having to choose a notation in which to re-render a Decimal: - normalize() and quantize() round to getcontext().prec, so an embedding application's decimal context changed what this library rendered: measured, '1.2345678901234567890123456789012345678901' came back as '1.2346' under getcontext().prec = 5. - str(Decimal) switches to exponent notation once the adjusted exponent falls below -6, so '0.0000001' rendered 1E-7. - format(value, 'f') is exact and plain at every magnitude but unbounded in the exponent, so '1e999999999' -- 11 characters of template text -- expanded to ~10**9 characters and needed a length bound to stay safe. Text needs none of those decisions. '1e999999999' now costs its own 11 characters, so the exponent bound and its boundary cases are gone with it, and an element can no longer silently exceed TaskParameterStringValueAsJob's 1024-character cap and fall through to a numeric member of the union. An <intstring> is unaffected: int() already discards leading zeros and there are no fractional digits to preserve. Verification. Model suite 5527 -> 5529 passed, 24 skipped, 3 xfailed; ruff, black and mypy clean at the CI-pinned versions. Three mutants confirm the tests pin both halves of the rule: dropping the leading-zero strip fails 7 tests, restoring the trailing-zero strip fails 7, and dropping the int() conversion fails 3. Full 2023-09 conformance suite, run from source against openjd-cli mainline 7c7ece4 with openjd-sessions 0.10.14, on openjd-specifications#180's tree: 1162 passed, 0 failed. The baseline at e7a17b3 passes 1161 and fails only base/jobs/7.5--numeric-string-zeros-in-range-elements, so this closes that one fixture and moves nothing else. Both controls hold -- base/jobs/3.4--float-parameter and EXPR/jobs/expr1.3.4--float-passthrough. Relative to released 0.11.6 the only behaviour change here is the leading-zero trim: every element whose rendering differs from 0.11.6 differs by a removed leading zero. openjd-rs, which backs this package's `openjd.model._v1` API, needs the matching change; that is OpenJobDescription/openjd-rs#354. With both in place the two implementations render every range-element case tested identically, including the zero and exponent forms. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
3 participants
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:
'02'is the task value2.range: [1.0]still renders1.0. openjd-rs renders an integral float the same way, and the suite pins it in2023-09/base/jobs/3.4--float-parameter.defaultvalues do not normalise. Defaults live on the job-parameter definitions and ranges on the task-parameter definitions — separate models, and this change touches only the latter. openjd-rs makes exactly the same split.2023-09/EXPR/jobs/expr1.3.4--float-passthrough, which pins a FLOATdefault: "3.500"rendering verbatim asPARAM:3.500, still passes.openjd-rs already behaves this way and passes both conformance fixtures. It parses every range element into an
i64/f64atcreate_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') == 2invalidate_int_fmtstring_field— butvalidate_list_fieldreturns 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-validatedtypefield, an INT orCHUNK[INT]element becomes anintand a FLOAT element becomes a normalisedDecimal.Decimal.normalize()needs one guard: it rewritesDecimal('100')asDecimal('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_containmentcomparedParameterValue.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_valuespins 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_setrepair.Conformance fixtures, run by direct file path against a venv built from this branch:
base/jobs/3.4.1.1--int-range-intstring-elements-normalizedbase/jobs/3.4.1.2--float-range-floatstring-elements-normalizedbase/jobs/3.4--float-parameter(numeric-literal control)EXPR/jobs/expr1.3.4--float-passthrough(default scope guard)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 is3.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
FloatRangeListelements and 2,420IntRangeListelements. 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-parameterassertsTASK:Scale=1.0from a literal1.0). Restricting normalisation to the string forms avoids that conflict without touching any existing assertion.ruff,blackandmypyclean at the CI-pinned versions.Related
proposed/. They were parked there pending a ruling on whether<floatstring>normalises; this PR adopts the normalising reading for range elements while leaving parameter defaults verbatim, which is the split that lets the landedexpr1.3.4--float-passthroughfixture and these two coexist.letbindings in template scope) is independent; neither touches the other's files.Known remaining divergence
An integral
<floatstring>still renders differently from openjd-rs:range: ['1.0']on a FLOAT parameter renders1here and1.0there, because openjd-rs formats every integralf64with a trailing.0while this implementation preserves the scale aDecimalcarries. No conformance fixture covers it, and closing it would change how numeric FLOAT literals render, which3.4--float-parameterpins. Left alone deliberately.