diff --git a/CHANGELOG.md b/CHANGELOG.md index a43da7c..a37c6bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,94 @@ 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 + 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 + 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 + `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 + 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.`). 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. + +### 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 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 + 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 + 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 + left without diagnostics. + ## [0.1.6] — 2026-11-09 ### Fixed diff --git a/README.md b/README.md index 5b7fbf9..36aa15a 100644 --- a/README.md +++ b/README.md @@ -62,12 +62,24 @@ 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) - **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/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/pyval/models.py b/pyval/models.py index a767f8f..bcb2d95 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 @@ -56,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/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/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..668540d 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( @@ -33,38 +41,47 @@ 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) - 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,12 +91,25 @@ 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, + )) + 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, @@ -90,28 +120,110 @@ def simulate( ) -def _evaluate_metric(problem: Problem, final_state) -> dict | None: - """Evaluate plan quality metric against the final state.""" +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, 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 ( - MinimizeExpressionOnFinalState, MaximizeExpressionOnFinalState, + MinimizeActionCosts, + MinimizeExpressionOnFinalState, ) + 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 ce9c68d..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 @@ -56,6 +58,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 +79,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: @@ -103,10 +116,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: @@ -118,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) @@ -125,6 +151,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": {}, } @@ -161,6 +188,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, @@ -189,13 +218,46 @@ 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 + 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: + 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] = [] @@ -216,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/syntax_checker.py b/pyval/syntax_checker.py index 53dfbb4..d98a5a9 100644 --- a/pyval/syntax_checker.py +++ b/pyval/syntax_checker.py @@ -2,9 +2,20 @@ from __future__ import annotations +import itertools + 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. +_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 +73,100 @@ 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)``. + + 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 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: + 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 not missing: + continue + if fluent.name in modified or len(missing) == len(instances): + warnings.append(_missing_init_warning(fluent.name, missing, len(instances))) - for fluent_expr in problem.initial_values: - if fluent_expr.is_fluent_exp(): - initialized_fluents.add(fluent_expr.fluent().name) + return warnings - for name in declared_numeric_fluents - initialized_fluents: - warnings.append( - f"Numeric function '{name}' has no initial value assigned — defaults to 0" + +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 = [] + 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 warnings + 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." + ) diff --git a/pyval/validator.py b/pyval/validator.py index 7060561..c1eb89f 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", @@ -100,11 +102,47 @@ def validate( numeric_trajectory=numeric_traj, failed_step=failed_step, unsatisfied_goals=unsatisfied_goals, + goal_results=goal_results, warnings=all_warnings, metric=metric, ) +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_evaluation_errors.py b/tests/test_evaluation_errors.py new file mode 100644 index 0000000..6a9ba74 --- /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, + check=False, + ) + 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 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" diff --git a/tests/test_metrics.py b/tests/test_metrics.py new file mode 100644 index 0000000..2153c31 --- /dev/null +++ b/tests/test_metrics.py @@ -0,0 +1,130 @@ +"""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 diff --git a/tests/test_numeric_init.py b/tests/test_numeric_init.py new file mode 100644 index 0000000..a50819d --- /dev/null +++ b/tests/test_numeric_init.py @@ -0,0 +1,225 @@ +"""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] + + +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): + 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 = 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 + + +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 = 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 new file mode 100644 index 0000000..7de39cb --- /dev/null +++ b/tests/test_precondition_diagnostics.py @@ -0,0 +1,191 @@ +"""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 diff --git a/tests/test_tracking.py b/tests/test_tracking.py new file mode 100644 index 0000000..60ac7eb --- /dev/null +++ b/tests/test_tracking.py @@ -0,0 +1,120 @@ +"""`--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, + check=False, + ) + assert completed.returncode == 0 + assert "fuel(truck1)" in completed.stdout + assert "No numeric fluents" not in completed.stdout