feat: Add WrappedAction.Cancelation.* wrap-hook template variables#313
feat: Add WrappedAction.Cancelation.* wrap-hook template variables#313leongdl wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # The mode is the schema selector, so partial interpolation | ||
| # like "TERMIN{{X}}" has no meaningful semantics: require a | ||
| # literal or a single whole-field expression. | ||
| if not (raw.startswith("{{") and raw.endswith("}}") and raw.count("{{") == 1): |
There was a problem hiding this comment.
The whole-field guard has a hole: it accepts any string that starts with {{, ends with }}, and contains exactly one {{. A value like "{{X}}Y}}" passes all three checks (startswith("{{") ✓, endswith("}}") ✓, count("{{") == 1 ✓) but is actually a partial interpolation — the expression {{X}} followed by the literal text Y}}. That is exactly the "partial interpolation has no meaningful semantics for a schema selector" case this validator is meant to reject (see test_partial_fmtstring_mode_rejected); such a value can never resolve to TERMINATE/NOTIFY_THEN_TERMINATE/null.
Since it starts with {{, _cancelation_discriminator still routes it to CancelationMethodDeferred, so it slips through parse-time validation and only fails (if at all) at runtime resolution. Consider also requiring raw.count("}}") == 1, or validating that the string is exactly one whole-field expression with no surrounding literal text.
There was a problem hiding this comment.
Fixed in ddaf000 — the guard now also requires raw.count("}}") == 1, so with both end anchors and exactly one opening and one closing brace pair, only {{ ... }} with no surrounding literal text is admitted; "{{X}}Y}}" (one open, two closes) is rejected at parse time with the whole-field error. The test_partial_fmtstring_mode_rejected test is now parametrized over both the leading-text (TERMIN{{X}}) and trailing-text ({{X}}Y}}) shapes.
The Rust guard in structure.rs had the identical hole and got the same fix (openjd-rs eec3f5e). Interesting asymmetry found while testing: Rust's expression parser already rejects the trailing }} on its own ("Missing opening braces"), so the stricter guard is defense-in-depth there — but Python's parser treats the trailing text as a literal, so this guard fix is load-bearing here.
06ab186 to
ddaf000
Compare
| cancelation: Optional[ | ||
| Union[CancelationMethodNotifyThenTerminate, CancelationMethodTerminate] | ||
| ] = Field(None, discriminator="mode") | ||
| cancelation: Optional[CancelationMethod] = None |
There was a problem hiding this comment.
The RFC 0008 wrapped-variable scope check does not inspect cancelation. _action_referenced_namespaces (lines 544-548) only walks command, args, and timeout, so WrappedAction.* / WrappedEnv.* / WrappedStep.* referenced inside cancelation.mode or cancelation.notifyPeriodInSeconds are invisible to _validate_wrapped_variable_scope.
Concretely, a template that puts cancelation: {mode: "{{WrappedAction.Cancelation.Mode}}"} on an ordinary onEnter/onExit action (not a wrap hook) will be accepted, contradicting CancelationMethodDeferred's own docstring: "at parse time the validator still checks ... that WrappedAction.* is only referenced inside wrap hooks." The new deferred cancelation is precisely what makes such references possible in this field, so the scope enforcement should be extended to cover the cancelation sub-model's FormatString fields.
|
this may be an issue: RFC 0008 says the special variables The PR added a new place format strings can live (cancelation.mode, cancelation.notifyPeriodInSeconds) but didn't add it to that list. Is this okay ? |
|
I saw all tests stop at validate/decode and do not advance to create_job? could we do so as well to make sure the flow is good e2e? |
be5e1cb to
e780df9
Compare
| return "terminate" | ||
| if isinstance(v, CancelationMethodDeferred): | ||
| return "deferred" | ||
| return None |
There was a problem hiding this comment.
Switching cancelation from pydantic's native Field(..., discriminator="mode") (a Literal-tagged union) to this callable Discriminator degrades the error message for an invalid literal mode.
Previously, a template with cancelation: {mode: "TERMINATEX"} (or any typo / wrong-case value) produced a clear, actionable error like Input should be 'NOTIFY_THEN_TERMINATE' or 'TERMINATE'. Now _cancelation_discriminator falls through all branches and returns None for such a value, so pydantic raises the generic union_tag_invalid error (Unable to extract tag using discriminator ...), which does not tell the author what the valid modes are.
Since a bad mode string is a common authoring mistake, consider having the discriminator route unrecognized non-format-string values to one of the fixed-shape classes (e.g. terminate) so the field validator emits the specific enum error, or otherwise surface the allowed mode names in the failure.
…C 0008 follow-up (openjd-specifications OpenJobDescription#148): expose the wrapped action's cancelation description to wrap hooks so a wrapper (e.g. a container runtime) can honor the same cancelation contract the runtime would have enforced in the unwrapped case: - WrappedAction.Cancelation.Mode (string?): TERMINATE, NOTIFY_THEN_TERMINATE, or null when the wrapped action declares no <Cancelation> — deliberately distinct from an explicit TERMINATE. - WrappedAction.Cancelation.NotifyPeriodInSeconds (int?): the effective grace period, applying the Template Schemas 5.3.2 defaults when the field is omitted; null when not applicable. - Format-string cancelation modes (FEATURE_BUNDLE_1) follow normal format-string semantics: any format string is statically valid, the resolved value is checked against the two mode names at run time, and a whole-field expression resolving to null drops the cancelation object. Resolution is deferred to run time, mirroring openjd-rs. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
e780df9 to
9c33ad7
Compare
| mode: FormatString | ||
| notifyPeriodInSeconds: Optional[Union[NotifyPeriodType, FormatString]] = None # noqa: N815 | ||
|
|
||
| _job_creation_metadata = JobCreationMetadata(resolve_fields={"notifyPeriodInSeconds"}) |
There was a problem hiding this comment.
CancelationMethodDeferred declares resolve_fields={"notifyPeriodInSeconds"}, so create_job will eagerly resolve this field against the job-creation symbol table. But the whole point of the deferred cancelation is forwarding, e.g.
cancelation:
mode: "{{WrappedAction.Cancelation.Mode}}"
notifyPeriodInSeconds: "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}"WrappedAction.* symbols are seeded only at session runtime (they are not in the create_job symbol table — _create_job.py populates it from job parameters only). Resolving {{WrappedAction.Cancelation.NotifyPeriodInSeconds}} there will hit FullNameNode.evaluate’s "... has no value" / the EXPR undefined-symbol path and raise a FormatStringError out of create_job.
Note Action.command/args avoid this precisely because they are not in resolve_fields (which is why test_create_job_with_wrap_env_succeeds passes with {{WrappedAction.Command}} in args). The forwarding round-trip tests (test_full_cancelation_forwarding_accepted, test_notify_period_only_forwarding_accepted) only call _decode, never create_job, so this path is uncovered. Please add a create_job test over a forwarded-cancelation wrap env; I suspect notifyPeriodInSeconds must be kept unresolved at job-creation time (like command/args) and resolved by the sessions runtime instead — mirroring the timeout-forwarding case the docstring already defers.
…Description#313 Port the test-only additions of PR OpenJobDescription#313: None-value rendering in format strings and nodes (RFC 0005 null rendering) and the TestCancelationRoundTripForwarding cases for wrap-hook cancelation forwarding (RFC 0008). These pin behavior whose production code OpenJobDescription#318 already carries, so OpenJobDescription#313 can close as superseded. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
|
Superseded by #318: all of this PR's production code is contained in #318's tip (verified hunk-by-hunk — the model changes are byte-identical, and the WrappedAction.Cancelation.* injection there uses the stricter per-hook scoping this PR's first-cut comment deferred). The test additions from this PR (deferred-cancelation round-trip forwarding, None-rendering) were ported to #318 in commit 6edc75a. Closing once #318 merges; the 4 wrap-cancelation conformance fixtures need sessions-runtime eager validation (openjd-sessions follow-up), not this PR. |
Bring the pure-Python (v0) 2023-09 model to EXPR (RFC 0007) and WRAP_ACTIONS (RFC 0008) parity with openjd-rs, via the PyO3-bound Rust expression engine. Base of the RFC 0008 stack (openjd-sessions-for-python #333, openjd-cli #230). Supersedes #313 (production code contained here; its tests ported). - Deferred cancelation mode (FEATURE_BUNDLE_1): a format-string `mode` becomes CancelationMethodDeferred (mirrors CancelationMode::DeferredMode); the shape decision moves to run time so wrap hooks can forward "{{WrappedAction.Cancelation.Mode}}". - Typed whole-field resolution (RFC 0006): FormatString.resolve_value + typed_resolve_fields metadata; range: "{{Param.Values}}" with a LIST[*] parameter instantiates to the native list, evaluated exactly once per field. §3.4's 1024-value range cap enforced at job creation for expression-driven ranges (parity with openjd-rs ranges.rs). - Per-hook WRAP_ACTIONS variables via _template_field_inject (WrappedAction.*/WrappedEnv.Name/WrappedStep.Name visible only in their hook) and template-scope validation of timeout/cancelation via _template_field_scopes. - EXPR runtime plumbing: step-level `let` at job creation (extends_symtab, preserved on Step.let), Job.Name seeding (§7.3.1), URI-form PATH values, exported parse-memoized openjd.model.evaluate_let_bindings. - Performance: EXPR engine symbol table + host-context profile cached on SymbolTable keyed on a mutation version — 8.6 -> 1.9 ms per 20-arg action (removes the per-expression Rust-boundary rebuild). - Crates pinned to the published releases containing DeferredMode: openjd-expr 0.2.1, openjd-model 0.4.0, openjd-sessions 0.4.0. Validation tightenings (all matching openjd-rs and the spec; not purely additive): onEnter required without WRAP_ACTIONS (§3.5); 1024-value range caps (§3.4); 64-char identifier names without FEATURE_BUNDLE_1 (§7.1) and decimals-requires-SPIN_BOX (§2.4). Tested: 5394 passed / 24 skipped / 3 xfailed (coverage 94.4%); mypy, black, ruff, clippy -D warnings clean; conformance 2023-09 at 1154/1158 (remaining 4 need sessions-runtime eager validation, tracked separately); new §3.4/§7.3.1 conformance fixtures in openjd-specifications#155 validated against both implementations. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
3 participants
What was the problem/requirement? (What/Why)
RFC 0008 follow-up (openjd-specifications #148): a wrap hook (e.g. a container runtime wrapper) needs to know the cancelation contract of the action it replaced, so it can honor the same
TERMINATE/NOTIFY_THEN_TERMINATEbehavior the runtime would have enforced in the unwrapped case. The model had no way to surface the wrapped action's<Cancelation>to wrap hooks.What was the solution? (How)
One squashed commit adding the
WrappedAction.Cancelation.*template variables to the 2023-09 model:WrappedAction.Cancelation.Mode(string?):"TERMINATE","NOTIFY_THEN_TERMINATE", ornullwhen the wrapped action declares no<Cancelation>— the null case is deliberately distinct from an explicitTERMINATEso wrap scripts can tell "author declared TERMINATE" apart from "author declared nothing".WrappedAction.Cancelation.NotifyPeriodInSeconds(int?): the effective grace period with the Template Schemas 5.3.2 defaults applied;nullwhen not applicable.nulldrops the cancelation object. Resolution is deferred to run time, mirroring the matching openjd-rs change.What is the impact of this change?
Wrap hooks can forward the wrapped action's cancelation contract (e.g. into
docker exectimeout flags). Purely additive to the model surface; gated on the FEATURE_BUNDLE_1 extension for the format-string mode.How was this change tested?
hatch run teston macOS), on a clean hatch env.wrap-cancelation-*) exercise the end-to-end behavior and pass once this change is available to the sessions/CLI stack.