-
Notifications
You must be signed in to change notification settings - Fork 22
feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005-0008) #318
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 | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -142,6 +142,31 @@ def _run_env_action( | |||||||||||||||||||||||||||||
| self._action = action | ||||||||||||||||||||||||||||||
|
Contributor
Author
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. PR #318 — WRAP_ACTIONS runtime + EXPR
|
||||||||||||||||||||||||||||||
| Concern | Python (this PR) | Rust (openjd-rs/crates/openjd-sessions/src/session.rs) |
Parity? |
|---|---|---|---|
| Wrap-env selection | _active_wrap_env / _wrap_env_excluding, innermost-wins |
active_wrap_env / wrap_env_excluding, identical traversal |
✅ matches |
WrappedAction.Timeout |
int(inner_action.timeout) (raises on FormatString) |
resolve_action_timeout(...) resolves the FormatString then parses |
⚠ diverges |
let evaluation |
re-parses RHS as ArgString("{{ rhs }}") in Python |
evaluate_let_bindings(...) (one Rust call) — also exposed to Python as openjd._openjd_rs.evaluate_let_bindings |
⚠ reimplements |
let timing vs embedded files |
before file materialization (documented limit) | step runner evaluates let first, then materializes files |
✅ matches (intentional first-cut) |
WrappedAction.Environment |
_openjd_defined_env() excludes host vars |
session_env_vars (openjd_env exports only), host excluded |
✅ matches |
| Args list flattening | str(element) per list/tuple element |
elem.to_display_string() per list element |
⚠ verify coercion match |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| TerminateCancelMethod, | ||
| ) | ||
| from ._session_user import SessionUser | ||
| from ._types import ActionState, StepScriptModel | ||
| from ._types import ActionModel, ActionState, StepScriptModel | ||
|
|
||
| __all__ = ("StepScriptRunner",) | ||
|
|
||
|
|
@@ -95,6 +95,29 @@ def __init__( | |
| if not isinstance(self._script, StepScript_2023_09): | ||
| raise NotImplementedError("Unknown model type") | ||
|
|
||
| def run_wrap_action( | ||
| self, | ||
| action: ActionModel, | ||
| *, | ||
| default_timeout: Optional[timedelta] = None, | ||
| ) -> None: | ||
| """Run a caller-supplied wrap action (RFC 0008) against this runner's | ||
| prepared symbol table. | ||
|
|
||
| Used by the Session's WRAP_ACTIONS dispatch when an active wrap | ||
| environment defines ``onWrapTaskRun``: the Session seeds the symbol | ||
| table with ``WrappedAction.*`` / ``WrappedStep.Name`` and supplies the | ||
| wrap action here, which runs instead of the step's own ``onRun``. | ||
|
|
||
| Unlike :meth:`run`, this does NOT materialize the step's embedded files | ||
| or evaluate let bindings — the caller prepares the symbol table. This | ||
| mirrors the Rust ``run_wrap_action`` first-cut behavior. | ||
| """ | ||
| if self.state != ScriptRunnerState.READY: | ||
| raise RuntimeError("This cannot be used to run a second subprocess.") | ||
| log_subsection_banner(self._logger, "Phase: Setup") | ||
| self._run_action(action, self._symtab, default_timeout=default_timeout) | ||
|
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. Cancelation of a running wrap task action uses the wrong action's cancelation config. |
||
|
|
||
| def run(self) -> None: | ||
| """Run the Step Script's onRun Action.""" | ||
| if self.state != ScriptRunnerState.READY: | ||
|
|
||
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.
Review — PR #318:
feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005–0008)Repo: OpenJobDescription/openjd-sessions-for-python ·
wrap-actions-v0→mainlineAuthor: leongdl · +596 / −5, 6 files · state: open
Reviewed against: the Rust reference at
openjd-rs/crates/openjd-sessions/src/session.rsand the
openjd-modelPython/Rust bindings.Summary
This PR teaches the pure-Python (v0)
Sessiontwo new tricks from the OpenJDexpression RFCs. First, RFC 0007
letbindings:enter_environment,exit_environment, andrun_tasknow evaluate a script'sletlist into thesymbol table before resolving the action, so an action's
{{ }}expressions canreference computed values. Second, RFC 0008 WRAP_ACTIONS: when an active
"wrap" environment declares
onWrapEnvEnter/onWrapTaskRun/onWrapEnvExit,that hook runs in place of an inner environment's
onEnter, a step'sonRun,or an inner environment's
onExit— withWrappedAction.*/WrappedEnv.Name/WrappedStep.Nameseeded from the inner action and resolved through the model'sRust EXPR engine. A new
run_wrap_actionentry point is added to both scriptrunners,
run_taskgains an optionalstep_name, and theopenjd-modelpinmoves to
>= 0.11,< 0.12.The change is well-structured and well-commented, the dispatch logic faithfully
mirrors the Rust reference (innermost-wins wrap selection, never-self-wrap,
host-vars-excluded environment), and the two new test files cover all three
hooks plus the no-wrap and chained-
letpaths end-to-end. The findings below aremostly about the Python↔Rust boundary and one real edge-case bug.
Call stacks (Phase 1 — required)
Full traces with file:line hops and boundary crossings are saved to:
review/call-stacks/318-wrap-actions-v0.mdThe three primary flows:
Key insight: wrap actions resolve their
{{ WrappedAction.* }}referencesthrough the same Rust
build_symbol_tablecoercion the rest of the model uses —which is why
_seed_wrapped_action_symbolsregistersexpr_typesfor each seededsymbol. The dispatch is correctly gated: nothing changes for templates that don't
declare a
letfield or a wrap hook.Findings
Correctness
1.
_session.py:1577—int(inner_action.timeout)raises on a FormatString timeout (FEATURE_BUNDLE_1). (PLAUSIBLE)Action.timeoutisOptional[Union[PositiveInt, FormatString]](
openjd-model .../v2023_09/_model.py:446). Under the FEATURE_BUNDLE_1 extensiona timeout may be a
{{ }}format string (e.g. one referencing a task parameter)that is still unresolved at runtime.
int("{{ Task.Param.t }}")raisesValueError: invalid literal for int()— which here would crash wrap-actionseeding rather than produce
WrappedAction.Timeout.resolve_action_timeout(...)(runner/mod.rs:234), which resolves the formatstring through EXPR then parses the integer. This is a genuine Python-vs-Rust
divergence (the ★ parity rule), latent until a wrapped action carries a
FormatString timeout.
a few lines up, then coerce: e.g. resolve
inner_action.timeoutvia theFormatString path when it's a
FormatString, fall back toint(...)only forthe already-numeric case. Mirror
resolve_action_timeout's "0/empty = unset"semantics.
valid wrapped-action template runs), so it warrants a conformance fixture once
fixed — both polarities (FormatString timeout resolves vs. malformed rejected).
Boundary / exceptions / reuse
2.
_session.py:1184_apply_let_bindingsreimplementsletevaluation in Python instead of calling the Rust binding the model already exposes. (reuse — item #5)The model already ships
openjd._openjd_rs.evaluate_let_bindings(bindings, symtab, *, profile=None)(declared in
_openjd_rs.pyi:3595, implemented atrust-bindings/src/model/create_job_fns.rs:218), and the Rust session usesexactly that (
let_bindings.rs→openjd_model::evaluate_let_bindings). ThisPR instead re-parses each RHS as
ArgString_2023_09("{{ " + rhs + " }}")andcalls
.evaluate(...)per binding in Python.letsemantics willdrift from the canonical Rust one (binding scope/ordering, error messages, the
name = exprsplit, profile/function-library handling). The Rust path takes thewhole
bindingslist and the function library in one call; the Python loop doesa naive
partition("=")that, e.g., would mishandle an=inside the RHSexpression differently than the model's
parse_let_bindingsvalidator. It'salso ~40 lines that could be ~3.
evaluate_let_bindings(let_bindings, symtab, profile=...)and merge the returned symbol table, matching the Rust session. This also gets
the
expr_typeshandling (Finding 3) for free, since the binding goes throughthe same builder.
3.
_session.py:1184—_apply_let_bindingsstores values without recordingsymtab.expr_types, unlike_seed_wrapped_action_symbols. (PLAUSIBLE)_seed_wrapped_action_symbolscarefully registersexpr_typesfor each seededsymbol so the Rust
build_symbol_tablecoerces it correctly on the next resolve;_apply_let_bindingsdoessymtab[name] = expression.evaluate(...)with noexpr_typesentry.letbinds a non-scalar (a list) and a lateraction/arg references it expecting list-flattening, the absence of a type tag
means coercion falls back to inference — the exact silent-revert hazard the
model's
expr_typesfield exists to prevent. The chained-int test passes, butthere's no test for a
let-bound list consumed by an arg.structurally. If keeping the Python path, record the result's EXPR type in
symtab.expr_types[name]. Either way, add a regression test for alet-boundlist referenced from
args.4.
_runner_env_script.py:166vs_runner_step_script.py—run_wrap_actioncancel asymmetry. (PLAUSIBLE / worth a comment)The env runner's
run_wrap_actionsetsself._action = action(line 166); thestep runner's does not. On cancel, the env runner reads
self._action.cancelation(the wrap action's config) while the step runner reads
self._script.actions.onRun.cancelation(the inner action's config).runner's choice is arguably the correct one — which makes the step runner the
odd one out (it would apply the inner onRun's cancelation to a wrap process).
Either way the two runners disagree, and nothing documents why.
(likely the wrap action's, since that's the live process), apply it
consistently in both runners, and add a one-line comment. Confirm the intended
behavior against the Rust runner's
run_wrap_action.Cleanup / minor
5.
_session.py:1572&:1585— defensivegetattr(symtab, "expr_types", ...)/# type: ignore[attr-defined]are unnecessary. (dead/defensive code)SymbolTable.expr_typesis a real declared field initialized in__init__andcopied in
union(openjd-model .../_symbol_table.py:22,32,76). Thegetattr(symtab, "expr_types", None) or {}fallback and the# type: ignore[attr-defined]on the assignment treat it as optional/untyped.symtab.expr_typesdirectly (it's always a dict) and dropthe
type: ignore. Minor, but it removes a "this attribute might not exist"signal that's no longer true.
6.
_session.pyenter_environment/exit_environment—getattr(env_script, "let", None). (minor)EnvironmentScriptdeclareslet: Optional[list[str]]as a real field, so thegetattr(..., "let", None)guard (vsstep_script.letused directly inrun_task) is inconsistent and slightly obscures intent. If the guard is onlyfor the
env_script is Nonecase, the surroundingif env_script is not Nonealready covers it. Prefer direct attribute access for symmetry with
run_task.What's good
innermost-wins
_active_wrap_env,_wrap_env_excludingfor never-self-wrap,and
_openjd_defined_env()excluding host-inherited variables — all matchsession.rsline-for-line in intent. The docstrings even cite the Rustbehavior they mirror.
evaluate let" caveat on
run_wrap_actionis documented in both runners andmatches the Rust
run_wrap_action, rather than being a silent gap.no-wrap negative path, the "inner actions never ran" assertions (proving
replacement, not augmentation), and chained
letordering.expr_typesdiscipline in_seed_wrapped_action_symbolsshows correctunderstanding of the model's coercion contract.
Refuted / considered but not flagged
_active_wrap_envafter pop inexit_environment— the env is popped from_environments_enteredbefore_active_wrap_env()is called, so it correctlyreturns the outer wrap env. Matches the Rust comment at
session.rs:1125. ✅wrapped_step_name=step_name if step_name is not None else ""— defaultingto
""rather than raising is intentional (the kwarg is optional). Harmless.str(element)per element) — broadlymatches the Rust
to_display_string()per element; not flagged, but worth aspot-check that scalar coercion (e.g. bool/decimal in a list) renders
identically across the two (a
let/wrap list of bools would be the test).Suggested next steps (only if you want them)
evaluate_let_bindings)— these are the two with real behavioral weight.
action;
let-bound list consumed by an arg).openjd-specificationsonce Finding 1 is fixed (parity rule).