diff --git a/docs/template_globals.md b/docs/template_globals.md index 47035daf..2fb7e6c6 100644 --- a/docs/template_globals.md +++ b/docs/template_globals.md @@ -54,7 +54,7 @@ validate`, exit 2). | `include_file` | 1 to 2 | The contents of a file, read when the step runs. | | `loop_item_at` | 2 | An item at a fixed index of a named loop: `loop_item_at('outer', 0)`. | | `loop_var` | 2 | A named loop's variable by name: `loop_var('outer', 'item')`. | -| `now` | 0 | The current time. Re-evaluated at every use, so two steps in one run disagree -- prefer `execution.timestamp` where a run needs one answer. | +| `now` | 0 | The current time, read afresh at every use, so two steps of one run disagree. Deprecated: use `execution.timestamp`, which is the same for every step of a run. | A name that is not on this list is not a global. `{{ nowx() }}` is a typo and is refused at compile time rather than becoming an undefined diff --git a/examples/data_processing_pipeline.yaml b/examples/data_processing_pipeline.yaml index a772abd8..59cb3823 100644 --- a/examples/data_processing_pipeline.yaml +++ b/examples/data_processing_pipeline.yaml @@ -237,7 +237,7 @@ steps: "errors": {{ validate_schema.errors | default([]) | to_json }}, "warnings": {{ validate_schema.warnings | default([]) | to_json }}, "rows_validated": {{ validate_schema.rows_validated | default(0) }}, - "timestamp": "{{ now() }}", + "timestamp": "{{ execution.timestamp }}", "input_file": "{{ input_file }}", "validation_mode": "{{ validate_schema.mode | default('strict') }}" } @@ -370,7 +370,7 @@ steps: --- - *Report generated on: {{ now() }}* + *Report generated on: {{ execution.timestamp }}* *Pipeline ID: {{ pipeline_id }}* dependencies: - clean_data diff --git a/examples/enhanced/data_processing_pipeline_enhanced.yaml b/examples/enhanced/data_processing_pipeline_enhanced.yaml index 1b919392..fcd7de46 100644 --- a/examples/enhanced/data_processing_pipeline_enhanced.yaml +++ b/examples/enhanced/data_processing_pipeline_enhanced.yaml @@ -235,7 +235,7 @@ steps: \ | to_json }},\n \"errors\": {{ validate_schema.errors | default([]) | to_json\ \ }},\n \"warnings\": {{ validate_schema.warnings | default([]) | to_json }},\n\ \ \"rows_validated\": {{ validate_schema.rows_validated | default(0) }},\n\ - \ \"timestamp\": \"{{ now() }}\",\n \"input_file\": \"{{ input_file }}\",\n\ + \ \"timestamp\": \"{{ execution.timestamp }}\",\n \"input_file\": \"{{ input_file }}\",\n\ \ \"validation_mode\": \"{{ validate_schema.mode | default('strict') }}\"\n\ }\n" dependencies: @@ -312,7 +312,7 @@ steps: \ %}\n*Statistical analysis pending*\n{% endif %}\n\n## Recommendations\n\n\ {% if quality_check.result.recommendations %}\n{% for rec in quality_check.result.recommendations\ \ %}\n- \U0001F4CC {{ rec }}\n{% endfor %}\n{% endif %}\n\n---\n\n*Report generated\ - \ on: {{ now() }}*\n*Pipeline ID: {{ pipeline_id }}*\n" + \ on: {{ execution.timestamp }}*\n*Pipeline ID: {{ pipeline_id }}*\n" dependencies: - clean_data - quality_check diff --git a/examples/enhanced/interactive_pipeline_enhanced.yaml b/examples/enhanced/interactive_pipeline_enhanced.yaml index d8787286..a443fa6c 100644 --- a/examples/enhanced/interactive_pipeline_enhanced.yaml +++ b/examples/enhanced/interactive_pipeline_enhanced.yaml @@ -318,7 +318,7 @@ steps: ## Timestamp - Generated at: {{ now() }} + Generated at: {{ execution.timestamp }} ' dependencies: diff --git a/examples/enhanced/validation_pipeline_enhanced.yaml b/examples/enhanced/validation_pipeline_enhanced.yaml index 0756e0e5..310e3beb 100644 --- a/examples/enhanced/validation_pipeline_enhanced.yaml +++ b/examples/enhanced/validation_pipeline_enhanced.yaml @@ -51,7 +51,7 @@ steps: parameters: path: examples/outputs/validation_pipeline/reports/validation_report.json content: "{\n \"validation_result\": {{ validate_data | to_json }},\n \"extracted_data\"\ - : {{ extract_info | to_json }},\n \"timestamp\": \"{{ now() }}\"\n}\n" + : {{ extract_info | to_json }},\n \"timestamp\": \"{{ execution.timestamp }}\"\n}\n" dependencies: - validate_data - extract_info diff --git a/examples/interactive_pipeline.yaml b/examples/interactive_pipeline.yaml index 7310d03d..e38c27a2 100644 --- a/examples/interactive_pipeline.yaml +++ b/examples/interactive_pipeline.yaml @@ -315,6 +315,6 @@ steps: - **Processing Useful**: {{ 'Yes' if collect_feedback.summary.boolean_summary.processing_useful else 'No' }} ## Timestamp - Generated at: {{ now() }} + Generated at: {{ execution.timestamp }} dependencies: - collect_feedback \ No newline at end of file diff --git a/examples/validation_pipeline.yaml b/examples/validation_pipeline.yaml index cc52bf36..48ddab36 100644 --- a/examples/validation_pipeline.yaml +++ b/examples/validation_pipeline.yaml @@ -59,7 +59,7 @@ steps: { "validation_result": {{ validate_data | to_json }}, "extracted_data": {{ extract_info | to_json }}, - "timestamp": "{{ now() }}" + "timestamp": "{{ execution.timestamp }}" } dependencies: - validate_data diff --git a/src/orchestrator/core/template_globals.py b/src/orchestrator/core/template_globals.py index 9a1eecf5..e99a5eee 100644 --- a/src/orchestrator/core/template_globals.py +++ b/src/orchestrator/core/template_globals.py @@ -50,6 +50,10 @@ class GlobalSpec: min_args: int max_args: Optional[int] # None means unbounded summary: str + #: What to write instead, if this global should no longer be used. The + #: call keeps working -- pipelines in the wild use it -- but validation + #: says so, once, with the replacement named. + deprecated_for: Optional[str] = None def accepts(self, positional: int) -> bool: if positional < self.min_args: @@ -71,8 +75,10 @@ def arity(self) -> str: GLOBAL_SPECS: Tuple[GlobalSpec, ...] = ( GlobalSpec( "now", 0, 0, - "The current time. Re-evaluated at every use, so two steps in one run " - "disagree -- prefer `execution.timestamp` where a run needs one answer.", + "The current time, read afresh at every use, so two steps of one run " + "disagree. Deprecated: use `execution.timestamp`, which is the same " + "for every step of a run.", + deprecated_for="execution.timestamp", ), GlobalSpec( "file_exists", 1, 2, @@ -113,6 +119,7 @@ def global_spec(name: str) -> Optional[GlobalSpec]: #: on these rather than on message text. NOT_CALLED = "global_not_called" WRONG_ARITY = "global_wrong_arity" +DEPRECATED = "global_deprecated" @dataclass(frozen=True) @@ -123,6 +130,8 @@ class GlobalMisuse: code: str message: str suggestion: str + #: "error" refuses the pipeline; "warning" lets it run and says so. + severity: str = "error" def find_global_misuse(ast: Any) -> List[GlobalMisuse]: @@ -202,5 +211,25 @@ def find_global_misuse(ast: Any) -> List[GlobalMisuse]: ), suggestion=f"{spec.name} expects {spec.arity} argument(s)", )) + continue + + # A correct call to something that should no longer be written. Not an + # error: pipelines in the wild use it and must keep running. + if spec.deprecated_for is not None: + key = (spec.name, DEPRECATED) + if key in seen: + continue + seen.add(key) + misuse.append(GlobalMisuse( + name=spec.name, + code=DEPRECATED, + message=( + f"'{spec.name}()' is deprecated: it is read afresh at every " + f"use, so two steps of one run disagree. Use " + f"'{spec.deprecated_for}', which is the same for every step." + ), + suggestion=spec.deprecated_for, + severity="warning", + )) return misuse diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index 39c8f6b3..cda65b1a 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/src/orchestrator/validation/template_validator.py @@ -335,13 +335,20 @@ def _validate_variables( # nothing fails. The AST is what tells a call apart from an # attribute access or a bare mention; the text does not. for misuse in find_global_misuse(ast): - errors.append(TemplateValidationError( + reported = TemplateValidationError( template=template, error_type=misuse.code, message=misuse.message, context_path=context_path, suggestions=[misuse.suggestion], - )) + severity=misuse.severity, + ) + # A deprecated global still works, so saying so must not stop + # the pipeline; a misused one cannot work, so it must. + if misuse.severity == "error": + errors.append(reported) + else: + warnings.append(reported) # Find all variable references var_names = meta.find_undeclared_variables(ast) diff --git a/tests/test_template_globals.py b/tests/test_template_globals.py index 54adabe5..9347ff7a 100644 --- a/tests/test_template_globals.py +++ b/tests/test_template_globals.py @@ -25,6 +25,7 @@ import pytest from orchestrator.core.template_globals import ( + DEPRECATED, GLOBAL_NAMES, GLOBAL_SPECS, NOT_CALLED, @@ -40,7 +41,13 @@ def _misuse(expression): - """The misuse codes a template's expression produces, as a set.""" + """Codes that would *refuse* the template. Warnings are not refusals.""" + ast = TemplateManager().env.parse(expression) + return {m.code for m in find_global_misuse(ast) if m.severity == "error"} + + +def _reported(expression): + """Every code, whatever its severity.""" ast = TemplateManager().env.parse(expression) return {m.code for m in find_global_misuse(ast)} @@ -159,6 +166,26 @@ def test_valid_use_is_left_alone(expression): assert _misuse(expression) == set(), f"{expression} is valid and was rejected" +def test_a_deprecated_global_is_reported_but_not_refused(): + """`now()` still runs -- pipelines in the wild use it -- and says so. + + Refusing it would break working pipelines to make a style point. Staying + silent would leave authors on a function that gives a different answer to + every step of one run. + """ + assert DEPRECATED in _reported("{{ now() }}") + assert _misuse("{{ now() }}") == set(), "a deprecated global must still validate" + + +def test_a_global_with_no_replacement_is_not_reported(): + assert _reported("{{ file_exists('a.txt') }}") == set() + + +def test_deprecation_is_not_reported_for_a_call_that_cannot_work(): + """One problem at a time: fix the call, then hear about the replacement.""" + assert _reported("{{ now(1, 2) }}") == {WRONG_ARITY} + + @pytest.mark.parametrize( "expression", [ @@ -284,3 +311,30 @@ def test_every_global_appears_in_the_documentation(): text = (ROOT / "docs" / "template_globals.md").read_text() for spec in GLOBAL_SPECS: assert f"`{spec.name}`" in text, f"{spec.name} is undocumented" + + +def test_no_example_still_calls_a_deprecated_global(): + """The catalogue was migrated; this keeps it migrated. + + `{{ now() }}` gave each step of a run a different answer, so the six + examples using it stamped their reports inconsistently. They now use + `execution.timestamp`. + """ + import re + + deprecated = {s.name for s in GLOBAL_SPECS if s.deprecated_for} + if not deprecated: + # An empty alternation matches every call, so guard rather than report + # the whole catalogue as offending. + return + pattern = re.compile(r"\{\{[^}]*\b(" + "|".join(sorted(deprecated)) + r")\s*\(") + + offenders = sorted( + f"{path.relative_to(ROOT)}: {pattern.search(path.read_text()).group(0)}" + for path in (ROOT / "examples").rglob("*.yaml") + if pattern.search(path.read_text()) + ) + assert not offenders, ( + "these examples call a deprecated global; the replacement is in " + f"docs/template_globals.md: {offenders}" + )