From 60820cbb1d9cb925e04ffc4e42bc62978ac99426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:07:09 +0000 Subject: [PATCH 1/9] fix: name numeric fluents whose :init value is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Phase-1 warning was decided per declared function name, so a problem that initialised (fuel t1) got no warning at all for fuel(t2) — and the message claimed the missing value "defaults to 0", which PDDL does not do. Reading such an instance raises UPUsageError from the simulator, so the warning was both silent where it mattered and wrong where it fired. Enumerate the ground instances of each numeric function and name the ones :init leaves out, with a PDDL-shaped repair hint. Cap the enumeration at 250 instances per function (a two-argument function over a big domain would explode) and summarise long missing lists. Still a warning: a plan that never reads the instance remains valid. Adds tests/test_numeric_init.py (7 tests). --- CHANGELOG.md | 20 +++++ pyval/syntax_checker.py | 103 ++++++++++++++++++++--- tests/test_numeric_init.py | 166 +++++++++++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 13 deletions(-) create mode 100644 tests/test_numeric_init.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a43da7c..3328d61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,26 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S ## [Unreleased] +### Fixed +- The Phase-1 "numeric function has no initial value" warning was decided per + declared function *name*, so initialising `(fuel t1)` silenced it for + `(fuel t2)` entirely, and the message claimed the missing value "defaults to + 0". Neither is true: PDDL defines no default for numeric fluents and the + simulator raises `UPUsageError: The state ... does not have a value for the + value fuel(t2)` when a precondition or effect reads an unassigned instance + (validation aborted with a traceback). `_check_numeric_initialization` in + `pyval/syntax_checker.py` now enumerates the ground instances of every + numeric function and names the ones `:init` leaves out, with a valid PDDL + repair hint (`Add (= (fuel t2) ) to :init.`). The warning stays a + warning: a plan that never reads the instance is still valid. Functions with + more than 250 ground instances are not enumerated and get a summary message + instead. + +### Added +- `tests/test_numeric_init.py`: partial initialisation, fully initialised + functions, zero-arity functions, summarised long lists, the enumeration bound, + and a guard against the "defaults to 0" claim coming back. + ## [0.1.6] — 2026-11-09 ### Fixed diff --git a/pyval/syntax_checker.py b/pyval/syntax_checker.py index 53dfbb4..6d0636b 100644 --- a/pyval/syntax_checker.py +++ b/pyval/syntax_checker.py @@ -2,9 +2,18 @@ from __future__ import annotations +import itertools + from unified_planning.io import PDDLReader from unified_planning.model import Problem +# Bound on how many ground instances of a numeric function we are willing to +# enumerate when cross-checking :init. Object counts multiply, so a two-argument +# function over a large domain explodes past any useful limit. +_MAX_GROUND_INSTANCES = 250 +# Bound on how many missing instances one warning lists before summarising. +_MAX_LISTED_MISSING = 6 + def check_syntax( domain_path: str, problem_path: str | None = None @@ -62,22 +71,90 @@ def _format_parse_error( def _check_numeric_initialization(problem: Problem) -> list[str]: - """Warn about numeric fluents with no explicit initial value (default 0).""" - warnings = [] - declared_numeric_fluents = set() - initialized_fluents = set() + """Warn about numeric fluent instances that :init never assigns a value to. + + PDDL defines no default for numeric fluents, and neither does the + simulator: a ``state.get_value`` on an unassigned instance raises + ``UPUsageError`` ("The state ... does not have a value for the value + fuel(t2)"), which used to abort validation with a traceback. The check is + per ground instance, not per declared name, because initialising + ``(fuel t1)`` says nothing about ``(fuel t2)``. + """ + warnings: list[str] = [] for fluent in problem.fluents: - if fluent.type.is_int_type() or fluent.type.is_real_type(): - declared_numeric_fluents.add(fluent.name) + if not (fluent.type.is_int_type() or fluent.type.is_real_type()): + continue + + instances = _ground_instances(problem, fluent) + if instances is None: + warnings.append( + f"Numeric function '{fluent.name}' has more than " + f"{_MAX_GROUND_INSTANCES} ground instances, so it was not " + f"cross-checked against :init. No default value is assumed: " + f"any instance that :init leaves out has no value in the state." + ) + continue + + missing = [expr for expr in instances if expr not in problem.initial_values] + if missing: + warnings.append(_missing_init_warning(fluent.name, missing, len(instances))) + + return warnings - for fluent_expr in problem.initial_values: - if fluent_expr.is_fluent_exp(): - initialized_fluents.add(fluent_expr.fluent().name) - for name in declared_numeric_fluents - initialized_fluents: - warnings.append( - f"Numeric function '{name}' has no initial value assigned — defaults to 0" +def _ground_instances(problem: Problem, fluent) -> list | None: + """Enumerate the ground instances of `fluent`, or None if there are too many.""" + domains = [] + total = 1 + for parameter in fluent.signature: + objects = list(problem.objects(parameter.type)) + if not objects: + # A parameter type with no objects has no ground instances at all. + return [] + domains.append(objects) + total *= len(objects) + if total > _MAX_GROUND_INSTANCES: + return None + + expression_manager = problem.environment.expression_manager + return [ + expression_manager.FluentExp(fluent, tuple(combination)) + for combination in itertools.product(*domains) + ] + + +def _missing_init_warning(name: str, missing: list, total: int) -> str: + """Build a repair-oriented message naming the unassigned instances.""" + rendered = [str(expr) for expr in missing[:_MAX_LISTED_MISSING]] + listed = ", ".join(rendered) + if len(missing) > _MAX_LISTED_MISSING: + listed += f", ... ({len(missing) - _MAX_LISTED_MISSING} more)" + + if total == 1: + subject = f"Numeric function '{name}' has no initial value" + example = f"(= {rendered[0]} )" + else: + subject = ( + f"Numeric function '{name}' has no initial value for " + f"{len(missing)} of its {total} ground instances" ) + example = f"(= {_as_pddl_call(rendered[0])} )" + + return ( + f"{subject}: {listed}. PDDL assigns no default, so the simulator has no " + f"value for them and any precondition or effect that reads one cannot be " + f"evaluated. Add {example} to :init." + ) + + +def _as_pddl_call(rendered: str) -> str: + """Turn UPF's ``fuel(t2)`` spelling into PDDL's ``(fuel t2)``.""" + head, sep, rest = rendered.partition("(") + if not sep or not rest.endswith(")"): + return f"({rendered})" + arguments = " ".join( + argument.strip() for argument in rest[:-1].split(",") if argument.strip() + ) + return f"({head.strip()} {arguments})" if arguments else f"({head.strip()})" - return warnings diff --git a/tests/test_numeric_init.py b/tests/test_numeric_init.py new file mode 100644 index 0000000..d3f1236 --- /dev/null +++ b/tests/test_numeric_init.py @@ -0,0 +1,166 @@ +"""Phase-1 warnings for numeric fluent instances that :init never assigns. + +PDDL defines no default for a numeric fluent, and the simulator has no value +for an instance that :init leaves out: reading one raises ``UPUsageError``. +These tests pin the per-instance coverage of that warning (the check used to be +per declared name, so initialising ``(fuel t1)`` silenced the warning for +``(fuel t2)`` entirely) and the absence of the old, false "defaults to 0" claim. +""" + +from pyval.syntax_checker import check_syntax +from tests.conftest import write_pddl_files + + +NUMERIC_DOMAIN = """\ +(define (domain numinit) + (:requirements :strips :typing :numeric-fluents) + (:types truck loc) + (:predicates (at ?t - truck ?l - loc) (link ?a - loc ?b - loc)) + (:functions (fuel ?t - truck) (distance ?a - loc ?b - loc)) + (:action drive + :parameters (?t - truck ?from - loc ?to - loc) + :precondition (and (at ?t ?from) (link ?from ?to) + (>= (fuel ?t) (distance ?from ?to))) + :effect (and (not (at ?t ?from)) (at ?t ?to) + (decrease (fuel ?t) (distance ?from ?to)))) +) +""" + + +def _problem(init: str) -> str: + return f"""\ +(define (problem numinit-p1) + (:domain numinit) + (:objects t1 t2 - truck l1 l2 - loc) + (:init (at t1 l1) (at t2 l1) (link l1 l2) (= (distance l1 l2) 10) + {init} + ) + (:goal (at t1 l2)) +) +""" + + +def _warnings(tmp_path, init: str) -> list[str]: + paths = write_pddl_files(tmp_path, NUMERIC_DOMAIN, _problem(init)) + result, problem = check_syntax(paths["domain"], paths["problem"]) + assert result["status"] == "PASS" + assert problem is not None + return result["warnings"] + + +def test_partial_initialisation_names_the_missing_instance(tmp_path): + """`(= (fuel t1) 50)` says nothing about `fuel(t2)` — name it explicitly.""" + warnings = _warnings(tmp_path, "(= (fuel t1) 50)") + + fuel = [w for w in warnings if "'fuel'" in w] + assert len(fuel) == 1, warnings + message = fuel[0] + assert "fuel(t2)" in message + assert "fuel(t1)" not in message # the initialised instance is not reported + assert "1 of its 2 ground instances" in message + # Repair hint must be valid PDDL: `(= (fuel t2) )`, not `(= fuel(t2) ...)` + assert "Add (= (fuel t2) ) to :init." in message + + +def test_warning_does_not_claim_a_default_of_zero(tmp_path): + """The old text said the value "defaults to 0" — it does not; it is absent.""" + for warning in _warnings(tmp_path, "(= (fuel t1) 50)"): + assert "defaults to 0" not in warning + assert "assigns no default" in warning + + +def test_fully_initialised_function_is_not_reported(tmp_path): + warnings = _warnings( + tmp_path, "(= (fuel t1) 50) (= (fuel t2) 20)" + ) + assert not [w for w in warnings if "'fuel'" in w] + + +def test_every_instance_missing_is_reported_once(tmp_path): + warnings = _warnings(tmp_path, "(= (distance l1 l2) 10) (= (fuel t1) 1) (= (fuel t2) 1)") + distance = [w for w in warnings if "'distance'" in w] + assert len(distance) == 1 + assert "3 of its 4 ground instances" in distance[0] + assert "distance(l1, l2)" not in distance[0] + # Comma-separated UPF rendering must not survive into the PDDL repair hint + assert "Add (= (distance l1 l1) ) to :init." in distance[0] + + +def test_zero_arity_function_without_init_is_reported(tmp_path): + domain = """\ +(define (domain nularity) + (:requirements :strips :numeric-fluents) + (:predicates (done)) + (:functions (total-cost)) + (:action noop :parameters () :precondition (done) :effect (increase (total-cost) 1)) +) +""" + problem = """\ +(define (problem nularity-p1) + (:domain nularity) + (:init (done)) + (:goal (done)) +) +""" + paths = write_pddl_files(tmp_path, domain, problem) + result, _ = check_syntax(paths["domain"], paths["problem"]) + message = [w for w in result["warnings"] if "'total-cost'" in w] + assert len(message) == 1 + assert "has no initial value: total-cost" in message[0] + assert "Add (= total-cost ) to :init." in message[0] + + +def test_long_missing_list_is_summarised(tmp_path): + """A function over many objects lists a few instances, then the remainder.""" + domain = """\ +(define (domain many) + (:requirements :strips :typing :numeric-fluents) + (:types thing) + (:predicates (p ?x - thing)) + (:functions (cost ?x - thing)) + (:action noop :parameters (?x - thing) :precondition (p ?x) + :effect (decrease (cost ?x) 1)) +) +""" + objects = " ".join(f"o{i}" for i in range(10)) + problem = f"""\ +(define (problem many-p1) + (:domain many) + (:objects {objects} - thing) + (:init (p o0)) + (:goal (p o0)) +) +""" + paths = write_pddl_files(tmp_path, domain, problem) + result, _ = check_syntax(paths["domain"], paths["problem"]) + message = [w for w in result["warnings"] if "'cost'" in w][0] + assert "10 of its 10 ground instances" in message + assert "4 more)" in message + + +def test_huge_ground_space_is_not_enumerated(tmp_path): + """Two 17-object parameters exceed the enumeration bound; warn instead.""" + domain = """\ +(define (domain huge) + (:requirements :strips :typing :numeric-fluents) + (:types thing) + (:predicates (p ?x - thing ?y - thing)) + (:functions (cost ?x - thing ?y - thing)) + (:action noop :parameters (?x - thing ?y - thing) :precondition (p ?x ?y) + :effect (decrease (cost ?x ?y) 1)) +) +""" + objects = " ".join(f"o{i}" for i in range(17)) + problem = f"""\ +(define (problem huge-p1) + (:domain huge) + (:objects {objects} - thing) + (:init (p o0 o0)) + (:goal (p o0 o0)) +) +""" + paths = write_pddl_files(tmp_path, domain, problem) + result, _ = check_syntax(paths["domain"], paths["problem"]) + message = [w for w in result["warnings"] if "'cost'" in w][0] + assert "more than 250 ground instances" in message + assert "No default value is assumed" in message From 2dfc537876f14e092a6f232bdfc867d3af700d8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:08:51 +0000 Subject: [PATCH 2/9] fix: report plan steps the simulator cannot evaluate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plan that reads a numeric fluent instance :init never assigned (the common case: (fuel t1) is set, (fuel t2) is not) made UPF raise UPUsageError from is_applicable, and validate() let it escape — the CLI died with a traceback and the library raised at the caller. The same happens for effects that write such an instance (sim.apply). Catch the evaluation failure, stop the plan at that step and record why on StepResult.evaluation_error: the fluent name plus the PDDL repair. The report says "could not be validated" instead of "Plan is INVALID" — an unevaluable step proves nothing about the plan as a whole — and the JSON step carries the message. Adds tests/test_evaluation_errors.py (6 tests) and a shared pyval/pddl_format.py for the fuel(t2) -> (fuel t2) rendering. --- CHANGELOG.md | 12 +++ pyval/models.py | 4 + pyval/pddl_format.py | 24 ++++++ pyval/plan_simulator.py | 97 ++++++++++++++++----- pyval/report_formatter.py | 23 ++++- pyval/syntax_checker.py | 16 +--- tests/test_evaluation_errors.py | 147 ++++++++++++++++++++++++++++++++ 7 files changed, 286 insertions(+), 37 deletions(-) create mode 100644 pyval/pddl_format.py create mode 100644 tests/test_evaluation_errors.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3328d61..1388532 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S ## [Unreleased] ### Fixed +- A plan step the simulator cannot evaluate no longer aborts validation with a + traceback. Reading (or writing) a numeric fluent instance that `:init` never + assigns makes UPF raise `UPUsageError` from `is_applicable`/`apply`, and + `PDDLValidator.validate` propagated it out to the CLI. `simulate` in + `pyval/plan_simulator.py` now catches the evaluation failure, stops the plan + there and records it on the step (`StepResult.evaluation_error`), naming the + fluent and the PDDL repair (`Add (= (fuel t2) ) to :init.`). The report + prints `✗ EVALUATION ERROR` and states that the plan could not be validated + rather than claiming it is invalid — the plan is not proven wrong, it simply + cannot be checked past that step. The JSON step carries `evaluation_error`. - The Phase-1 "numeric function has no initial value" warning was decided per declared function *name*, so initialising `(fuel t1)` silenced it for `(fuel t2)` entirely, and the message claimed the missing value "defaults to @@ -25,6 +35,8 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S - `tests/test_numeric_init.py`: partial initialisation, fully initialised functions, zero-arity functions, summarised long lists, the enumeration bound, and a guard against the "defaults to 0" claim coming back. +- `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the + report wording, the JSON field, and a CLI run that must not print a traceback. ## [0.1.6] — 2026-11-09 diff --git a/pyval/models.py b/pyval/models.py index a767f8f..0a7c21a 100644 --- a/pyval/models.py +++ b/pyval/models.py @@ -44,6 +44,10 @@ class StepResult: boolean_changes: dict[str, bool] = field(default_factory=dict) numeric_changes: dict[str, NumericChange] = field(default_factory=dict) unsatisfied: list[PreconditionFailure] = field(default_factory=list) + # Set when the simulator could not evaluate the step at all (e.g. a numeric + # fluent with no value in the state). The plan is not proven invalid — it + # simply could not be checked past this point. + evaluation_error: str | None = None @dataclass diff --git a/pyval/pddl_format.py b/pyval/pddl_format.py new file mode 100644 index 0000000..1763133 --- /dev/null +++ b/pyval/pddl_format.py @@ -0,0 +1,24 @@ +"""Small formatting helpers shared by the diagnostics paths. + +UPF renders fluent instances the way the API spells them — ``fuel(t2)``, +``distance(l1, l2)`` — while the messages we emit are read by people editing +PDDL source files, where the same term is ``(fuel t2)``. Converting once here +keeps every repair hint valid PDDL. +""" + +from __future__ import annotations + + +def as_pddl_call(rendered: str) -> str: + """Turn a UPF name rendering into the PDDL source spelling. + + ``fuel(t2)`` -> ``(fuel t2)``; ``total-cost`` -> ``(total-cost)``. + """ + head, separator, rest = rendered.partition("(") + if not separator or not rest.endswith(")"): + return f"({rendered.strip()})" + arguments = " ".join( + argument.strip() for argument in rest[:-1].split(",") if argument.strip() + ) + name = head.strip() + return f"({name} {arguments})" if arguments else f"({name})" diff --git a/pyval/plan_simulator.py b/pyval/plan_simulator.py index bfc8654..42facfd 100644 --- a/pyval/plan_simulator.py +++ b/pyval/plan_simulator.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re import warnings from unified_planning.engines import UPSequentialSimulator @@ -11,6 +12,13 @@ from pyval.diagnostics import check_goals, decompose_preconditions from pyval.models import GoalResult, NumericChange, StateSnapshot, StepResult from pyval.numeric_tracker import NumericTracker +from pyval.pddl_format import as_pddl_call + +# UPF raises `UPUsageError: The state {...} does not have a value for the value +# fuel(t2)` when a condition or effect reads a numeric fluent instance that +# :init never assigned. The whole state is in that message; only the fluent +# matters for the report. +_MISSING_VALUE_RE = re.compile(r"does not have a value for the value (?P.+?)\s*$") def simulate( @@ -37,34 +45,40 @@ def simulate( for idx, ai in enumerate(action_instances, start=1): action_display = _format_action(ai, original_names) - if sim.is_applicable(state, ai): - new_state = sim.apply(state, ai) - if new_state is None: - # apply() returned None — treat as inapplicable (per upf-gotchas.md) - failures = decompose_preconditions( - ai.action, ai.actual_parameters, state, problem - ) - steps.append(StepResult( - index=idx, - action=action_display, - status="FAILED", - unsatisfied=failures, - )) - break - - boolean_changes, numeric_changes = _compute_changes( - problem, state, new_state + # The simulator evaluates conditions and effects against the state, and + # raises when the state has no value for something the plan reads (a + # numeric fluent :init never assigned is the usual cause). That is a + # property of the input, not a crash: report the step and stop. + try: + applicable = sim.is_applicable(state, ai) + except Exception as exc: + steps.append(_evaluation_failed_step(idx, action_display, exc)) + tracker.record(idx, action_display, state) + break + + if not applicable: + failures = decompose_preconditions( + ai.action, ai.actual_parameters, state, problem ) steps.append(StepResult( index=idx, action=action_display, - status="OK", - boolean_changes=boolean_changes, - numeric_changes=numeric_changes, + status="FAILED", + unsatisfied=failures, )) - state = new_state + # Record snapshot at failure point too + tracker.record(idx, action_display, state) + break + + try: + new_state = sim.apply(state, ai) + except Exception as exc: + steps.append(_evaluation_failed_step(idx, action_display, exc)) tracker.record(idx, action_display, state) - else: + break + + if new_state is None: + # apply() returned None — treat as inapplicable (per upf-gotchas.md) failures = decompose_preconditions( ai.action, ai.actual_parameters, state, problem ) @@ -74,10 +88,22 @@ def simulate( status="FAILED", unsatisfied=failures, )) - # Record snapshot at failure point too tracker.record(idx, action_display, state) break + boolean_changes, numeric_changes = _compute_changes( + problem, state, new_state + ) + steps.append(StepResult( + index=idx, + action=action_display, + status="OK", + boolean_changes=boolean_changes, + numeric_changes=numeric_changes, + )) + state = new_state + tracker.record(idx, action_display, state) + goal_results = check_goals(problem, state) metric = _evaluate_metric(problem, state) @@ -90,6 +116,31 @@ def simulate( ) +def _evaluation_failed_step(idx: int, action_display: str, exc: Exception) -> StepResult: + """Build a FAILED step that says why the step could not be evaluated.""" + return StepResult( + index=idx, + action=action_display, + status="FAILED", + evaluation_error=_describe_evaluation_error(exc), + ) + + +def _describe_evaluation_error(exc: Exception) -> str: + """Turn a simulator exception into a repair-oriented one-line explanation.""" + message = str(exc).strip() + missing = _MISSING_VALUE_RE.search(message) + if missing: + value = missing.group("value").strip() + return ( + f"No value in the current state for {value}: this problem's :init never " + f"assigns it, and PDDL defines no default. The step cannot be evaluated " + f"until it is initialised. Add (= {as_pddl_call(value)} ) to :init." + ) + first_line = message.splitlines()[0] if message else "no message" + return f"{type(exc).__name__}: {first_line}"[:400] + + def _evaluate_metric(problem: Problem, final_state) -> dict | None: """Evaluate plan quality metric against the final state.""" from unified_planning.model.metrics import ( diff --git a/pyval/report_formatter.py b/pyval/report_formatter.py index ce9c68d..947a67b 100644 --- a/pyval/report_formatter.py +++ b/pyval/report_formatter.py @@ -56,6 +56,9 @@ def format_plain_text(result: ValidationResult, verbose: bool = False) -> str: changes = _format_changes(step) if changes: lines.append(f" Changed: {changes}") + elif step.evaluation_error: + lines.append(f"Step {step.index}: {step.action} \u2717 EVALUATION ERROR") + lines.append(f" {step.evaluation_error}") else: lines.append(f"Step {step.index}: {step.action} \u2717 PRECONDITION FAILURE") for failure in step.unsatisfied: @@ -74,7 +77,15 @@ def format_plain_text(result: ValidationResult, verbose: bool = False) -> str: # that was not checked. if "execution" in result.phases: lines.append("=== Goal Check ===") - if result.is_valid: + unevaluable = _unevaluable_step(result) + if unevaluable is not None: + total = result.phases.get("execution", {}).get("total_steps", "?") + lines.append( + f"Plan could not be validated: step {unevaluable.index} of {total} " + f"({unevaluable.action}) could not be evaluated, so no verdict is " + f"given for the plan as a whole." + ) + elif result.is_valid: lines.append("All goals satisfied. Plan is VALID.") else: if result.failed_step is not None: @@ -161,6 +172,8 @@ def format_json(result: ValidationResult) -> dict: }, } else: + if step.evaluation_error: + step_dict["evaluation_error"] = step.evaluation_error step_dict["unsatisfied_preconditions"] = [ { "expression": f.expression, @@ -196,6 +209,14 @@ def format_json(result: ValidationResult) -> dict: return output +def _unevaluable_step(result: ValidationResult): + """Return the first step the simulator could not evaluate, if any.""" + for step in result.steps: + if step.evaluation_error: + return step + return None + + def _format_changes(step) -> str: """Format boolean and numeric changes for a step as a compact string.""" parts: list[str] = [] diff --git a/pyval/syntax_checker.py b/pyval/syntax_checker.py index 6d0636b..1522b2e 100644 --- a/pyval/syntax_checker.py +++ b/pyval/syntax_checker.py @@ -7,6 +7,8 @@ from unified_planning.io import PDDLReader from unified_planning.model import Problem +from pyval.pddl_format import as_pddl_call + # Bound on how many ground instances of a numeric function we are willing to # enumerate when cross-checking :init. Object counts multiply, so a two-argument # function over a large domain explodes past any useful limit. @@ -139,22 +141,10 @@ def _missing_init_warning(name: str, missing: list, total: int) -> str: f"Numeric function '{name}' has no initial value for " f"{len(missing)} of its {total} ground instances" ) - example = f"(= {_as_pddl_call(rendered[0])} )" + example = f"(= {as_pddl_call(rendered[0])} )" return ( f"{subject}: {listed}. PDDL assigns no default, so the simulator has no " f"value for them and any precondition or effect that reads one cannot be " f"evaluated. Add {example} to :init." ) - - -def _as_pddl_call(rendered: str) -> str: - """Turn UPF's ``fuel(t2)`` spelling into PDDL's ``(fuel t2)``.""" - head, sep, rest = rendered.partition("(") - if not sep or not rest.endswith(")"): - return f"({rendered})" - arguments = " ".join( - argument.strip() for argument in rest[:-1].split(",") if argument.strip() - ) - return f"({head.strip()} {arguments})" if arguments else f"({head.strip()})" - diff --git a/tests/test_evaluation_errors.py b/tests/test_evaluation_errors.py new file mode 100644 index 0000000..516721e --- /dev/null +++ b/tests/test_evaluation_errors.py @@ -0,0 +1,147 @@ +"""A plan step the simulator cannot evaluate must be reported, not raised. + +Both crash paths are covered: a precondition reading a numeric fluent instance +that ``:init`` never assigns (``sim.is_applicable`` raises ``UPUsageError``) and +an effect that writes one (``sim.apply`` raises). Before this, ``validate()`` +propagated the UPF exception, so the CLI died with a traceback instead of +producing a report. +""" + +from pyval import PDDLValidator +from pyval.report_formatter import format_json, format_plain_text +from tests.conftest import write_pddl_files + + +PARTIAL_DOMAIN = """\ +(define (domain partial) + (:requirements :strips :typing :numeric-fluents) + (:types truck loc) + (:predicates (at ?t - truck ?l - loc) (link ?a - loc ?b - loc)) + (:functions (fuel ?t - truck) (distance ?a - loc ?b - loc)) + (:action drive + :parameters (?t - truck ?from - loc ?to - loc) + :precondition (and (at ?t ?from) (link ?from ?to) + (>= (fuel ?t) (distance ?from ?to))) + :effect (and (not (at ?t ?from)) (at ?t ?to) + (decrease (fuel ?t) (distance ?from ?to)))) +) +""" + + +def _partial_problem(init: str) -> str: + return f"""\ +(define (problem partial-p1) + (:domain partial) + (:objects t1 t2 - truck l1 l2 - loc) + (:init (at t1 l1) (at t2 l1) (link l1 l2) (= (distance l1 l2) 10) + {init} + ) + (:goal (at t1 l2)) +) +""" + + +def test_uninitialised_precondition_fluent_is_reported(tmp_path): + """The second truck's fuel has no value: precondition evaluation raises.""" + paths = write_pddl_files( + tmp_path, + PARTIAL_DOMAIN, + _partial_problem("(= (fuel t1) 50)"), + "(drive t1 l1 l2)\n(drive t2 l1 l2)\n", + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert not result.is_valid + assert result.failed_step == 2 + failed = result.steps[1] + assert failed.status == "FAILED" + assert failed.unsatisfied == [] + assert failed.evaluation_error is not None + assert "fuel(t2)" in failed.evaluation_error + assert "Add (= (fuel t2) ) to :init." in failed.evaluation_error + + +def test_uninitialised_effect_fluent_is_reported(tmp_path): + """An effect that increases a fluent with no value raises in sim.apply.""" + domain = """\ +(define (domain apperr) + (:requirements :strips :typing :numeric-fluents) + (:types thing) + (:predicates (done ?x - thing)) + (:functions (cost ?x - thing)) + (:action bump + :parameters (?x - thing) + :precondition (and) + :effect (and (done ?x) (increase (cost ?x) 1))) +) +""" + problem = """\ +(define (problem apperr-p1) + (:domain apperr) + (:objects a b - thing) + (:init (= (cost a) 0)) + (:goal (done b)) +) +""" + paths = write_pddl_files(tmp_path, domain, problem, "(bump a)\n(bump b)\n") + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert not result.is_valid + assert result.failed_step == 2 + assert "cost(b)" in result.steps[1].evaluation_error + assert result.steps[0].status == "OK" + + +def test_report_does_not_claim_the_plan_is_invalid(tmp_path): + """An unevaluable step means "unknown", not "invalid".""" + paths = write_pddl_files( + tmp_path, PARTIAL_DOMAIN, _partial_problem("(= (fuel t1) 50)"), "(drive t2 l1 l2)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + text = format_plain_text(result) + + assert "EVALUATION ERROR" in text + assert "No value in the current state for fuel(t2)" in text + assert "Plan could not be validated" in text + assert "Plan is INVALID" not in text + + +def test_json_carries_the_evaluation_error(tmp_path): + paths = write_pddl_files( + tmp_path, PARTIAL_DOMAIN, _partial_problem("(= (fuel t1) 50)"), "(drive t2 l1 l2)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + payload = format_json(result) + step = payload["phases"]["execution"]["steps"][0] + + assert step["status"] == "FAILED" + assert step["unsatisfied_preconditions"] == [] + assert "fuel(t2)" in step["evaluation_error"] + + +def test_cli_survives_the_unevaluable_step(tmp_path): + """The CLI printed a traceback and exited non-zero before; now it reports.""" + import subprocess + import sys + + paths = write_pddl_files( + tmp_path, PARTIAL_DOMAIN, _partial_problem("(= (fuel t1) 50)"), "(drive t2 l1 l2)\n" + ) + completed = subprocess.run( + [sys.executable, "-m", "pyval.cli", paths["domain"], paths["problem"], paths["plan"]], + capture_output=True, + text=True, + ) + assert completed.returncode == 1 + assert "Traceback" not in completed.stderr + assert "EVALUATION ERROR" in completed.stdout + + +def test_message_names_only_the_missing_fluent(tmp_path): + """The UPF message embeds the whole state; only the fluent name survives.""" + paths = write_pddl_files( + tmp_path, PARTIAL_DOMAIN, _partial_problem("(= (fuel t1) 50)"), "(drive t2 l1 l2)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + message = result.steps[0].evaluation_error + assert "{" not in message and "at(t1, l1)" not in message From ada81c5c5866309a89e741d05cdc1c2df7918151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:10:49 +0000 Subject: [PATCH 3/9] fix: explain every step reported as a precondition failure Two shapes of precondition produced "PRECONDITION FAILURE" with nothing under it. The OR branch compared the number of collected sub-failures with the number of disjuncts, so (or (and p q s) r) yielded four failures for two disjuncts and reported none of them. And the walker had no branch for quantifiers, implication or object equality, so (forall (?y) (p ?y)) fell through silently. Evaluate each disjunct with UPF's StateEvaluator instead of counting failures, and add a final branch that evaluates and reports anything the walker does not special-case. Route EQUALS through the boolean path when its operands are not numeric: an object equality is a boolean condition, not a numeric deficit, and it used to read "Could not evaluate numeric comparison". Render expressions with " = " so the text is PDDL-shaped. Adds tests/test_precondition_diagnostics.py (7 tests), including an invariant that a FAILED step always carries diagnostics. --- CHANGELOG.md | 16 ++ pyval/diagnostics.py | 229 +++++++++++++++++++------ tests/test_precondition_diagnostics.py | 192 +++++++++++++++++++++ 3 files changed, 381 insertions(+), 56 deletions(-) create mode 100644 tests/test_precondition_diagnostics.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1388532..74f35a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S ## [Unreleased] ### Fixed +- A step reported `✗ PRECONDITION FAILURE` with an empty explanation in two + cases, both in `decompose_preconditions` (`pyval/diagnostics.py`). First, the + `or` branch counted collected sub-failures instead of evaluating the + disjuncts, so `(or (and (p) (q) (s)) (r))` produced four "failures" for two + disjuncts and the mismatch suppressed the report entirely. Second, any node + the walker did not handle (quantifiers, implication, object equality) fell + through silently. Disjunctions are now decided by evaluating each disjunct + with UPF's `StateEvaluator`, and an unhandled node with a final branch that + reports when it does not hold. `:equality` comparisons between objects are no + longer routed through the numeric path (they used to answer "Could not + evaluate numeric comparison" with `type: "numeric"`); expressions render as + `(a = c)` rather than UPF's `(a == c)`. - A plan step the simulator cannot evaluate no longer aborts validation with a traceback. Reading (or writing) a numeric fluent instance that `:init` never assigns makes UPF raise `UPUsageError` from `is_applicable`/`apply`, and @@ -37,6 +49,10 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S and a guard against the "defaults to 0" claim coming back. - `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the report wording, the JSON field, and a CLI run that must not print a traceback. +- `tests/test_precondition_diagnostics.py`: disjunctions with multi-conjunct + disjuncts, quantified preconditions, object equality both ways, a conjunction + reporting every failing conjunct, and an invariant that no `FAILED` step is + left without diagnostics. ## [0.1.6] — 2026-11-09 diff --git a/pyval/diagnostics.py b/pyval/diagnostics.py index 9977fe6..ff18603 100644 --- a/pyval/diagnostics.py +++ b/pyval/diagnostics.py @@ -17,9 +17,10 @@ def decompose_preconditions( """ failures: list[PreconditionFailure] = [] param_map = dict(zip(action.parameters, parameters)) + evaluator = StateEvaluator(problem) for prec in action.preconditions: - _evaluate_expr(prec, state, param_map, failures) + _evaluate_expr(prec, state, param_map, failures, evaluator) return failures @@ -38,72 +39,181 @@ def _evaluate_expr( state, param_map: dict, failures: list[PreconditionFailure], + evaluator, ) -> None: """Recursively evaluate a precondition expression, collecting failures.""" node_type = expr.node_type if node_type == OperatorKind.AND: for arg in expr.args: - _evaluate_expr(arg, state, param_map, failures) + _evaluate_expr(arg, state, param_map, failures, evaluator) return if node_type == OperatorKind.OR: - # Check if any disjunct is satisfied - sub_failures: list[PreconditionFailure] = [] - for arg in expr.args: - _evaluate_expr(arg, state, param_map, sub_failures) - if len(sub_failures) == len(expr.args): - # None satisfied — report the OR as failed - failures.append(PreconditionFailure( - expression=_expr_to_pddl(expr, param_map), - type="boolean", - current_values={}, - explanation="None of the disjuncts are satisfied", - )) + _evaluate_disjunction(expr, state, param_map, failures, evaluator) return if node_type == OperatorKind.NOT: inner = expr.args[0] - grounded = _substitute(inner, param_map) - try: - val = state.get_value(grounded) - if val.is_true(): - failures.append(PreconditionFailure( - expression=_expr_to_pddl(expr, param_map), - type="boolean", - current_values={_expr_to_pddl(inner, param_map): True}, - explanation=f"{_expr_to_pddl(inner, param_map)} is true but should be false", - )) - except Exception: - pass + verdict = _holds(expr, state, param_map, evaluator) + if verdict is False: + failures.append(PreconditionFailure( + expression=_expr_to_pddl(expr, param_map), + type="boolean", + current_values={_expr_to_pddl(inner, param_map): True}, + explanation=f"{_expr_to_pddl(inner, param_map)} is true but should be false", + )) + elif verdict is None: + failures.append(_unevaluable_failure(expr, param_map)) return - # Comparison operators: LE, LT, EQUALS (UPF normalizes >= to <= with swapped args) + # Comparison operators: LE, LT, EQUALS (UPF normalizes >= to <= with swapped args). + # EQUALS is also how `:equality` compares *objects*, which carries no numeric + # deficit and must not be reported as an unreadable numeric comparison. if node_type in (OperatorKind.LE, OperatorKind.LT, OperatorKind.EQUALS): - _evaluate_comparison(expr, state, param_map, failures) + if _is_numeric(expr.args[0]) and _is_numeric(expr.args[1]): + _evaluate_comparison(expr, state, param_map, failures) + else: + _evaluate_boolean( + expr, + state, + param_map, + failures, + evaluator, + unsatisfied="is not satisfied in the current state", + ) return # Fluent expression (boolean) if expr.is_fluent_exp(): - grounded = _substitute(expr, param_map) - try: - val = state.get_value(grounded) - if val.is_false(): - failures.append(PreconditionFailure( - expression=_expr_to_pddl(expr, param_map), - type="boolean", - current_values={_expr_to_pddl(expr, param_map): False}, - explanation=f"{_expr_to_pddl(expr, param_map)} is not true in the current state", - )) - except Exception: - failures.append(PreconditionFailure( - expression=_expr_to_pddl(expr, param_map), - type="boolean", - current_values={}, - explanation=f"Could not evaluate {_expr_to_pddl(expr, param_map)}", - )) + _evaluate_boolean( + expr, + state, + param_map, + failures, + evaluator, + unsatisfied="is not true in the current state", + ) return + # Everything else — quantifiers, implication, equivalence, object equality — + # used to fall through this function silently, so a failing quantified + # precondition produced a FAILED step with an empty explanation. + _evaluate_boolean( + expr, + state, + param_map, + failures, + evaluator, + unsatisfied="is not satisfied in the current state", + ) + + +def _evaluate_disjunction( + expr: FNode, + state, + param_map: dict, + failures: list[PreconditionFailure], + evaluator, +) -> None: + """Report a disjunction only when no disjunct holds. + + Counting sub-failures is not the same as evaluating the disjuncts: an + ``(and ...)`` disjunct contributes one failure per failing conjunct, so + ``(or (and (p) (q) (s)) (r))`` produced more "failures" than there were + disjuncts and the OR reported nothing at all — the failing step then showed + no unsatisfied precondition. + """ + verdicts = [_holds(arg, state, param_map, evaluator) for arg in expr.args] + if any(verdict is True for verdict in verdicts): + return + + rendered = _expr_to_pddl(expr, param_map) + disjuncts = [_expr_to_pddl(arg, param_map) for arg in expr.args] + + if all(verdict is False for verdict in verdicts): + explanation = ( + f"None of the {len(disjuncts)} disjuncts holds: " + "; ".join(disjuncts) + ) + else: + unevaluable = [ + disjunct + for disjunct, verdict in zip(disjuncts, verdicts) + if verdict is None + ] + explanation = ( + "No disjunct could be shown to hold; the simulator raised on: " + + "; ".join(unevaluable) + ) + + failures.append(PreconditionFailure( + expression=rendered, + type="boolean", + current_values={}, + explanation=explanation, + )) + + +def _evaluate_boolean( + expr: FNode, + state, + param_map: dict, + failures: list[PreconditionFailure], + evaluator, + unsatisfied: str, +) -> None: + """Report a boolean condition that does not hold, using UPF's evaluator.""" + verdict = _holds(expr, state, param_map, evaluator) + if verdict is True: + return + if verdict is None: + failures.append(_unevaluable_failure(expr, param_map)) + return + + rendered = _expr_to_pddl(expr, param_map) + failures.append(PreconditionFailure( + expression=rendered, + type="boolean", + current_values={rendered: False}, + explanation=f"{rendered} {unsatisfied}", + )) + + +def _unevaluable_failure(expr: FNode, param_map: dict) -> PreconditionFailure: + """Build a failure for a condition the simulator could not evaluate.""" + rendered = _expr_to_pddl(expr, param_map) + return PreconditionFailure( + expression=rendered, + type="boolean", + current_values={}, + explanation=f"Could not evaluate {rendered}", + ) + + +def _holds(expr: FNode, state, param_map: dict, evaluator) -> bool | None: + """Return whether `expr` holds in `state`, or None when it cannot be evaluated. + + Uses UPF's StateEvaluator so constants, arithmetic, quantifiers and object + equality evaluate the same way the simulator evaluates them. + """ + grounded = _substitute(expr, param_map) + try: + value = evaluator.evaluate(grounded, state) + except Exception: + return None + if value.is_bool_constant(): + return value.is_true() + return None + + +def _is_numeric(expr: FNode) -> bool: + """Whether an expression denotes a number (as opposed to an object or bool).""" + try: + expr_type = expr.type + except Exception: + return False + return expr_type.is_int_type() or expr_type.is_real_type() + def _evaluate_comparison( expr: FNode, @@ -124,7 +234,7 @@ def _evaluate_comparison( expression=_expr_to_pddl(expr, param_map), type="numeric", current_values={}, - explanation=f"Could not evaluate numeric comparison", + explanation="Could not evaluate numeric comparison", )) return @@ -194,16 +304,12 @@ def _evaluate_goal(expr: FNode, state, results: list[GoalResult], evaluator) -> return if expr.node_type in (OperatorKind.LE, OperatorKind.LT, OperatorKind.EQUALS): - satisfied, current_values = _evaluate_comparison_goal(expr, state, evaluator) + if _is_numeric(expr.args[0]) and _is_numeric(expr.args[1]): + satisfied, current_values = _evaluate_comparison_goal(expr, state, evaluator) + else: + satisfied, current_values = _evaluate_boolean_goal(expr, state, evaluator) else: - satisfied = False - try: - val = evaluator.evaluate(expr, state) - if val.is_bool_constant(): - satisfied = val.is_true() - except Exception: - pass - current_values = {str(expr): satisfied} + satisfied, current_values = _evaluate_boolean_goal(expr, state, evaluator) results.append(GoalResult( expression=str(expr), @@ -212,6 +318,17 @@ def _evaluate_goal(expr: FNode, state, results: list[GoalResult], evaluator) -> )) +def _evaluate_boolean_goal(expr: FNode, state, evaluator) -> tuple[bool, dict]: + """Evaluate a non-numeric goal (fluent, NOT, quantified, object equality).""" + try: + value = evaluator.evaluate(expr, state) + except Exception: + return (False, {}) + if value.is_bool_constant(): + return (value.is_true(), {str(expr): value.is_true()}) + return (False, {}) + + def _evaluate_comparison_goal(expr: FNode, state, evaluator) -> tuple[bool, dict]: try: left_val = float(evaluator.evaluate(expr.args[0], state).constant_value()) @@ -238,4 +355,4 @@ def _expr_to_pddl(expr: FNode, param_map: dict | None = None) -> str: """Convert an FNode expression to a PDDL-like string.""" if param_map: expr = _substitute(expr, param_map) - return str(expr) + return str(expr).replace(" == ", " = ") diff --git a/tests/test_precondition_diagnostics.py b/tests/test_precondition_diagnostics.py new file mode 100644 index 0000000..c951856 --- /dev/null +++ b/tests/test_precondition_diagnostics.py @@ -0,0 +1,192 @@ +"""Precondition diagnostics must explain every step they call FAILED. + +`decompose_preconditions` walked AND/OR/NOT/comparisons and silently ignored +everything else, and its OR branch counted sub-failures instead of evaluating +the disjuncts. Both produced a FAILED step with an empty +`unsatisfied_preconditions` list, which is the one thing a repair-oriented +validator must never do. +""" + +from pyval import PDDLValidator +from tests.conftest import ( + BLOCKSWORLD_DOMAIN, + BLOCKSWORLD_PROBLEM, + LOGISTICS_FUEL_DOMAIN, + LOGISTICS_FUEL_PROBLEM, + write_pddl_files, +) + + +DISJUNCT_DOMAIN = """\ +(define (domain disjuncts) + (:requirements :strips :typing) + (:types thing) + (:predicates (p ?x - thing) (q ?x - thing) (s ?x - thing) (r ?x - thing) + (done ?x - thing)) + (:action go + :parameters (?x - thing) + :precondition (or (and (p ?x) (q ?x) (s ?x)) (r ?x)) + :effect (done ?x)) +) +""" + + +def _disjunct_problem(init: str) -> str: + return f"""\ +(define (problem disjuncts-p1) + (:domain disjuncts) + (:objects a - thing) + (:init {init}) + (:goal (done a)) +) +""" + + +def test_or_with_multi_conjunct_disjunct_still_reports(tmp_path): + """`(and p q s)` fails 3 times and `r` fails once — the OR must still speak.""" + paths = write_pddl_files( + tmp_path, DISJUNCT_DOMAIN, _disjunct_problem(""), "(go a)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.status == "INVALID" + failures = result.steps[0].unsatisfied + assert len(failures) == 1, failures + assert "or" in failures[0].expression + assert "None of the 2 disjuncts holds" in failures[0].explanation + assert "p(a)" in failures[0].explanation + assert "r(a)" in failures[0].explanation + + +def test_or_with_satisfied_disjunct_does_not_report(tmp_path): + paths = write_pddl_files( + tmp_path, DISJUNCT_DOMAIN, _disjunct_problem("(r a)"), "(go a)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + assert result.is_valid, result.report() + + +QUANTIFIED_DOMAIN = """\ +(define (domain quantified) + (:requirements :strips :typing :quantified-preconditions) + (:types thing) + (:predicates (p ?x - thing) (done ?x - thing)) + (:action mark + :parameters (?x - thing) + :precondition (forall (?y - thing) (p ?y)) + :effect (done ?x)) +) +""" + + +def test_universal_precondition_failure_is_reported(tmp_path): + problem = """\ +(define (problem quantified-p1) + (:domain quantified) + (:objects a b c - thing) + (:init (p a) (p b)) + (:goal (done a)) +) +""" + paths = write_pddl_files(tmp_path, QUANTIFIED_DOMAIN, problem, "(mark a)\n") + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.status == "INVALID" + failures = result.steps[0].unsatisfied + assert len(failures) == 1 + assert failures[0].type == "boolean" + assert "Forall" in failures[0].expression + assert "is not satisfied in the current state" in failures[0].explanation + + +EQUALITY_DOMAIN = """\ +(define (domain equality) + (:requirements :strips :typing :equality) + (:types thing) + (:predicates (at ?x - thing ?loc - thing)) + (:action move + :parameters (?x - thing ?from - thing ?to - thing) + :precondition (and (at ?x ?from) (= ?from ?to)) + :effect (and (not (at ?x ?from)) (at ?x ?to))) +) +""" + +EQUALITY_PROBLEM = """\ +(define (problem equality-p1) + (:domain equality) + (:objects a b c - thing) + (:init (at a a)) + (:goal (at a c)) +) +""" + + +def test_object_equality_failure_is_not_reported_as_numeric(tmp_path): + """`:equality` compares objects; the old path said "numeric comparison".""" + paths = write_pddl_files( + tmp_path, EQUALITY_DOMAIN, EQUALITY_PROBLEM, "(move a a c)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.status == "INVALID" + failure = result.steps[0].unsatisfied[0] + assert failure.type == "boolean" + assert "numeric comparison" not in failure.explanation + assert failure.deficit is None + assert "= c" in failure.expression # rendered as PDDL equality, not "==" + + +def test_object_equality_that_holds_does_not_report(tmp_path): + paths = write_pddl_files( + tmp_path, EQUALITY_DOMAIN, EQUALITY_PROBLEM, "(move a a a)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.steps[0].status == "OK" + assert result.steps[0].unsatisfied == [] + # The plan is still invalid: it never reaches (at a c). That is a goal + # failure, not a precondition failure. + assert result.status == "INVALID" + assert result.failed_step is None + assert [g.expression for g in result.unsatisfied_goals] == ["at(a, c)"] + + +def test_conjunction_reports_every_failing_conjunct(tmp_path): + """(on b2 b1) and (clear b2) both fail for `unstack b2 b1`.""" + paths = write_pddl_files( + tmp_path, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM, "(unstack b2 b1)\n" + ) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + expressions = sorted(failure.expression for failure in result.steps[0].unsatisfied) + assert expressions == ["clear(b2)", "on(b2, b1)"], expressions + + +def test_failed_steps_always_explain_themselves(tmp_path): + """Invariant: a FAILED step without an evaluation error has diagnostics.""" + numeric_paths = write_pddl_files( + tmp_path, + LOGISTICS_FUEL_DOMAIN, + LOGISTICS_FUEL_PROBLEM, + "(drive truck1 loc1 loc2)\n" + "(drive truck1 loc2 loc3)\n" + "(drive truck1 loc3 loc2)\n" + "(drive truck1 loc2 loc3)\n" + "(drive truck1 loc3 loc2)\n" + "(drive truck1 loc2 loc3)\n" + "(drive truck1 loc3 loc2)\n" + "(drive truck1 loc2 loc3)\n", + ) + bw_dir = tmp_path / "bw" + bw_dir.mkdir() + classical_paths = write_pddl_files( + bw_dir, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM, "(unstack b1 b2)\n(stack b1 b3)\n" + ) + + for paths in (numeric_paths, classical_paths): + result = PDDLValidator().validate( + paths["domain"], paths["problem"], paths["plan"] + ) + failed = [step for step in result.steps if step.status == "FAILED"] + assert failed, result.report() + for step in failed: + assert step.unsatisfied or step.evaluation_error, step From 98858f50aea7d89320a0f538b08a22c73c6bcd90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:12:32 +0000 Subject: [PATCH 4/9] fix: evaluate arithmetic and action-cost metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (:metric minimize (+ (* 2 (fuel)) 5)) produced no metric at all: the evaluation used state.get_value, which UPF asserts on for arithmetic sub-expressions, and the exception was swallowed into a bare None — indistinguishable from a problem that declares no metric. 0.1.4 fixed the same bug for goals by moving to StateEvaluator; the metric path kept it. It now reports 17.0 on the probe that used to return nothing. Domains declaring :action-costs carry MinimizeActionCosts, which is not an expression over the final state, so (:metric minimize (total-cost)) was dropped too. Sum each executed action's declared cost, evaluated in the state the action was applied in. When a metric still cannot be evaluated, say so (n/a plus the reason) instead of dropping it. --- CHANGELOG.md | 13 ++++ pyval/plan_simulator.py | 85 +++++++++++++++++++++---- pyval/report_formatter.py | 6 +- tests/test_metrics.py | 131 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 tests/test_metrics.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 74f35a2..ed34efb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S ## [Unreleased] ### Fixed +- Plan quality metrics were silently dropped in two cases. `_evaluate_metric` + (`pyval/plan_simulator.py`) used `state.get_value`, which UPF asserts on for + arithmetic sub-expressions, so `(:metric minimize (+ (* 2 (fuel)) 5))` came + back as `metric: null` — exactly the bug the 0.1.4 goal fix addressed, left + behind on the metric path; it now evaluates through `StateEvaluator` (same + problem, `= 17.0`). And domains declaring `:action-costs` get a + `MinimizeActionCosts` metric, which is not an expression on the final state, so + `(:metric minimize (total-cost))` was dropped entirely; the plan cost is now + the sum of each executed action's declared cost, evaluated in the state it was + applied in. A metric that still cannot be evaluated is reported as `n/a` with + the reason instead of vanishing. - A step reported `✗ PRECONDITION FAILURE` with an empty explanation in two cases, both in `decompose_preconditions` (`pyval/diagnostics.py`). First, the `or` branch counted collected sub-failures instead of evaluating the @@ -49,6 +60,8 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S and a guard against the "defaults to 0" claim coming back. - `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the report wording, the JSON field, and a CLI run that must not print a traceback. +- `tests/test_metrics.py`: plain and arithmetic metrics, `:action-costs`, the + report/JSON rendering, and a metric that cannot be evaluated. - `tests/test_precondition_diagnostics.py`: disjunctions with multi-conjunct disjuncts, quantified preconditions, object equality both ways, a conjunction reporting every failing conjunct, and an invariant that no `FAILED` step is diff --git a/pyval/plan_simulator.py b/pyval/plan_simulator.py index 42facfd..c1ac88b 100644 --- a/pyval/plan_simulator.py +++ b/pyval/plan_simulator.py @@ -41,6 +41,9 @@ def simulate( tracker.record(0, None, state) steps: list[StepResult] = [] + # (action instance, state it was applied in) — the pre-state is what an + # action-cost expression must be evaluated against. + executed: list[tuple[ActionInstance, object]] = [] for idx, ai in enumerate(action_instances, start=1): action_display = _format_action(ai, original_names) @@ -101,11 +104,12 @@ def simulate( boolean_changes=boolean_changes, numeric_changes=numeric_changes, )) + executed.append((ai, state)) state = new_state tracker.record(idx, action_display, state) goal_results = check_goals(problem, state) - metric = _evaluate_metric(problem, state) + metric = _evaluate_metric(problem, state, executed) return ( steps, @@ -141,28 +145,85 @@ def _describe_evaluation_error(exc: Exception) -> str: return f"{type(exc).__name__}: {first_line}"[:400] -def _evaluate_metric(problem: Problem, final_state) -> dict | None: - """Evaluate plan quality metric against the final state.""" +def _evaluate_metric( + problem: Problem, final_state, executed: list[tuple[ActionInstance, object]] | None = None +) -> dict | None: + """Evaluate plan quality metric against the final state. + + Goes through UPF's ``StateEvaluator``: ``state.get_value`` asserts on + arithmetic sub-expressions, so a metric like + ``(:metric minimize (+ (* 2 (fuel)) 5))`` used to come back as ``None`` — + indistinguishable from a problem that declares no metric at all. + + ``:action-costs`` domains carry ``MinimizeActionCosts``, which has no + single expression: the plan's cost is the sum of the per-action cost + expressions for the actions that actually executed, each evaluated in the + state it was applied in. + """ from unified_planning.model.metrics import ( + MinimizeActionCosts, MinimizeExpressionOnFinalState, MaximizeExpressionOnFinalState, ) + from unified_planning.model.walkers import StateEvaluator for metric in problem.quality_metrics: + if isinstance(metric, MinimizeActionCosts): + return _evaluate_action_costs(problem, metric, executed or []) if isinstance(metric, (MinimizeExpressionOnFinalState, MaximizeExpressionOnFinalState)): + kind = "minimize" if isinstance(metric, MinimizeExpressionOnFinalState) else "maximize" + entry: dict = { + "type": kind, + "expression": str(metric.expression), + "value": None, + } try: - value = final_state.get_value(metric.expression) - kind = "minimize" if isinstance(metric, MinimizeExpressionOnFinalState) else "maximize" - return { - "type": kind, - "expression": str(metric.expression), - "value": float(value.constant_value()), - } - except Exception: - return None + value = StateEvaluator(problem).evaluate(metric.expression, final_state) + entry["value"] = float(value.constant_value()) + except Exception as exc: + entry["unavailable"] = _describe_metric_error(exc) + return entry return None +def _evaluate_action_costs( + problem: Problem, metric, executed: list[tuple[ActionInstance, object]] +) -> dict: + """Sum the declared cost of every action that executed.""" + from unified_planning.model.walkers import StateEvaluator + + entry: dict = {"type": "minimize", "expression": "action-costs", "value": None} + evaluator = StateEvaluator(problem) + total = 0.0 + + for instance, state_before in executed: + cost_expression = metric.get_action_cost(instance.action) + if cost_expression is None: + entry["unavailable"] = ( + f"No cost is declared for action '{instance.action.name}', so the " + f"plan cost is incomplete" + ) + return entry + substitutions = dict(zip(instance.action.parameters, instance.actual_parameters)) + try: + grounded = ( + cost_expression.substitute(substitutions) if substitutions else cost_expression + ) + total += float(evaluator.evaluate(grounded, state_before).constant_value()) + except Exception as exc: + entry["unavailable"] = _describe_metric_error(exc) + return entry + + entry["value"] = total + return entry + + +def _describe_metric_error(exc: Exception) -> str: + """One-line reason a metric expression could not be evaluated.""" + message = str(exc).strip().splitlines()[0] if str(exc).strip() else "no message" + return f"{type(exc).__name__}: {message}"[:300] + + def _compute_changes( problem: Problem, old_state, new_state ) -> tuple[dict[str, bool], dict[str, NumericChange]]: diff --git a/pyval/report_formatter.py b/pyval/report_formatter.py index 947a67b..012b123 100644 --- a/pyval/report_formatter.py +++ b/pyval/report_formatter.py @@ -114,10 +114,14 @@ def format_plain_text(result: ValidationResult, verbose: bool = False) -> str: # Metric if result.metric: + value = result.metric.get("value") + rendered = "n/a" if value is None else value lines.append( f"Plan metric: {result.metric['type']} {result.metric['expression']} " - f"= {result.metric['value']}" + f"= {rendered}" ) + if value is None and result.metric.get("unavailable"): + lines.append(f" Metric could not be evaluated: {result.metric['unavailable']}") # Numeric summary if result.numeric_trajectory: diff --git a/tests/test_metrics.py b/tests/test_metrics.py new file mode 100644 index 0000000..dd0e92e --- /dev/null +++ b/tests/test_metrics.py @@ -0,0 +1,131 @@ +"""Plan quality metrics are evaluated on the final state. + +`_evaluate_metric` used `state.get_value`, which asserts on arithmetic +sub-expressions, so any metric like `(:metric minimize (+ (* 2 (fuel)) 5))` +silently produced no metric at all — the same root cause as the 0.1.4 goal fix, +which switched goal evaluation to `StateEvaluator` but left the metric path +behind. +""" + +from pyval import PDDLValidator +from pyval.report_formatter import format_json, format_plain_text +from tests.conftest import write_pddl_files + + +PROBLEM = """\ +(define (problem metric-p1) + (:domain metric) + (:objects l1 l2 - loc) + (:init (at l1) (link l1 l2) (= (fuel l1) 10) (= (distance l1 l2) 4)) + (:goal (at l2)) + (:metric minimize {metric}) +) +""" + +DOMAIN = """\ +(define (domain metric) + (:requirements :strips :typing :numeric-fluents) + (:types loc) + (:predicates (at ?l - loc) (link ?a - loc ?b - loc)) + (:functions (fuel ?l - loc) (distance ?a - loc ?b - loc)) + (:action drive + :parameters (?from - loc ?to - loc) + :precondition (and (at ?from) (link ?from ?to) + (>= (fuel ?from) (distance ?from ?to))) + :effect (and (not (at ?from)) (at ?to) + (decrease (fuel ?from) (distance ?from ?to)))) +) +""" + +PLAN = "(drive l1 l2)\n" + + +def _run(tmp_path, metric: str, domain: str = DOMAIN, problem: str | None = None): + paths = write_pddl_files( + tmp_path, + domain, + (problem or PROBLEM).replace("{metric}", metric), + PLAN, + ) + return PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + +def test_plain_fluent_metric(tmp_path): + result = _run(tmp_path, "(fuel l1)") + assert result.metric == { + "type": "minimize", + "expression": "fuel(l1)", + "value": 6.0, + } + + +def test_arithmetic_metric_is_evaluated(tmp_path): + """`(+ (* 2 (fuel l1)) 5)` = 2 * 6 + 5 = 17.""" + result = _run(tmp_path, "(+ (* 2 (fuel l1)) 5)") + + assert result.metric is not None + assert result.metric["value"] == 17.0 + assert "unavailable" not in result.metric + + +def test_arithmetic_metric_survives_the_report(tmp_path): + result = _run(tmp_path, "(+ (* 2 (fuel l1)) 5)") + text = format_plain_text(result) + + assert "Plan metric: minimize" in text + assert "= 17.0" in text + assert "could not be evaluated" not in text + assert format_json(result)["metric"]["value"] == 17.0 + + +def test_action_costs_metric_still_works(tmp_path): + """`:action-costs` produces MinimizeActionCosts, a final-state metric.""" + domain = """\ +(define (domain costs) + (:requirements :strips :typing :action-costs) + (:types loc) + (:predicates (at ?l - loc) (link ?a - loc ?b - loc)) + (:functions (total-cost)) + (:action drive + :parameters (?from - loc ?to - loc) + :precondition (and (at ?from) (link ?from ?to)) + :effect (and (not (at ?from)) (at ?to) (increase (total-cost) 3))) +) +""" + problem = """\ +(define (problem costs-p1) + (:domain costs) + (:objects l1 l2 - loc) + (:init (at l1) (link l1 l2) (= (total-cost) 0)) + (:goal (at l2)) + (:metric minimize (total-cost)) +) +""" + paths = write_pddl_files(tmp_path, domain, problem, PLAN) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.metric["value"] == 3.0 + assert result.metric["expression"] == "action-costs" + + +def test_unevaluable_metric_is_reported_not_dropped(tmp_path): + """A metric over a fluent with no value says so instead of vanishing.""" + problem = """\ +(define (problem metric-p1) + (:domain metric) + (:objects l1 l2 - loc) + (:init (at l1) (link l1 l2) (= (fuel l1) 10) (= (distance l1 l2) 4)) + (:goal (at l2)) + (:metric minimize (distance l2 l1)) +) +""" + paths = write_pddl_files(tmp_path, DOMAIN, problem, PLAN) + result = PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + + assert result.metric is not None + assert result.metric["value"] is None + assert "UPUsageError" in result.metric["unavailable"] + + text = format_plain_text(result) + assert "= n/a" in text + assert "Metric could not be evaluated" in text From 4702b9ba87dedfbe62c09b355f49da6b4172d1a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:13:44 +0000 Subject: [PATCH 5/9] feat: expose is_valid and the goal check in JSON output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit phases.goals held only the unsatisfied goals and was null both when no plan had been executed and when every goal passed, so a consumer could not tell "checked and green" from "not checked" — the ambiguity 0.1.5 removed from the plain-text report, still present in the JSON. is_valid was missing from the payload entirely. Add both: is_valid at the top level, and phases.goal_check with every goal and its verdict whenever the execution phase ran. ValidationResult keeps the full goal list alongside the unsatisfied subset, and phases.goals is untouched so existing consumers keep working. Adds tests/test_json_output.py (4 tests). --- CHANGELOG.md | 8 +++++ pyval/models.py | 4 +++ pyval/report_formatter.py | 14 ++++++++ pyval/validator.py | 1 + tests/test_json_output.py | 71 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+) create mode 100644 tests/test_json_output.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ed34efb..f725066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,11 +55,19 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S instead. ### Added +- The JSON output carries `is_valid` and, when a plan was executed, + `phases.goal_check` with every goal and its verdict. `phases.goals` only ever + held the *unsatisfied* goals, so `null` meant both "no plan was executed" and + "every goal passed" — the same ambiguity 0.1.5 removed from the plain-text + report, left in the machine-readable one. `phases.goals` is unchanged for + existing consumers; `tests/test_json_output.py` covers both keys. - `tests/test_numeric_init.py`: partial initialisation, fully initialised functions, zero-arity functions, summarised long lists, the enumeration bound, and a guard against the "defaults to 0" claim coming back. - `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the report wording, the JSON field, and a CLI run that must not print a traceback. +- `tests/test_json_output.py`: `is_valid` on every status, every goal in + `goal_check`, and no `goal_check` when no plan ran. - `tests/test_metrics.py`: plain and arithmetic metrics, `:action-costs`, the report/JSON rendering, and a metric that cannot be evaluated. - `tests/test_precondition_diagnostics.py`: disjunctions with multi-conjunct diff --git a/pyval/models.py b/pyval/models.py index 0a7c21a..bcb2d95 100644 --- a/pyval/models.py +++ b/pyval/models.py @@ -60,6 +60,10 @@ class ValidationResult: numeric_trajectory: dict[str, list] = field(default_factory=dict) failed_step: int | None = None unsatisfied_goals: list[GoalResult] = field(default_factory=list) + # Every goal, satisfied or not. `unsatisfied_goals` stays the convenient + # subset; this is what lets the JSON output show that a goal check happened + # at all (an empty `unsatisfied_goals` is also what a syntax-only run gives). + goal_results: list[GoalResult] = field(default_factory=list) warnings: list[str] = field(default_factory=list) metric: dict | None = None # {"type": "minimize"/"maximize", "expression": str, "value": float} diff --git a/pyval/report_formatter.py b/pyval/report_formatter.py index 012b123..f8975a3 100644 --- a/pyval/report_formatter.py +++ b/pyval/report_formatter.py @@ -140,6 +140,7 @@ def format_json(result: ValidationResult) -> dict: """Format validation result as structured JSON dict.""" output: dict = { "status": result.status, + "is_valid": result.is_valid, "phases": {}, } @@ -206,6 +207,19 @@ def format_json(result: ValidationResult) -> dict: else: output["phases"]["goals"] = None + # Goal check detail. `phases["goals"]` only ever held the *unsatisfied* + # goals, so `null` meant either "no plan was executed" or "every goal passed". + # Keyed on the execution phase instead, and carrying every goal evaluated. + if "execution" in result.phases: + output["phases"]["goal_check"] = [ + { + "expression": g.expression, + "satisfied": g.satisfied, + "current_values": g.current_values, + } + for g in result.goal_results + ] + # Metric if result.metric: output["metric"] = result.metric diff --git a/pyval/validator.py b/pyval/validator.py index 7060561..6f8e2d4 100644 --- a/pyval/validator.py +++ b/pyval/validator.py @@ -100,6 +100,7 @@ def validate( numeric_trajectory=numeric_traj, failed_step=failed_step, unsatisfied_goals=unsatisfied_goals, + goal_results=goal_results, warnings=all_warnings, metric=metric, ) diff --git a/tests/test_json_output.py b/tests/test_json_output.py new file mode 100644 index 0000000..71f133d --- /dev/null +++ b/tests/test_json_output.py @@ -0,0 +1,71 @@ +"""The JSON output must say whether a goal check happened, and what it found. + +`phases["goals"]` only ever carried the *unsatisfied* goals and was `null` both +for a syntax-only run and for a VALID plan, so a consumer could not tell "goals +were checked and passed" from "no plan was executed" — and `is_valid` was not in +the payload at all. +""" + +import json + +from pyval import PDDLValidator +from pyval.report_formatter import format_json +from tests.conftest import ( + BLOCKSWORLD_DOMAIN, + BLOCKSWORLD_PROBLEM, + BLOCKSWORLD_VALID_PLAN, + write_pddl_files, +) + + +def test_is_valid_is_present_for_every_status(tmp_path): + paths = write_pddl_files( + tmp_path, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM, BLOCKSWORLD_VALID_PLAN + ) + valid = format_json( + PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + ) + assert valid["is_valid"] is True + + syntax_only = format_json(PDDLValidator().validate_syntax(paths["domain"])) + assert syntax_only["is_valid"] is True + assert syntax_only["status"] == "VALID" + + +def test_valid_plan_exposes_every_goal(tmp_path): + paths = write_pddl_files( + tmp_path, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM, BLOCKSWORLD_VALID_PLAN + ) + payload = format_json( + PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + ) + + goal_check = payload["phases"]["goal_check"] + assert [g["expression"] for g in goal_check] == ["on(b3, b2)", "on(b2, b1)", "ontable(b1)"] + assert all(g["satisfied"] for g in goal_check) + # Backwards compatibility: the unsatisfied-only key still reads as before. + assert payload["phases"]["goals"] is None + + +def test_unevaluable_goals_are_still_listed(tmp_path): + plan = "(unstack b1 b2)\n(put-down b1)\n" + paths = write_pddl_files(tmp_path, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM, plan) + payload = format_json( + PDDLValidator().validate(paths["domain"], paths["problem"], paths["plan"]) + ) + + goal_check = payload["phases"]["goal_check"] + assert len(goal_check) == 3 + unmet = [g["expression"] for g in goal_check if not g["satisfied"]] + assert unmet == ["on(b3, b2)", "on(b2, b1)"] + assert payload["is_valid"] is False + + +def test_syntax_only_has_no_goal_check(tmp_path): + paths = write_pddl_files(tmp_path, BLOCKSWORLD_DOMAIN, BLOCKSWORLD_PROBLEM) + payload = format_json(PDDLValidator().validate_syntax(paths["domain"], paths["problem"])) + + assert "goal_check" not in payload["phases"] + assert payload["phases"]["goals"] is None + # And it must still be valid JSON when printed, as the CLI does. + assert json.loads(json.dumps(payload))["status"] == "VALID" From a73475443b465f5ae7be2320184a85a6aa59e271 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:15:35 +0000 Subject: [PATCH 6/9] fix: let --track match the spellings the README documents pyval domain.pddl problem.pddl plan.txt --trajectory --track fuel printed "No numeric fluents to display." for any fluent with arguments. Two comparisons were wrong at once: NumericTracker filtered on a display name ("fuel truck1") while snapshots are keyed by UPF's rendering ("fuel(truck1)"), and format_trajectory compared the raw reference with the same key. Route both through fluent_reference_matches, which accepts the bare function name (all instances), "fuel truck1" and "fuel(truck1)". A reference that matches nothing now warns with the fluents that do exist (new "Warnings" section in the text report, `warnings` in the JSON) instead of handing back an empty table. Adds tests/test_tracking.py (9 tests) and documents the accepted spellings in the README. --- CHANGELOG.md | 13 +++++ README.md | 5 ++ pyval/numeric_tracker.py | 83 ++++++++++++++++++++++---- pyval/report_formatter.py | 30 +++++++++- pyval/validator.py | 37 ++++++++++++ tests/test_tracking.py | 119 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 273 insertions(+), 14 deletions(-) create mode 100644 tests/test_tracking.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f725066..97f3ed7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S ## [Unreleased] ### Fixed +- `--track` matched nothing for any parameterised fluent, so the README's own + `pyval domain.pddl problem.pddl plan.txt --trajectory --track fuel` printed + "No numeric fluents to display.". Two different comparisons were wrong: the + tracker compared a display name (`fuel truck1`) with the snapshot key + (`fuel(truck1)`), and `format_trajectory` compared the raw reference with that + same key. Both now go through `fluent_reference_matches` + (`pyval/numeric_tracker.py`), which accepts the bare function name (every + instance), the PDDL spelling (`fuel truck1`) and UPF's rendering + (`fuel(truck1)`). A reference matching nothing raises a result-level warning + naming the fluents that exist, printed in a new "Warnings" section and carried + in the JSON output, instead of an empty table. - Plan quality metrics were silently dropped in two cases. `_evaluate_metric` (`pyval/plan_simulator.py`) used `state.get_value`, which UPF asserts on for arithmetic sub-expressions, so `(:metric minimize (+ (* 2 (fuel)) 5))` came @@ -66,6 +77,8 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S and a guard against the "defaults to 0" claim coming back. - `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the report wording, the JSON field, and a CLI run that must not print a traceback. +- `tests/test_tracking.py`: every accepted `--track` spelling, single-instance + selection, the unknown-reference warning, and the README command end to end. - `tests/test_json_output.py`: `is_valid` on every status, every goal in `goal_check`, and no `goal_check` when no plan ran. - `tests/test_metrics.py`: plain and arithmetic metrics, `:action-costs`, the diff --git a/README.md b/README.md index 5b7fbf9..2a306e9 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,11 @@ Unsupported PDDL features (durative actions, PDDL+ processes/events) are reporte - **JSON** (`--json`) — structured for programmatic consumption - **Trajectory** (`--trajectory`) — numeric fluent values per step, optionally filtered via `--track` +`--track` accepts the bare function name (`--track fuel`, every instance of `fuel`), +one instance in PDDL spelling (`--track "fuel truck1"`), or UPF's rendering +(`--track "fuel(truck1)"`). A reference that matches no numeric fluent is reported +as a warning listing the fluents that exist, rather than printing an empty table. + ## Development ```bash diff --git a/pyval/numeric_tracker.py b/pyval/numeric_tracker.py index 520d7e5..64baf76 100644 --- a/pyval/numeric_tracker.py +++ b/pyval/numeric_tracker.py @@ -25,13 +25,22 @@ def __init__( else: self._numeric_exprs.append(fluent_expr) - # Filter numeric fluents if tracking specific ones + # Filter numeric fluents if tracking specific ones. A reference may be + # the bare function name (`fuel`, every instance), or one instance in + # either spelling (`fuel truck1`, `fuel(truck1)`). + self.tracked_unknown: list[str] = [] if tracked_fluents is not None: - tracked_set = set(tracked_fluents) - self._numeric_exprs = [ - e for e in self._numeric_exprs - if _fluent_display_name(e) in tracked_set + kept = [ + expr + for expr in self._numeric_exprs + if _matches_any(tracked_fluents, str(expr)) ] + self.tracked_unknown = [ + reference + for reference in tracked_fluents + if not _matches_any([reference], *(str(e) for e in self._numeric_exprs)) + ] + self._numeric_exprs = kept def record(self, step: int, action: str | None, state) -> StateSnapshot: """Record a state snapshot at a given step.""" @@ -68,10 +77,60 @@ def get_numeric_trajectory(self) -> dict[str, list[float]]: return trajectory -def _fluent_display_name(expr) -> str: - """Extract a display name like 'fuel truck1' from a fluent expression.""" - if expr.is_fluent_exp(): - name = expr.fluent().name - args = " ".join(str(a) for a in expr.args) - return f"{name} {args}".strip() if args else name - return str(expr) +def available_numeric_fluents(problem: Problem) -> list[str]: + """Names of the tracked-able numeric fluent instances, as `str(expr)`.""" + return [ + str(expr) + for expr, value in problem.initial_values.items() + if not value.is_bool_constant() + ] + + +def fluent_reference_matches(reference: str, fluent_name: str) -> bool: + """Whether a user-supplied --track reference selects `fluent_name`. + + Accepts the PDDL spelling, UPF's rendering and the bare function name: + ``fuel`` (all instances of `fuel`), ``fuel truck1``, ``fuel(truck1)`` and + ``fuel(truck1, ...)`` all select ``fuel(truck1)``. Before this, `--track` + compared the display name (`fuel truck1`) with the snapshot key + (`fuel(truck1)`), so no value ever matched and the table came back empty. + """ + name, arguments = parse_fluent_reference(reference) + fluent, fluent_arguments = parse_fluent_reference(fluent_name) + if name != fluent: + return False + if arguments is None: + return True + return arguments == fluent_arguments + + +def parse_fluent_reference(reference: str) -> tuple[str, tuple[str, ...] | None]: + """Split a fluent reference into (name, arguments or None). + + ``None`` for the arguments means "every instance of this function". + """ + text = reference.strip() + if text.startswith("(") and text.endswith(")"): + text = text[1:-1].strip() + + head, separator, rest = text.partition("(") + if separator and rest.endswith(")"): + arguments = tuple( + argument.strip() for argument in rest[:-1].split(",") if argument.strip() + ) + return head.strip(), arguments + + parts = text.split() + if not parts: + return text, None + if len(parts) == 1: + return parts[0], None + return parts[0], tuple(parts[1:]) + + +def _matches_any(references, *fluent_names: str) -> bool: + return any( + fluent_reference_matches(reference, fluent_name) + for reference in references + for fluent_name in fluent_names + ) diff --git a/pyval/report_formatter.py b/pyval/report_formatter.py index f8975a3..deb276d 100644 --- a/pyval/report_formatter.py +++ b/pyval/report_formatter.py @@ -5,6 +5,8 @@ import json from typing import TYPE_CHECKING +from pyval.numeric_tracker import fluent_reference_matches + if TYPE_CHECKING: from pyval.models import ValidationResult @@ -133,6 +135,15 @@ def format_plain_text(result: ValidationResult, verbose: bool = False) -> str: parts = [f"{k} = {v}" for k, v in numeric_finals.items()] lines.append(f"Final state numeric values: {', '.join(parts)}") + # Result-level warnings (e.g. a --track request that matched nothing). Phase + # warnings were already printed with their phase; only print the others. + unreported = _unreported_warnings(result) + if unreported: + lines.append("") + lines.append("=== Warnings ===") + for warning in unreported: + lines.append(f"[WARNING] {warning}") + return "\n".join(lines) @@ -224,9 +235,21 @@ def format_json(result: ValidationResult) -> dict: if result.metric: output["metric"] = result.metric + if result.warnings: + output["warnings"] = result.warnings + return output +def _unreported_warnings(result: ValidationResult) -> list[str]: + """Warnings that the phase blocks did not already print.""" + printed: list[str] = [] + for phase in result.phases.values(): + if isinstance(phase, dict): + printed.extend(phase.get("warnings", [])) + return [warning for warning in result.warnings if warning not in printed] + + def _unevaluable_step(result: ValidationResult): """Return the first step the simulator could not evaluate, if any.""" for step in result.steps: @@ -255,8 +278,11 @@ def format_trajectory( # Filter if specific fluents requested if tracked: - tracked_set = set(tracked) - traj = {k: v for k, v in traj.items() if k in tracked_set} + traj = { + name: values + for name, values in traj.items() + if any(fluent_reference_matches(reference, name) for reference in tracked) + } if not traj: return "No matching numeric fluents found." diff --git a/pyval/validator.py b/pyval/validator.py index 6f8e2d4..86d2258 100644 --- a/pyval/validator.py +++ b/pyval/validator.py @@ -6,6 +6,7 @@ from unified_planning.plans import ActionInstance from pyval.models import ValidationResult +from pyval.numeric_tracker import available_numeric_fluents, fluent_reference_matches from pyval.syntax_checker import check_syntax @@ -90,6 +91,7 @@ def validate( } all_warnings = phase1_result["warnings"] + phase2_result["warnings"] + all_warnings.extend(_unmatched_tracking_warnings(problem, tracked_fluents)) return ValidationResult( status="VALID" if is_valid else "INVALID", @@ -106,6 +108,41 @@ def validate( ) +def _unmatched_tracking_warnings( + problem: Problem, tracked_fluents: list[str] | None +) -> list[str]: + """Warn about --track values that select no numeric fluent at all. + + A typo used to produce an empty trajectory table with no explanation, which + reads exactly like a plan with no numeric fluents. + """ + if not tracked_fluents: + return [] + + available = available_numeric_fluents(problem) + unmatched = [ + reference + for reference in tracked_fluents + if not any(fluent_reference_matches(reference, name) for name in available) + ] + if not unmatched: + return [] + + listed = ", ".join(f"'{reference}'" for reference in unmatched) + if available: + shown = ", ".join(available[:10]) + if len(available) > 10: + shown += f", ... ({len(available) - 10} more)" + return [ + f"Tracking request {listed} matches no numeric fluent. " + f"Available: {shown}" + ] + return [ + f"Tracking request {listed} matches no numeric fluent: this problem has " + f"no numeric fluents with an initial value." + ] + + def _validate_plan_structure( problem: Problem, plan_path: str ) -> tuple[dict, list[ActionInstance], dict[str, str]]: diff --git a/tests/test_tracking.py b/tests/test_tracking.py new file mode 100644 index 0000000..4d9c9cc --- /dev/null +++ b/tests/test_tracking.py @@ -0,0 +1,119 @@ +"""`--track` must accept the spellings people actually type. + +The README documents `pyval ... --trajectory --track fuel`, and that command +printed "No numeric fluents to display." for every domain whose fluent has +arguments: the tracker compared a display name ("fuel truck1") against a +snapshot key ("fuel(truck1)"), and the formatter compared the raw reference +against the same key, so nothing ever matched. +""" + +import subprocess +import sys + +from pyval import PDDLValidator +from pyval.numeric_tracker import ( + fluent_reference_matches, + parse_fluent_reference, +) +from pyval.report_formatter import format_json, format_plain_text, format_trajectory +from tests.conftest import ( + LOGISTICS_FUEL_DOMAIN, + LOGISTICS_FUEL_PROBLEM, + LOGISTICS_FUEL_VALID_PLAN, + write_pddl_files, +) + + +def _files(tmp_path): + return write_pddl_files( + tmp_path, LOGISTICS_FUEL_DOMAIN, LOGISTICS_FUEL_PROBLEM, LOGISTICS_FUEL_VALID_PLAN + ) + + +def _validate(files, tracked): + return PDDLValidator().validate( + files["domain"], files["problem"], files["plan"], tracked_fluents=tracked + ) + + +def test_parse_reference_spellings(): + assert parse_fluent_reference("fuel") == ("fuel", None) + assert parse_fluent_reference("fuel truck1") == ("fuel", ("truck1",)) + assert parse_fluent_reference("fuel(truck1)") == ("fuel", ("truck1",)) + assert parse_fluent_reference("distance(loc1, loc2)") == ("distance", ("loc1", "loc2")) + + +def test_matching_bare_name_covers_every_instance(): + assert fluent_reference_matches("fuel", "fuel(truck1)") + assert fluent_reference_matches("fuel", "fuel(truck2)") + assert not fluent_reference_matches("fuel", "distance(loc1, loc2)") + + +def test_matching_single_instance(): + assert fluent_reference_matches("fuel truck1", "fuel(truck1)") + assert fluent_reference_matches("fuel(truck1)", "fuel(truck1)") + assert not fluent_reference_matches("fuel truck2", "fuel(truck1)") + assert fluent_reference_matches("distance loc1 loc2", "distance(loc1, loc2)") + + +def test_bare_function_name_tracks_all_instances(tmp_path): + result = _validate(_files(tmp_path), ["fuel"]) + assert list(result.numeric_trajectory) == ["fuel(truck1)"] + assert result.numeric_trajectory["fuel(truck1)"][:2] == [100.0, 90.0] + + +def test_instance_name_tracks_one_fluent(tmp_path): + result = _validate(_files(tmp_path), ["distance loc1 loc2", "fuel truck1"]) + assert set(result.numeric_trajectory) == {"fuel(truck1)", "distance(loc1, loc2)"} + + +def test_trajectory_tables_the_tracked_fluent(tmp_path): + files = _files(tmp_path) + result = _validate(files, ["fuel"]) + table = format_trajectory(result, tracked=["fuel"]) + + assert "fuel(truck1)" in table + assert "distance" not in table + assert "100" in table + + +def test_unknown_track_reference_is_reported(tmp_path): + files = _files(tmp_path) + result = _validate(files, ["fuell"]) + + assert result.numeric_trajectory == {} + assert any("'fuell' matches no numeric fluent" in w for w in result.warnings) + assert any("fuel(truck1)" in w for w in result.warnings if "'fuell'" in w) + + text = format_plain_text(result) + assert "=== Warnings ===" in text + assert "'fuell' matches no numeric fluent" in text + assert format_json(result)["warnings"] + + +def test_no_warning_when_every_reference_matches(tmp_path): + result = _validate(_files(tmp_path), ["fuel", "distance"]) + assert not [w for w in result.warnings if "matches no numeric fluent" in w] + + +def test_cli_readme_command_tracks_fuel(tmp_path): + """The exact command from the README: --trajectory --track fuel.""" + files = _files(tmp_path) + completed = subprocess.run( + [ + sys.executable, + "-m", + "pyval.cli", + files["domain"], + files["problem"], + files["plan"], + "--trajectory", + "--track", + "fuel", + ], + capture_output=True, + text=True, + ) + assert completed.returncode == 0 + assert "fuel(truck1)" in completed.stdout + assert "No numeric fluents" not in completed.stdout From 87fdba2bf72009d26c9c2f0ef47ce0af68e934d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:17:18 +0000 Subject: [PATCH 7/9] fix: warn only about numeric instances an action can write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporting every unassigned instance of every numeric function was noisy: a static table like (distance ?a ?b) is routinely given only for the pairs that exist, so ordinary logistics benchmarks grew a warning per run. The instances that actually hurt are the ones an effect writes — a read of a missing static pair now gets the precise step-level error from the previous commit instead. Report a function when an action assigns or updates it, or when it has no initial value at all; stay quiet about partial coverage of read-only tables. --- CHANGELOG.md | 14 +++++-- pyval/syntax_checker.py | 36 ++++++++++++++---- tests/test_numeric_init.py | 76 ++++++++++++++++++++++++++++++++++---- 3 files changed, 107 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97f3ed7..a37c6bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,12 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S (validation aborted with a traceback). `_check_numeric_initialization` in `pyval/syntax_checker.py` now enumerates the ground instances of every numeric function and names the ones `:init` leaves out, with a valid PDDL - repair hint (`Add (= (fuel t2) ) to :init.`). The warning stays a + repair hint (`Add (= (fuel t2) ) to :init.`). It reports only the + instances that can bite: a function an action *writes* must have a value for + every object it ranges over, and a function with no values at all is reported + as a whole — a static table such as `(distance ?a ?b)` is routinely specified + only for the pairs that exist, and a plan that reads a missing pair now gets + the step-level error instead of a blanket warning. The warning stays a warning: a plan that never reads the instance is still valid. Functions with more than 250 ground instances are not enumerated and get a summary message instead. @@ -72,9 +77,10 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versioning: [S "every goal passed" — the same ambiguity 0.1.5 removed from the plain-text report, left in the machine-readable one. `phases.goals` is unchanged for existing consumers; `tests/test_json_output.py` covers both keys. -- `tests/test_numeric_init.py`: partial initialisation, fully initialised - functions, zero-arity functions, summarised long lists, the enumeration bound, - and a guard against the "defaults to 0" claim coming back. +- `tests/test_numeric_init.py`: partial coverage of a written function, static + tables left partially specified, zero-arity functions, summarised long lists, + the enumeration bound, and a guard against the "defaults to 0" claim coming + back. - `tests/test_evaluation_errors.py`: the precondition and effect crash paths, the report wording, the JSON field, and a CLI run that must not print a traceback. - `tests/test_tracking.py`: every accepted `--track` spelling, single-instance diff --git a/pyval/syntax_checker.py b/pyval/syntax_checker.py index 1522b2e..d98a5a9 100644 --- a/pyval/syntax_checker.py +++ b/pyval/syntax_checker.py @@ -81,8 +81,16 @@ def _check_numeric_initialization(problem: Problem) -> list[str]: fuel(t2)"), which used to abort validation with a traceback. The check is per ground instance, not per declared name, because initialising ``(fuel t1)`` says nothing about ``(fuel t2)``. + + Only instances that matter are reported: a function an action *writes* must + have a value for every object it ranges over (the effect raises otherwise), + and a function with no initial value at all is reported as a whole. A static + table such as ``(distance ?a ?b)`` is routinely specified only for the pairs + that exist, so partial coverage there is not a defect — if a plan reads a + missing pair, the step reports it by name. """ warnings: list[str] = [] + modified = _functions_written_by_actions(problem) for fluent in problem.fluents: if not (fluent.type.is_int_type() or fluent.type.is_real_type()): @@ -90,21 +98,35 @@ def _check_numeric_initialization(problem: Problem) -> list[str]: instances = _ground_instances(problem, fluent) if instances is None: - warnings.append( - f"Numeric function '{fluent.name}' has more than " - f"{_MAX_GROUND_INSTANCES} ground instances, so it was not " - f"cross-checked against :init. No default value is assumed: " - f"any instance that :init leaves out has no value in the state." - ) + if fluent.name in modified: + warnings.append( + f"Numeric function '{fluent.name}' has more than " + f"{_MAX_GROUND_INSTANCES} ground instances, so it was not " + f"cross-checked against :init. No default value is assumed: " + f"any instance that :init leaves out has no value in the state." + ) continue missing = [expr for expr in instances if expr not in problem.initial_values] - if missing: + if not missing: + continue + if fluent.name in modified or len(missing) == len(instances): warnings.append(_missing_init_warning(fluent.name, missing, len(instances))) return warnings +def _functions_written_by_actions(problem: Problem) -> set[str]: + """Names of the functions some action effect assigns or updates.""" + names: set[str] = set() + for action in problem.actions: + for effect in action.effects: + fluent_expression = getattr(effect, "fluent", None) + if fluent_expression is not None and fluent_expression.is_fluent_exp(): + names.add(fluent_expression.fluent().name) + return names + + def _ground_instances(problem: Problem, fluent) -> list | None: """Enumerate the ground instances of `fluent`, or None if there are too many.""" domains = [] diff --git a/tests/test_numeric_init.py b/tests/test_numeric_init.py index d3f1236..504e8d4 100644 --- a/tests/test_numeric_init.py +++ b/tests/test_numeric_init.py @@ -76,14 +76,74 @@ def test_fully_initialised_function_is_not_reported(tmp_path): assert not [w for w in warnings if "'fuel'" in w] -def test_every_instance_missing_is_reported_once(tmp_path): - warnings = _warnings(tmp_path, "(= (distance l1 l2) 10) (= (fuel t1) 1) (= (fuel t2) 1)") - distance = [w for w in warnings if "'distance'" in w] - assert len(distance) == 1 - assert "3 of its 4 ground instances" in distance[0] - assert "distance(l1, l2)" not in distance[0] - # Comma-separated UPF rendering must not survive into the PDDL repair hint - assert "Add (= (distance l1 l1) ) to :init." in distance[0] +COST_DOMAIN = """\ +(define (domain costs) + (:requirements :strips :typing :numeric-fluents) + (:types thing) + (:predicates (p ?x - thing)) + (:functions (cost ?x - thing) (distance ?a - thing ?b - thing)) + (:action spend + :parameters (?x - thing) + :precondition (p ?x) + :effect (decrease (cost ?x) 1)) +) +""" + + +def _cost_problem(init: str, objects: int = 4) -> str: + objs = " ".join(f"o{i}" for i in range(objects)) + return f"""\ +(define (problem costs-p1) + (:domain costs) + (:objects {objs} - thing) + (:init (p o0) {init}) + (:goal (p o0)) +) +""" + + +def test_partial_coverage_of_a_written_function_is_reported(tmp_path): + """`cost` is updated by `spend`, so every instance needs a value.""" + paths = write_pddl_files( + tmp_path, COST_DOMAIN, _cost_problem("(= (cost o0) 5) (= (cost o1) 5)") + ) + result, _ = check_syntax(paths["domain"], paths["problem"]) + + message = [w for w in result["warnings"] if "'cost'" in w] + assert len(message) == 1 + assert "2 of its 4 ground instances" in message[0] + assert "cost(o0)" not in message[0] + assert "Add (= (cost o2) ) to :init." in message[0] + + +def test_partial_coverage_of_a_static_table_is_not_reported(tmp_path): + """`(distance o0 o1)` only is normal for a static table nobody writes.""" + paths = write_pddl_files( + tmp_path, + COST_DOMAIN, + _cost_problem( + "(= (cost o0) 5) (= (cost o1) 5) (= (cost o2) 5) (= (cost o3) 5)" + " (= (distance o0 o1) 10)" + ), + ) + result, _ = check_syntax(paths["domain"], paths["problem"]) + + assert not [w for w in result["warnings"] if "'distance'" in w] + + +def test_static_table_with_no_values_at_all_is_reported(tmp_path): + paths = write_pddl_files( + tmp_path, + COST_DOMAIN, + _cost_problem( + "(= (cost o0) 5) (= (cost o1) 5) (= (cost o2) 5) (= (cost o3) 5)" + ), + ) + result, _ = check_syntax(paths["domain"], paths["problem"]) + + message = [w for w in result["warnings"] if "'distance'" in w] + assert len(message) == 1 + assert "16 of its 16 ground instances" in message[0] def test_zero_arity_function_without_init_is_reported(tmp_path): From eef8efad30462d28cb94b7158eff244961e74b58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:17:43 +0000 Subject: [PATCH 8/9] docs: describe numeric initialisation and evaluation errors in the README --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index 2a306e9..36aa15a 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,13 @@ PyVAL runs a three-phase pipeline and halts on the first fatal error: Unsupported PDDL features (durative actions, PDDL+ processes/events) are reported as warnings rather than silent failures. +Numeric fluents have no default value in PDDL, so `:init` must assign every +instance an action reads or writes. Phase 1 warns about the instances a written +function leaves out; if the simulator still cannot evaluate a step at execution +time, validation stops there and reports `EVALUATION ERROR` naming the fluent and +the repair, instead of claiming the plan is invalid — an unevaluable step proves +nothing about the plan. + ## Output Modes - **Plain text** (default, VAL-like, optionally `-v` for per-step detail) From 24e6e954344d46685b883c50a87174c7bf8c9ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zoe=20=E2=80=94=20ghost=20fixer?= Date: Mon, 9 Nov 2026 12:18:56 +0000 Subject: [PATCH 9/9] chore: sort imports and satisfy ruff in the new tests --- pyval/plan_simulator.py | 2 +- pyval/validator.py | 8 ++++---- tests/test_evaluation_errors.py | 2 +- tests/test_metrics.py | 1 - tests/test_numeric_init.py | 5 ++--- tests/test_precondition_diagnostics.py | 1 - tests/test_tracking.py | 1 + 7 files changed, 9 insertions(+), 11 deletions(-) diff --git a/pyval/plan_simulator.py b/pyval/plan_simulator.py index c1ac88b..668540d 100644 --- a/pyval/plan_simulator.py +++ b/pyval/plan_simulator.py @@ -161,9 +161,9 @@ def _evaluate_metric( state it was applied in. """ from unified_planning.model.metrics import ( + MaximizeExpressionOnFinalState, MinimizeActionCosts, MinimizeExpressionOnFinalState, - MaximizeExpressionOnFinalState, ) from unified_planning.model.walkers import StateEvaluator diff --git a/pyval/validator.py b/pyval/validator.py index 86d2258..c1eb89f 100644 --- a/pyval/validator.py +++ b/pyval/validator.py @@ -133,14 +133,14 @@ def _unmatched_tracking_warnings( shown = ", ".join(available[:10]) if len(available) > 10: shown += f", ... ({len(available) - 10} more)" - return [ + return [( f"Tracking request {listed} matches no numeric fluent. " f"Available: {shown}" - ] - return [ + )] + return [( f"Tracking request {listed} matches no numeric fluent: this problem has " f"no numeric fluents with an initial value." - ] + )] def _validate_plan_structure( diff --git a/tests/test_evaluation_errors.py b/tests/test_evaluation_errors.py index 516721e..6a9ba74 100644 --- a/tests/test_evaluation_errors.py +++ b/tests/test_evaluation_errors.py @@ -11,7 +11,6 @@ from pyval.report_formatter import format_json, format_plain_text from tests.conftest import write_pddl_files - PARTIAL_DOMAIN = """\ (define (domain partial) (:requirements :strips :typing :numeric-fluents) @@ -131,6 +130,7 @@ def test_cli_survives_the_unevaluable_step(tmp_path): [sys.executable, "-m", "pyval.cli", paths["domain"], paths["problem"], paths["plan"]], capture_output=True, text=True, + check=False, ) assert completed.returncode == 1 assert "Traceback" not in completed.stderr diff --git a/tests/test_metrics.py b/tests/test_metrics.py index dd0e92e..2153c31 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -11,7 +11,6 @@ from pyval.report_formatter import format_json, format_plain_text from tests.conftest import write_pddl_files - PROBLEM = """\ (define (problem metric-p1) (:domain metric) diff --git a/tests/test_numeric_init.py b/tests/test_numeric_init.py index 504e8d4..a50819d 100644 --- a/tests/test_numeric_init.py +++ b/tests/test_numeric_init.py @@ -10,7 +10,6 @@ from pyval.syntax_checker import check_syntax from tests.conftest import write_pddl_files - NUMERIC_DOMAIN = """\ (define (domain numinit) (:requirements :strips :typing :numeric-fluents) @@ -193,7 +192,7 @@ def test_long_missing_list_is_summarised(tmp_path): """ paths = write_pddl_files(tmp_path, domain, problem) result, _ = check_syntax(paths["domain"], paths["problem"]) - message = [w for w in result["warnings"] if "'cost'" in w][0] + message = next(w for w in result["warnings"] if "'cost'" in w) assert "10 of its 10 ground instances" in message assert "4 more)" in message @@ -221,6 +220,6 @@ def test_huge_ground_space_is_not_enumerated(tmp_path): """ paths = write_pddl_files(tmp_path, domain, problem) result, _ = check_syntax(paths["domain"], paths["problem"]) - message = [w for w in result["warnings"] if "'cost'" in w][0] + message = next(w for w in result["warnings"] if "'cost'" in w) assert "more than 250 ground instances" in message assert "No default value is assumed" in message diff --git a/tests/test_precondition_diagnostics.py b/tests/test_precondition_diagnostics.py index c951856..7de39cb 100644 --- a/tests/test_precondition_diagnostics.py +++ b/tests/test_precondition_diagnostics.py @@ -16,7 +16,6 @@ write_pddl_files, ) - DISJUNCT_DOMAIN = """\ (define (domain disjuncts) (:requirements :strips :typing) diff --git a/tests/test_tracking.py b/tests/test_tracking.py index 4d9c9cc..60ac7eb 100644 --- a/tests/test_tracking.py +++ b/tests/test_tracking.py @@ -113,6 +113,7 @@ def test_cli_readme_command_tracks_fuel(tmp_path): ], capture_output=True, text=True, + check=False, ) assert completed.returncode == 0 assert "fuel(truck1)" in completed.stdout