[eb-v2 review] Eval V2 core: sandbox, trace persistence, and datamodel guard fixes - #1652
chiang-daniel wants to merge 3 commits into
Conversation
…alidation, EvalRun docstring, dead tool_id branch, test comment cleanup Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, fail-loud g_eval preflight, parser-based trace detection, tombstone dedup parity Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…for successful full_trace task-run evals, with load-compat for historical files Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR updates V2 eval persistence, validation, template analysis, and sandbox execution. It stores full traces on successful task evaluations, carries superseded skip tombstones into replacement jobs, tightens EvalRun and EvalConfig validation, refines template-variable detection, and hardens sandbox scorer result handling. ChangesV2 eval execution and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Coverage ReportOverall Coverage: 93% Diff: origin/review/eb-v2/base...HEAD
Summary
Line-by-lineView line-by-line diff coveragelibs/core/kiln_ai/adapters/eval/eval_runner.pyLines 270-278 270 if (
271 run.task_run_config_id is None
272 or run.task_run_config_id not in already_run[eval_config.id]
273 ):
! 274 continue
275 if self._counts_as_already_run(run, eval_config):
276 already_run[eval_config.id][run.task_run_config_id].add(
277 run.dataset_id
278 )libs/core/kiln_ai/adapters/eval/sandbox_worker.pyLines 148-156 148 p.join(timeout=5)
149 raise RuntimeError(f"Code eval scorer timed out after {timeout}s")
150 if p.exitcode not in (0, None):
151 raise RuntimeError(f"Scorer crashed (exit code {p.exitcode})")
! 152 raise RuntimeError("Scorer process exited without returning results")
153 finally:
154 q.close()
155 q.join_thread()Lines 157-166 157 p.join(timeout=5)
158 if p.is_alive():
159 # Result already in hand; a child that won't exit on its own (e.g.
160 # user code left non-daemon threads running) is force-reaped.
! 161 p.kill()
! 162 p.join(timeout=5)
163 elif p.exitcode not in (0, None) and "ok" not in result:
164 raise RuntimeError(f"Scorer crashed (exit code {p.exitcode})")
165
166 return result
|
|
|
||
| # already_run[eval_config_id][run_config_id][dataset_id] | ||
| already_run: Dict[ID_TYPE, Dict[ID_TYPE, Set[ID_TYPE]]] = {} | ||
| superseded: Dict[Tuple[ID_TYPE, ID_TYPE, ID_TYPE], List[EvalRun]] = {} |
There was a problem hiding this comment.
The task-run-eval collector was missing the superseded-tombstone dedup map its sibling collectors already build, so it couldn't recognise runs superseded by a newer config and carry their tombstones onto the job. This adds the same map — keyed by (eval_config, run_config, dataset) — and threads superseded_tombstones onto each EvalJob, bringing this collector to parity with the others.
| eval_task_input = EvalTaskInput.from_eval_input(job.item, run_output) | ||
| result = await evaluator.evaluate(eval_task_input) | ||
|
|
||
| # Like the legacy runner, only successful task-run-eval records of |
There was a problem hiding this comment.
On the single-turn V2 lanes, a full_trace eval scored the conversation but never persisted the trace onto the EvalRun — the V1 validator was exempting V2 records from the trace requirement, so paid generations lost the conversation unrecoverably and the record couldn't be re-scored. This restores parity with the legacy lanes: a successful task-run eval of a full_trace eval serializes and stores the trace. (The same block appears on both the eval_input and dataset lanes.) The gate that let this slip silently is tightened separately in 551107df4.
| another variable) does not. | ||
| """ | ||
| try: | ||
| # Same parse compile_expression performs, wrapped into a template node |
There was a problem hiding this comment.
references_trace decided whether an expression reads the trace variable with a word-boundary regex, so trace appearing as data — a quoted string, a dict key, an attribute of another variable — falsely counted as a trace reference. This uses Jinja's own parser and undeclared-variable analysis, so only a genuine variable reference to trace counts. Invalid expressions return False here; the extraction path owns reporting their syntax errors.
| "stderr": captured_stderr.getvalue(), | ||
| } | ||
| ) | ||
| except SystemExit as e: |
There was a problem hiding this comment.
A sys.exit() in user scorer code raises SystemExit, which the broad except Exception didn't catch — so the child died before queueing a result and the user saw a generic "process exited without returning results" with no clue why. This catches SystemExit and queues an explanatory error naming the exit code. KeyboardInterrupt is left to propagate on purpose: it means the operator interrupted the process group, not that the user's code failed.
| q: multiprocessing.queues.Queue, # type: ignore[type-arg] | ||
| timeout: float, | ||
| ) -> dict[str, Any] | None: | ||
| """Wait for the child's result dict; None on timeout or silent child death. |
There was a problem hiding this comment.
The runner joined the scorer process before reading its result queue. When a scorer returns more than the OS pipe buffer holds (~64KB), the child's queue feeder thread blocks until the parent reads, so the child never exits, the join times out, and a perfectly good run is reported as a timeout — which also stalls the batch behind the global lock. This inverts the wait: poll the queue in short slices (so a silently-dead child is noticed well before the full timeout), then reap the process. See the flag on the wait shape.
| let a child that died without reporting be noticed well before the full | ||
| timeout. | ||
| """ | ||
| deadline = time.monotonic() + timeout |
There was a problem hiding this comment.
Personal review requested: The wait replaces join-first with a queue-first poll in 0.1s slices until the deadline, then force-reaps the process (with a final grace read to catch a result flushed between the empty read and the liveness check). Does the 0.1s slice / force-reap-after-grace shape look right to you, or would you prefer a different poll granularity or a hard join timeout as a backstop?
| except UndefinedError as e: | ||
| # A typo'd template variable and genuinely missing reference data | ||
| # both raise UndefinedError; only the latter is a data problem. | ||
| unknown = _unknown_template_variables(props.prompt_template, namespace) |
There was a problem hiding this comment.
A typo'd variable in the judge prompt template and a genuinely-missing reference-data key both raise Jinja's UndefinedError, and both were labeled missing_reference_key — so a template-authoring bug masqueraded as absent data. This distinguishes them by parsing the template for top-level names that aren't EvalTaskInput fields: unknown names are an authoring error (extraction_failed, naming them), everything else stays a data problem.
| if props.g_eval: | ||
| model_provider = built_in_models_from_provider(provider, model_name) | ||
| if model_provider is not None and not model_provider.supports_logprobs: | ||
| if model_provider is None: |
There was a problem hiding this comment.
The g_eval preflight only rejected models it could confirm lack logprobs. A model absent from the built-in list returned None and slipped through — then the paid judge call failed deterministically downstream, after spending. This fails the preflight loudly when the model can't be verified, before the call, with a message telling the author to pick a built-in logprobs model or turn off G-Eval.
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def validate_tags(self) -> Self: |
There was a problem hiding this comment.
EvalInput.tags were unvalidated, so an empty or space-containing tag could be written. Every tag filter treats those as unselectable, so the item silently disappeared from filtered views with no error at write time. This mirrors the TaskRun tag rules (no empty strings, no spaces) and rejects them at creation where the author can fix it.
| def validate_tags(self) -> Self: | ||
| # Empty or space-containing tags can't be selected by tag filters, so | ||
| # reject them at creation instead of silently dropping the item later. | ||
| for tag in self.tags: |
There was a problem hiding this comment.
Personal review requested: EvalInput.tags are now validated like TaskRun.tags (reject empty and space-containing), which is a deliberate tightening of your schema. Existing data with such tags would fail on the next revalidating write. Confirm you want the TaskRun rules applied to EvalInput, and that a write-time (not load-time) tightening is the right posture.
| class EvalRun(KilnParentedModel): | ||
| """ | ||
| The results of running an eval on a single dataset item. | ||
| The result of running an eval on a single item, stored as a child of the |
There was a problem hiding this comment.
The EvalRun docstring still described a V1-only world, but the branch had already added the eval_input_id source and skipped-item semantics that contradicted it — so the class's own documentation was actively wrong about when task_run_config_id and the two input sources apply. Rewritten to state the real contract: the two eval purposes, the mutually-exclusive dataset_id / eval_input_id sources, and that output/scores may be absent only when skipped.
|
|
||
| @model_validator(mode="after") | ||
| def validate_output_fields(self) -> Self: | ||
| def validate_output_fields(self, info: ValidationInfo) -> Self: |
There was a problem hiding this comment.
Personal review requested: I chose enforcement over exemption for the full_trace trace gate: single-shot no-trace runs are held to it, and an MCP+full_trace pairing that produces no trace now fails at save, matching V1. The argument is V1 parity — V1 already required the trace for exactly this shape. Your call to ratify: is holding V2 single-shot full_trace runs to the trace requirement correct, or is there a legitimate V2 full_trace flow that produces no trace and should be exempt?
| and self.task_run_trace is None | ||
| and not self.loaded_from_file(info) | ||
| ): | ||
| raise ValueError("full_trace task run eval runs should include trace") |
There was a problem hiding this comment.
The old validate_output_fields returned early for every V2 config, which is exactly why the missing-trace bug above could persist silently — the alarm was disconnected for V2. This makes the full_trace-requires-trace gate apply to both V1 and V2 writers of a scored, non-skipped task-run eval, so a writer that drops the trace now fails loudly instead of saving an un-re-scorable record. Historical files predating the gate are exempt via loaded_from_file so they still load. See the flag — enforcement was chosen over exemption here.
| description="This is used to determine the type of eval to run.", | ||
| ) | ||
| properties: V2EvalConfigProperties | dict[str, Any] | None = Field( | ||
| properties: dict[str, Any] | V2EvalConfigProperties | None = Field( |
There was a problem hiding this comment.
Personal review requested: The legacy/V2 properties-parsing contract is now explicit: the union lists dict first so any raw dict stays a plain dict, and V2 configs are parsed into the typed union by a module-level TypeAdapter in the before-validator. This is your data model — please sign off on the mechanism (field-order-as-contract plus TypeAdapter dispatch) rather than relying on discriminated-union fallback ordering.
| # configs store properties as an untyped dict, so we shallow-copy and | ||
| # re-assign it here to force Pydantic to accept the dict branch of the union. | ||
| def dispatch_properties_parsing(cls, data: Any) -> Any: | ||
| # The union lists dict first, so a raw dict always stays a plain dict — |
There was a problem hiding this comment.
The old dispatch validator was a no-op: it shallow-copied the properties dict back onto itself, and legacy dicts only loaded by luck of the union's fallback ordering. A legacy config whose properties happened to carry a colliding "type" key was unloadable — the discriminated union would try to parse it as a typed V2 shape and reject the whole file. This makes the dispatch real: legacy dicts stay plain dicts (the union lists dict first), and V2 configs are parsed into the typed union up front via a module-level TypeAdapter. The field order flip on properties (dict first) is part of the same contract — see the flag.
| raise ValueError( | ||
| f"Invalid code tool ID: {id}. Expected format: 'kiln_tool::code::<code_tool_id>'." | ||
| ) | ||
| # Raises ValueError on malformed IDs; the extracted ID isn't needed here. |
There was a problem hiding this comment.
Dead defensive branch: this guarded if not ct_id: raise, but code_tool_id_from_tool_id already raises on a malformed id, so the extracted value is never falsy by the time the check runs — the branch could never fire. Removed, keeping the raising call for its validation side effect and noting that the return value isn't used here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/core/kiln_ai/adapters/eval/v2_eval_llm_judge.py (1)
157-168: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider moving the g_eval logprobs preflight to
__init__.Raising here is correct behavior. The placement costs repeated work.
evaluate()runs once per eval item, so a misconfigured g_eval judge raises on every item.EvalRunner.run_jobre-raises, andAsyncJobRunnerretries withmax_retries=2, so one config error produces three deterministic failures per dataset item.Both g_eval checks depend only on
props.model_name,props.model_provider, andprops.g_eval. All three are fixed at construction.__init__already validatesmodel_providerthe same way, so the check fits the existing pattern and fails once instead of per item.♻️ Proposed move to the constructor
if self.properties.model_provider not in ModelProviderName.__members__: raise ValueError( f"Invalid model provider: {self.properties.model_provider}" ) + if self.properties.g_eval: + self._assert_logprobs_supported(self.properties) + + `@staticmethod` + def _assert_logprobs_supported(props: LlmJudgeProperties) -> None: + provider = ModelProviderName(props.model_provider) + model_provider = built_in_models_from_provider(provider, props.model_name) + if model_provider is None: + raise ValueError( + f"g_eval=True requires logprobs support, but model " + f"'{props.model_name}' is not a built-in model for provider " + f"'{props.model_provider}', so logprobs support can't be " + f"verified. Use a built-in model that supports logprobs, " + f"or disable G-Eval for this judge." + ) + if not model_provider.supports_logprobs: + raise ValueError( + f"g_eval=True requires logprobs support, but provider " + f"'{props.model_provider}' for model '{props.model_name}' does not " + f"support logprobs" + )Then delete the
if props.g_eval:block fromevaluate(). The tests inlibs/core/kiln_ai/adapters/eval/test_v2_eval_llm_judge.pylines 365-427 would need thebuilt_in_models_from_providerpatch to wrapLlmJudgeEval(cfg)instead ofadapter.evaluate(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/kiln_ai/adapters/eval/v2_eval_llm_judge.py` around lines 157 - 168, Move the g_eval logprobs preflight from evaluate() into the LlmJudgeEval constructor, using the fixed props.model_name, props.model_provider, and props.g_eval values and preserving both built-in-model and supports_logprobs validations. Remove the duplicate if props.g_eval block from evaluate(), and update the affected tests so built_in_models_from_provider is patched while constructing LlmJudgeEval.libs/core/kiln_ai/adapters/eval/eval_utils/v2_eval_helpers.py (1)
29-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Jinja2’s
Environment.parsefor undeclared-variable inspection.
jinja2.parser.Parser,state="variable", andparse_expression()are low-level/internal Jinja APIs, whilemeta.find_undeclared_variablesis intended for the AST fromEnvironment.parse. Usereturn "trace" in meta.find_undeclared_variables(_expression_env.parse("{{ " + expression + " }}"))to remove the internal-API coupling and avoid breaking on a Jinja minor upgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/core/kiln_ai/adapters/eval/eval_utils/v2_eval_helpers.py` around lines 29 - 46, Update references_trace to use _expression_env.parse with the expression wrapped as a Jinja output statement, then pass that template AST directly to meta.find_undeclared_variables. Remove the direct Parser, nodes.Template, and parse_expression usage while preserving the existing invalid-expression handling and trace-reference result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/core/kiln_ai/adapters/eval/eval_runner.py`:
- Around line 659-674: The shared trace-persistence logic around trace_json must
align with EvalRun validation: for successful full-trace fresh-generation
records, treat a missing or empty eval_task_input.trace as missing_trace and
mark the record skipped instead of producing task_run_trace=None. Update the
shared condition used by the single-shot and TaskRun paths, while preserving
serialized trace persistence for non-empty traces and file-loaded history
behavior.
---
Nitpick comments:
In `@libs/core/kiln_ai/adapters/eval/eval_utils/v2_eval_helpers.py`:
- Around line 29-46: Update references_trace to use _expression_env.parse with
the expression wrapped as a Jinja output statement, then pass that template AST
directly to meta.find_undeclared_variables. Remove the direct Parser,
nodes.Template, and parse_expression usage while preserving the existing
invalid-expression handling and trace-reference result.
In `@libs/core/kiln_ai/adapters/eval/v2_eval_llm_judge.py`:
- Around line 157-168: Move the g_eval logprobs preflight from evaluate() into
the LlmJudgeEval constructor, using the fixed props.model_name,
props.model_provider, and props.g_eval values and preserving both built-in-model
and supports_logprobs validations. Remove the duplicate if props.g_eval block
from evaluate(), and update the affected tests so built_in_models_from_provider
is patched while constructing LlmJudgeEval.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b64b200c-e474-48a4-915b-ecdfd9695c20
📒 Files selected for processing (14)
app/web_ui/src/lib/api_schema.d.tslibs/core/kiln_ai/adapters/eval/eval_runner.pylibs/core/kiln_ai/adapters/eval/eval_utils/v2_eval_helpers.pylibs/core/kiln_ai/adapters/eval/sandbox_worker.pylibs/core/kiln_ai/adapters/eval/test_eval_runner.pylibs/core/kiln_ai/adapters/eval/test_sandbox_worker.pylibs/core/kiln_ai/adapters/eval/test_v2_datamodel_contracts.pylibs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.pylibs/core/kiln_ai/adapters/eval/test_v2_eval_llm_judge.pylibs/core/kiln_ai/adapters/eval/v2_eval_llm_judge.pylibs/core/kiln_ai/adapters/eval/v2_eval_tool_call_check.pylibs/core/kiln_ai/datamodel/eval.pylibs/core/kiln_ai/datamodel/test_eval_model.pylibs/core/kiln_ai/datamodel/tool_id.py
| # Like the legacy runner, only successful task-run-eval records of | ||
| # a full_trace eval carry the serialized trace (single-shot | ||
| # generations don't always produce one). | ||
| trace_json: str | None = None | ||
| if ( | ||
| result.skipped_reason is None | ||
| and self.eval.evaluation_data_type == EvalDataType.full_trace | ||
| and eval_task_input.trace | ||
| ): | ||
| trace_json = json.dumps( | ||
| eval_task_input.trace, | ||
| indent=2, | ||
| ensure_ascii=False, | ||
| default=_trace_json_default, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the EvalRun full-trace validator to confirm the exact gate conditions.
set -euo pipefail
fd -t f 'eval.py' libs/core/kiln_ai/datamodel --exec ast-grep outline {} --items all
rg -n -C 20 'full_trace task run eval runs should include trace' libs/core/kiln_ai/datamodel/eval.py
# Show every validator that reads task_run_trace.
rg -n -C 15 'task_run_trace' libs/core/kiln_ai/datamodel/eval.pyRepository: Kiln-AI/Kiln
Length of output: 11417
🏁 Script executed:
#!/bin/bash
# Inspect EvalRun creation paths around eval_runner.py and related test behavior.
set -euo pipefail
wc -l libs/core/kiln_ai/adapters/eval/eval_runner.py libs/core/kiln_ai/datamodel/test_eval_model.py
ast-grep outline libs/core/kiln_ai/adapters/eval/eval_runner.py --view compact | sed -n '1,260p'
sed -n '620,740p' libs/core/kiln_ai/adapters/eval/eval_runner.py
rg -n -C 12 'missing_trace|skipped_reason|make_eval_run|EvalRun\\(' libs/core/kiln_ai/adapters/eval/eval_runner.py
test -f libs/core/kiln_ai/datamodel/test_eval_model.py && rg -n -C 16 'full_trace task run eval runs should include trace|task_run_trace=None|missing_trace' libs/core/kiln_ai/datamodel/test_eval_model.pyRepository: Kiln-AI/Kiln
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '620,740p' libs/core/kiln_ai/adapters/eval/eval_runner.py
rg -n -C 12 'missing_trace|skipped_reason|EvalRun\\(' libs/core/kiln_ai/adapters/eval/eval_runner.py
rg -n -C 16 'full_trace task run eval runs should include trace|task_run_trace=None|missing_trace' libs/core/kiln_ai/datamodel/test_eval_model.py
python3 - <<'PY'
from pathlib import Path
p = Path("libs/core/kiln_ai/adapters/eval/eval_runner.py")
text = p.read_text()
idx_trace = text.find('trace_json: str | None = None')
idx_task_json = text.find('task_json')
idx_eval_run = text.find('EvalRun(')
print("trace_json assignment index:", idx_trace)
print("task_json index:", idx_task_json)
print("EvalRun call index:", idx_eval_run)
print("EvalRun call appears after task_json?", idx_eval_run > idx_task_json)
print("EvalRun call appears after trace_json assignment?", idx_eval_run > idx_trace)
print("includes trace_json in EvalRun call?", "trace_json" in text[idx_trace:idx_eval_run].split("trace_json: str | None", 1)[-1].rsplit("EvalRun(", 1)[1] if idx_trace!=-1 and idx_eval_run!=-1 and idx_task_json!=-1 else "unknown")
PYRepository: Kiln-AI/Kiln
Length of output: 5735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'missing_trace|skipped_reason|EvalRun\(' libs/core/kiln_ai/adapters/eval/eval_runner.py
rg -n -C 16 'full_trace task run eval runs should include trace|task_run_trace=None|missing_trace' libs/core/kiln_ai/datamodel/test_eval_model.py
python3 - <<'PY'
from pathlib import Path
p = Path("libs/core/kiln_ai/adapters/eval/eval_runner.py")
text = p.read_text()
idx_trace = text.find('trace_json: str | None = None')
idx_task_json = text.find('task_json')
idx_eval_run = text.find('EvalRun(')
trace_block = text[idx_trace:idx_task_json if idx_task_json != -1 else idx_eval_run]
print("trace_json block:")
print(trace_block)
print("EvalRun task_run_trace argument in EvalRun call:", "task_run_trace=trace_json" in text[idx_eval_run:text.find(")", idx_eval_run)])
PYRepository: Kiln-AI/Kiln
Length of output: 28227
🌐 Web query:
pydantic BaseModel truthiness truthy dict list empty None truthiness __bool__
💡 Result:
In Python, Pydantic BaseModel instances are always truthy by default [1]. This is because they do not implement bool or len [1]. 1. Truthiness of BaseModel: Because Pydantic BaseModel does not define a custom bool or len method, an instance of a BaseModel will evaluate to True in a boolean context, regardless of whether the model's fields are empty, None, or contain data [1]. 2. Truthiness of Fields: The truthiness of individual fields within the model follows standard Python rules [2]. For example, a list field will be falsy if it is empty ([]) and truthy if it contains elements [3][2]. A None value is always falsy [3]. 3. Customizing Truthiness: If you require a Pydantic model to evaluate to False when certain conditions are met (e.g., when all fields are empty or None), you must explicitly implement the bool magic method in your model class [1]. Example of custom truthiness: class MyModel(BaseModel): data: list[str] = [] def bool(self) -> bool: # Evaluate to False if 'data' is empty return bool(self.data) If you are dealing with empty data structures or None values and want to treat them as "missing" or "empty" during validation or business logic, it is recommended to use Pydantic validators (e.g., @field_validator) or Annotated types with constraints (e.g., min_length) rather than relying on the truthiness of the model instance itself [4][2][5].
Citations:
- 1: https://techqa.club/v/q/truth-value-of-empty-user-defined-data-objects-78112247
- 2: https://stackoverflow.com/questions/61468548/check-if-list-is-not-empty-with-pydantic-in-an-elegant-way
- 3: https://pyguides.dev/reference/dunder-methods/dunder-bool/
- 4: Empty objects should be validated as None for optional fields pydantic/pydantic#3295
- 5: Pydantic model field: convert empty string to None pydantic/pydantic#2687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
source = Path("libs/core/kiln_ai/adapters/eval/eval_runner.py").read_text()
tree = ast.parse(source)
def find_eval_task_input_declare_assign(node):
for n in ast.walk(node):
if isinstance(n, ast.Assign):
for target in n.targets:
if isinstance(target, ast.Name) and target.id == "eval_task_input":
value = n.value
else:
continue
if isinstance(value, ast.Call) and isinstance(value.func, ast.Attribute):
attr = value.func.attr
# inspect the condition on result.skipped_reason / full_trace / eval_task_input.trace
# We'll locate relevant nodes and print source slices.
print("AST check: look for EvalTaskInput.from_task_run calls with if result.skipped_reason is None and ...", end=" ")
hits = []
for n in ast.walk(ast.parse(source)):
if isinstance(n, ast.If):
s = ast.get_source_segment(source, n.test) or ""
if "result.skipped_reason is None" in s and "EvaluationDataType.full_trace" in s or "EvalDataType.full_trace" in s:
hits.append((n.lineno, n.col_offset, ast.get_source_segment(source, n)))
print(len(hits))
for lineno, col, s in hits:
print(f"\nline {lineno}: {s.replace(chr(10), ' ')}")
PY
# Also inspect relevant trace construction in the fresh multi-turn synthetic path and trace data shape.
sed -n '840,910p' libs/core/kiln_ai/adapters/eval/eval_runner.py
rg -n -C 8 'class EvalTaskInput|from_task_run|from_eval_input' libs/core/kiln_ai/datamodel/eval.pyRepository: Kiln-AI/Kiln
Length of output: 6967
Align full-trace trace-persistence with the EvalRun validator.
EvalRun rejects successful full-trace task-run records with task_run_trace=None, except for file-loaded history. The single-shot and TaskRun fresh-generation paths set trace_json only when eval_task_input.trace is truthy, so an empty-trace successful V2 run is rejected. Update the shared condition to persist a missing_trace skip when needed instead of relying on separate per-lane logic.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 667-672: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
eval_task_input.trace,
indent=2,
ensure_ascii=False,
default=_trace_json_default,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/core/kiln_ai/adapters/eval/eval_runner.py` around lines 659 - 674, The
shared trace-persistence logic around trace_json must align with EvalRun
validation: for successful full-trace fresh-generation records, treat a missing
or empty eval_task_input.trace as missing_trace and mark the record skipped
instead of producing task_run_trace=None. Update the shared condition used by
the single-shot and TaskRun paths, while preserving serialized trace persistence
for non-empty traces and file-loaded history behavior.
|
Superseded. The branch is now split into seven review-only views with the current tip; this area is inside them. Closing so there is one set to review. |
1 participant
Surface: the V2 eval data model and the eval adapters (sandbox scorer runner, LLM judge, runner). Pure Python; about half the diff is tests. Three commits: d625c94 (landed as c22bb47) — make the EvalConfig properties dispatch real, validate EvalInput tags, rewrite the EvalRun docstring, drop a dead tool-id branch; fdeda16 (landed as 1a5f81c) — drain the scorer queue before joining so large scorer output isn't misread as a timeout, handle SystemExit from user scorers, restore single-turn full_trace trace persistence, fail the g_eval logprobs preflight loudly, classify template-authoring errors correctly, parser-based trace-reference detection, superseded-tombstone dedup parity; 551107d (landed as 1e19a18) — make the EvalRun full_trace trace gate V2-aware. Four Personal review requested comments flag the contract/invariant calls.
Note: several of these fixes dedupe when #1454 syncs into the branch — they touch the same datamodel/adapter regions, so expect to resolve to this behavior and adopt any copy updates from that PR.
🤖 Generated with Claude Code
CI note: the "Check API Schema Bindings" failure here is an artifact of this review PR's pinned snapshot — the canonical schema verification lives on
dchiang/eb-v2-merge. No action needed from reviewers.