Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions app/desktop/studio_server/test_eval_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -6388,6 +6388,33 @@ def test_exact_match_fail(self, client, mock_task, mock_task_from_id):
assert response.status_code == 200
assert response.json()["scores"]["accuracy"] == 0.0

@pytest.mark.parametrize(
"final_message,expected_score",
[
# Same expression and same expected_value both times. The only
# difference is whether `user` is there, so the 0.0 can only come
# from the extraction failing -- not from a value that never matched.
('{"user": {"status": "hello"}}', 1.0),
('{"status": "hello"}', 0.0),
],
ids=["field_present", "field_missing"],
)
def test_missing_nested_field_scores_fail(
self, client, mock_task, mock_task_from_id, final_message, expected_score
):
# Reaching into a value that isn't there is a scored FAIL, not a request error.
mock_task_from_id.return_value = mock_task
payload = self._payload()
payload["properties"]["value_expression"] = (
"(final_message | fromjson).user.status"
)
payload["eval_input"]["final_message"] = final_message
response = client.post(self._url(), json=payload)
assert response.status_code == 200
body = response.json()
assert body["scores"]["accuracy"] == expected_score
assert body["skipped_reason"] is None

def test_nothing_is_persisted(self, client, mock_task, mock_task_from_id):
mock_task_from_id.return_value = mock_task
response = client.post(self._url(), json=self._payload())
Expand Down
13 changes: 13 additions & 0 deletions libs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,19 @@ def test_fromjson_valid_json_succeeds(self):
assert value == "val"
assert fail_result is None

def test_empty_trace_scores_fail_not_missing_trace_skip(self):
# A trace that exists but is empty is not a missing trace: the run did
# produce one, it just has nothing in it. That scores FAIL, so the
# missing-trace skip must key off `is None`, not falsiness.
inp = _make_input(trace=[])
value, fail_result = extract_output_value(
"trace[-1].tool_calls[0].function.name", inp, _SAMPLE_SCORES
)
assert value is None
assert fail_result is not None
assert fail_result.skipped_reason is None
assert fail_result.scores == {"s1": 0.0, "s2": 0.0}


# ---------------------------------------------------------------------------
# stringify_for_match
Expand Down
22 changes: 21 additions & 1 deletion libs/core/kiln_ai/datamodel/eval.py
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ def validate_properties(self) -> Self:
raise ValueError(f"Invalid eval config type: {self.config_type}")

@model_validator(mode="after")
def validate_v2_templates_and_expressions(self) -> Self:
def validate_v2_templates_and_expressions(self, info: ValidationInfo) -> Self:
if self.config_type != EvalConfigType.v2 or not isinstance(
self.properties, BaseModel
):
Expand All @@ -1037,6 +1037,7 @@ def validate_v2_templates_and_expressions(self) -> Self:
from kiln_ai.utils.jinja_engine import (
compile_expression_or_raise,
compile_template_or_raise,
expression_variables,
)

props = self.properties
Expand Down Expand Up @@ -1069,6 +1070,25 @@ def validate_v2_templates_and_expressions(self) -> Self:
):
if props.value_expression is not None:
compile_expression_or_raise(props.value_expression)
# Syntax alone isn't enough: a typo'd root variable resolves to
# Undefined at eval time, which scores every row a silent 0.0.
# Catch it while the author can still see what they typed.
#
# Authoring-time only. A config written before this check exists
# may name a variable we now reject, and refusing to load it would
# take the whole eval down rather than the one check that was
# already scoring zeros.
if not self.loading_from_file(info):
allowed = set(EvalTaskInput.model_fields.keys())
unknown = sorted(
expression_variables(props.value_expression) - allowed
)
if unknown:
raise ValueError(
f"value_expression references unknown variable "
f"'{unknown[0]}'. Available variables: "
f"{', '.join(sorted(allowed))}."
)

return self

Expand Down
73 changes: 73 additions & 0 deletions libs/core/kiln_ai/datamodel/test_eval_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -2557,6 +2557,16 @@ def _make_v2_eval_config(**kwargs) -> EvalConfig:
return EvalConfig(name="V2 Test", config_type=EvalConfigType.v2, **kwargs)


# One value_expression per EvalTaskInput field, keyed by the root variable it
# reads. Checked against EvalTaskInput.model_fields so the coverage claim holds.
_EVAL_INPUT_FIELD_EXPRESSIONS = {
"final_message": "final_message",
"trace": "trace[-1].content",
"task_input": "task_input | upper",
"reference_data": "reference_data",
}


class TestV2TemplateValidation:
def test_valid_prompt_template(self):
"""A prompt_template with a Jinja expression passes validation."""
Expand Down Expand Up @@ -2666,6 +2676,69 @@ def test_invalid_value_expression(self):
),
)

@pytest.mark.parametrize(
"root,expression",
list(_EVAL_INPUT_FIELD_EXPRESSIONS.items()),
)
def test_value_expression_accepts_every_eval_input_field(self, root, expression):
# Tied to the model so a new EvalTaskInput field can't leave this test
# named "every field" while silently skipping one.
assert set(_EVAL_INPUT_FIELD_EXPRESSIONS) == set(EvalTaskInput.model_fields)
cfg = _make_v2_eval_config(
properties=ExactMatchProperties(
expected_value="yes",
value_expression=expression,
),
)
assert cfg.properties.value_expression == expression

@pytest.mark.parametrize(
"expression,unknown",
[
("outpt.status", "outpt"),
("messages[-1].content", "messages"),
("final_message ~ typo", "typo"),
],
)
def test_value_expression_rejects_unknown_variable(self, expression, unknown):
"""A typo'd root fails silently at runtime, so it has to fail loudly here.

Depending on what the expression does with it, the typo either resolves
to Undefined and scores every row 0.0, or -- as with `~`, which
stringifies Undefined to '' -- produces a plausible-looking wrong value
('hi' for `final_message ~ typo`) that no one notices.
"""
with pytest.raises(
ValidationError, match=f"unknown variable '{unknown}'"
) as exc:
_make_v2_eval_config(
properties=ExactMatchProperties(
expected_value="yes",
value_expression=expression,
),
)
assert "final_message, reference_data, task_input, trace" in str(exc.value)

def test_value_expression_unknown_variable_still_loads_from_file(self):
"""Already-saved configs keep loading; the check gates writes, not reads."""
cfg = EvalConfig.model_validate(
{
"v": 1,
"id": "123",
"name": "Saved before the check existed",
"config_type": "v2",
"model_type": "eval_config",
"properties": {
"type": "exact_match",
"expected_value": "yes",
"value_expression": "outpt.status",
},
},
context={"loading_from_file": True},
)
assert isinstance(cfg.properties, ExactMatchProperties)
assert cfg.properties.value_expression == "outpt.status"

def test_none_value_expression_skipped(self):
"""value_expression=None (default) should not be validated."""
cfg = _make_v2_eval_config(
Expand Down
47 changes: 40 additions & 7 deletions libs/core/kiln_ai/utils/jinja_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
Public API:
- compile_template_or_raise(template) -> None
- compile_expression_or_raise(expression) -> None
- expression_variables(expression) -> set[str]
- render_input_transform(transform, task_input) -> str
- extract(expression, data) -> Any
"""
Expand All @@ -20,18 +21,21 @@
import types
from typing import TYPE_CHECKING, Any

from jinja2 import StrictUndefined, TemplateSyntaxError, Undefined
from jinja2 import StrictUndefined, TemplateSyntaxError, Undefined, UndefinedError
from jinja2.sandbox import SandboxedEnvironment

if TYPE_CHECKING:
from kiln_ai.datamodel.input_transform import InputTransform


class JinjaExtractionError(ValueError):
"""Raised when a Jinja2 filter encounters invalid data during extraction.
"""Raised when a Jinja2 expression encounters missing or invalid data during extraction.

For example, the ``fromjson`` filter raises this when the input string is
not valid JSON.
Covers both filters that reject their input (``fromjson`` on a non-JSON
string) and expressions that operate on a value that isn't there (reaching
into a missing nested field, indexing past the end of a list). Callers treat
it as "this input didn't have what the expression asked for" rather than an
unexpected crash.
"""


Expand Down Expand Up @@ -85,6 +89,24 @@ def compile_expression_or_raise(expression: str) -> None:
) from e


def expression_variables(expression: str) -> set[str]:
"""Return the namespace variables a Jinja2 expression reads.

An expression is not a template -- ``final_message.strip()`` parsed as a
template is just literal text with no variables -- so it is wrapped in an
output block before the AST walk. Raises ValueError on syntax error.
"""
from jinja2 import meta

try:
ast = _expression_env.parse("{{ " + expression + " }}")
except TemplateSyntaxError as e:
raise ValueError(
f"Invalid Jinja2 expression: {e.message} (line {e.lineno})"
) from e
return meta.find_undeclared_variables(ast)


def render_input_transform(
transform: InputTransform,
task_input: Any,
Expand All @@ -108,6 +130,7 @@ def extract(expression: str, data: dict) -> Any:
- Missing keys return Undefined (not None, not a raise).
- Explicit null values return None.
- Generators are auto-materialized to lists.
- Operating on a missing value raises JinjaExtractionError, never UndefinedError.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
"""
try:
compiled = _expression_env.compile_expression(
Expand All @@ -117,9 +140,19 @@ def extract(expression: str, data: dict) -> Any:
raise ValueError(
f"Invalid Jinja2 expression: {e.message} (line {e.lineno})"
) from e
result = compiled(**data)
if isinstance(result, types.GeneratorType):
result = list(result)
try:
result = compiled(**data)
# Materializing inside the try is load-bearing: map/selectattr/groupby
# return lazy generators, so their per-item lookups don't run until
# list() does. A missing field there raises here, not above.
if isinstance(result, types.GeneratorType):
result = list(result)
except UndefinedError as e:
# A single missing lookup yields Undefined, but any further operation on
# it (attribute access, indexing, a filter) raises. That's still just
# missing data, so surface it as an extraction error callers handle --
# keeping Jinja's message, which names the field that wasn't there.
raise JinjaExtractionError(str(e)) from e
return result


Expand Down
81 changes: 81 additions & 0 deletions libs/core/kiln_ai/utils/test_jinja_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
JinjaExtractionError,
compile_expression_or_raise,
compile_template_or_raise,
expression_variables,
extract,
render_input_transform,
)
Expand Down Expand Up @@ -173,6 +174,86 @@ def test_malformed_expression_raises_value_error(self):
extract("{{ invalid", {})


class TestExpressionVariables:
@pytest.mark.parametrize(
"expression,expected",
[
# A bare expression has no {{ }} of its own, so it only reports
# variables once the function wraps it.
("final_message", {"final_message"}),
("final_message.strip()", {"final_message"}),
("(final_message | fromjson).user.status", {"final_message"}),
("trace | map(attribute='x') | list", {"trace"}),
("outpt.status", {"outpt"}),
# Delimiters inside a string literal are lexed as text, not as the
# end of the wrapper block.
('final_message ~ "}}"', {"final_message"}),
("'literal'", set()),
],
)
def test_reports_referenced_variables(self, expression, expected):
assert expression_variables(expression) == expected

def test_syntax_error_raises_value_error(self):
with pytest.raises(ValueError, match="Invalid Jinja2 expression"):
expression_variables("final_message[")


class TestExtractOperationsOnMissingData:
"""Operating on a missing value is an extraction error, not an unhandled raise.

A single missing lookup returns Undefined, but anything done to that
Undefined raises inside Jinja. Callers only handle JinjaExtractionError, so
extract() must convert -- while keeping Jinja's message, which names the
field that wasn't there.
"""

@pytest.mark.parametrize(
"expression,data,expected_message",
[
# A filter or operator that forces the Undefined.
("missing | int", {}, "'missing' is undefined"),
("missing + 1", {}, "'missing' is undefined"),
# Reaching into a field the parsed JSON doesn't have.
(
"(final_message | fromjson).user.status",
{"final_message": '{"status": "ok"}'},
"'dict object' has no attribute 'user'",
),
# Indexing past the end of a list.
(
"trace[-1].tool_calls[0].function.name",
{"trace": []},
"has no element -1",
),
],
)
def test_raising_operation_on_missing_value(
self, expression, data, expected_message
):
# Only operations that force the value raise; others stay silent, which
# test_silent_operation_on_missing_value pins.
with pytest.raises(JinjaExtractionError, match=expected_message):
extract(expression, data)

def test_silent_operation_on_missing_value(self):
# The other side of the boundary: `~` stringifies Undefined to '' rather
# than raising, so a typo'd name yields a wrong value, not an error.
assert extract("final_message ~ typo", {"final_message": "hi"}) == "hi"

def test_lazy_generator_over_missing_field_raises_extraction_error(self):
# The sole guard that generators are materialized inside extract()'s try:
# map() defers its per-item lookups, so the raise fires during list(),
# not when the compiled expression is called.
with pytest.raises(
JinjaExtractionError, match="'dict object' has no attribute 'tool_calls'"
):
extract(
"trace | map(attribute='tool_calls.0.function.name')",
{"trace": [{"content": "no tools"}]},
)


class TestTrimAndLstripBlocks:
def test_trim_blocks_strips_newline_after_block_tag(self):
template = "{% if True %}\nyes\n{% endif %}"
Expand Down
Loading