-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Implement WRAP_ACTIONS with EXPR binding to RS #285
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
bce1eef
feat: Implement WRAP_ACTIONS with EXPR binding to RS
leongdl 2db9342
feat: Pass conformance test
leongdl 78b2eae
feat: add build_symbol_table binding for typed EXPR symbol tables
leongdl f923082
feat: expr and wrap_actions pr comments
leongdl 0c69c6e
feat: v0 parity for RFC 0007 typed params and RFC 0008 wrap-action rules
leongdl ca33b9d
feat: reuse Rust via bindings to simplify the EXPR model glue
leongdl 4611561
feat: scope-check timeout format strings and cover RANGE_EXPR typed v…
leongdl 1250821
feat: harden EXPR/WRAP_ACTIONS v0 model — spec string coercion, value…
leongdl 3c7bab5
test: cover create_job instantiation of WRAP_ACTIONS templates
leongdl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
PR #285 — EXPR / WRAP_ACTIONS: key call stacks
Repo: OpenJobDescription/openjd-model-for-python · branch
python-rsWhat the PR does: teaches the pure-Python (v0) model to handle the RFC
0005–0008 expression extensions by delegating
{{ }}parsing, type-checking,and evaluation to the Rust
openjd-exprengine over PyO3 bindings.TL;DR
Before this PR the pure-Python model only understood the legacy
Name.Dot.Nameinterpolation grammar. This PR makes it a thin front-end overthe Rust expression engine whenever a template declares the
EXPRextension,and adds the parameter types and wrap-action rules that ride on top of it.
EXPRis declared,{{ }}parsing/eval routes toa new Rust-backed
ExprNode(arithmetic, functions, lists, paths,let,comprehensions) instead of the legacy name parser. Non-EXPR templates are
byte-for-byte unchanged, and the Rust surface is imported lazily so the
non-EXPR path never loads it.
fed to a new
ParsedExpression.typecheck()that catches real type errors atdecode time without false-rejecting runtime-dependent expressions (no more
error-string sniffing).
BOOL,RANGE_EXPR, and theLIST[*]family — with case-insensitive type names, item/length constraints,and an EXPR-extension gate, all sharing one base class.
ParameterValue.value/JobParameter.valuewiden from
strtoAnyso lists/bools survivecreate_jobintact; a newSymbolTable.expr_typesfield carries the type tags so the Rust buildercoerces correctly (and is preserved across copy/union).
RFC 0005 coercion via
evaluate_to_str()({{ true }}→"true", null →"", quoted list items) instead of Python'sstr().onWrapEnvEnter/onWrapTaskRun/onWrapEnvExitonEnvironmentActions, gated onWRAP_ACTIONS+EXPR, all-or-nothing, withper-hook
Wrapped*variable scoping and a job-wide single-wrap-layer rule.build_symbol_table,ParsedExpression.typecheck,job_parameter_type_expr_spec(single-sources the OpenJD→EXPR type mapping).Risk shape: almost all behavior change is gated behind the
EXPR/WRAP_ACTIONSextension declarations, so templates that don't opt in areunaffected. The sharp edges are at the Python↔Rust boundary (the three flows
that cross it) and the
str → Anyvalue widening.Two flows form the spine of the change: (1) decode-time validation of an
expression's symbol references, and (2) runtime resolution of an expression
into the string substituted into a format string.
──BOUNDARY──▶marks a callcrossing from Python into compiled Rust;
◀──marks the return.Flow 1 — Decode-time: validate an EXPR
{{ }}expression's symbolsEntry:
decode_job_template(...)→ pydantic validation → the variable-referenceprevalidator.
Key insight: step 16's
typecheck()discards the result, so a "well-typedbut unresolved at runtime" expression passes cleanly — the model no longer has
to sniff Rust error-message strings to tell a real type error from a
runtime-dependent one. (The
let-binding path reaches the sameExprNode.validate_symbol_refsfrom_variable_reference_validation.py:606,building one
ExprNodeper binding.)Flow 2 — Runtime: resolve a
{{ }}expression to its substituted stringEntry: a caller invokes
FormatString.resolve(symtab=...)during jobinstantiation / argument resolution.
Key insight: step 12's
str(ExprValue)is the fix for the{{ true }}→"True"Python-repr bug — it uses the engine's RFC 0005 coercion(
true/false, double-quoted list items, preservedDecimaltrailing zeros),not Python's
str(). Step 9 is where theexpr_typesmetadata earns its keep:if that map is ever dropped on a
SymbolTablecopy/union, coercion silentlyreverts to inference and produces a wrong-typed value — which is exactly why the
PR promotes
expr_typesto a real field that is copied in__init__andunion.ExprNode.evaluate(_nodes.py:231) instead calls.item()to handback the native Python value for callers that want the typed result.
Flow 3 — Decode-time: a new typed job parameter (BOOL / LIST[*] / RANGE_EXPR)
The PR adds eight EXPR-extension job-parameter types. They share one base class
and a normalize-then-discriminate decode path. Entry:
decode_job_template(...)on a template whose
parameterDefinitionscontains, say,{"type": "list[int]"}.Key insight: every concrete list type is just a
typeliteral + anitemconstraint + a one-line
_check_itemoverride on the shared base — the lengthcheck, EXPR gate, create-job metadata, and template-variable defs all live once
on
_JobListParameterDefinitionBase.RANGE_EXPR(_model.py:3644) is the oddone out: it validates its default string through the existing
IntRangeExprgrammar rather than a list check. Two-stage decode (normalize before,
discriminate after) is why a lowercase
list[int]resolves to the rightclass instead of failing the union.
Flow 4 — Instantiation: create_job carries native values for the new types
Legacy scalars (STRING/INT/FLOAT/PATH) are stringified through preprocessing;
the new EXPR types must keep their native Python value (a real
list/bool) sothe typed symbol-table builder (Flow 2, step 8) can coerce them. Entry:
create_job(job_template, job_parameter_values).Key insight: this is the producer side of the
expr_typescontract thatFlow 2 consumes. The widened
ParameterValue.value: Any(wasstr) is what letsa
[1, 2, 3]survive instantiation intact; the review confirmed no downstreamconsumer string-ops that value, and the type tag travels alongside it via
symtab.expr_typesso the Rust builder coerces correctly.Merge wrinkle —
_merge_job_parameter.pyWhen the same parameter is defined in multiple templates, only the four legacy
scalar types take the cross-merge path (
merge_job_parameter_definitions_for_one:191, gated on
_LEGACY_CONSTRAINT_TYPES). EXPR types skip it and return thelast-defined definition via
model_copy(:241) — which bypasses validators, sothe PR re-runs
getattr(base, "_check_constraints", None)(:235) on the carrieddefault to avoid letting a constraint-violating default slip through.
Flow 5 — Decode-time: WRAP_ACTIONS structural validation (RFC 0008)
Two independent checks fire when
WRAP_ACTIONSis declared: per-EnvironmentActionsshape/scope, and a job-wide single-wrap-layer rule.
Key insight: the three checks live at different altitudes on purpose —
extension/all-or-nothing gating per
EnvironmentActions(before), per-hookvariable scoping per
EnvironmentActions(after), and the cross-cuttingsingle-layer count at
JobTemplate(after, where it can see jobEnvironments +all steps). The scope check (B) deliberately inspects the
timeoutFormatStringtoo, not just command/args, because
timeoutis a format string underFEATURE_BUNDLE_1.
Reference — type changes that ripple through the flows
ParameterValue.value: str → Any_types.pyJobParameter.value: str → Anyv2023_09/_model.pySymbolTable.expr_types: dict[str,str](new field)_symbol_table.py__init__/unionso coercion (Flow 2 step 8) never silently reverts to inferenceExpressionInfo.resolved_value: Optional[Union[Real,str]] → Optional[str]_format_string.pyresolve()now appends the value directly (str() wrapper dropped);evaluate_to_strguarantees a stringNode.allows_nonscalar+evaluate_to_str()(new)_nodes.pyFullNameNodestays str/Real-onlyvalidate_symbol_refs(..., symbol_types=...)(new kwarg)_nodes.py/_expression.pyDefinesTemplateVariables.expr_inject(new)_types.pyStep.Name) applied by the prevalidator, which has the parsing contextbuild_symbol_table,ParsedExpression.typecheck,job_parameter_type_expr_specrust-bindings/