feat(evaluations): add client-side evaluation runner - #61
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if self._sdk_key: | ||
| client = await self._resolve_client() |
There was a problem hiding this comment.
🔴 Keyless evaluation runs always stall
Without an SDK key, run executes every row but sends no results. Summary polling then times out after the provider work completes.
Prompt for agents
EvaluationsModule.run creates an API-source run with a nonzero rowCount, but the only result-delivery path is the LaunchDarkly SDK custom event transport. When sdk_key is absent, it still invokes every handler and then polls a summary that cannot account for those rows. Either require an SDK key before creating records or invoking handlers, or add a supported result-ingestion path for keyless runs. Update the public initialization contract and tests to match the chosen behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| f"{_segment(evaluation.id)}/runs/{_segment(evaluation_run.id)}" | ||
| ) | ||
| return EvalRunResult( | ||
| passed=(summary.error_rows == 0 and summary.pending_rows == 0), |
There was a problem hiding this comment.
🔴 Failed runs report success
A terminal FAILED, ERROR, or canceled summary with zeroed counts produces passed=True. CI consumers then accept an unsuccessful evaluation.
Prompt for agents
EvaluationsModule._is_terminal_summary treats FAILED, ERROR, CANCELED, and CANCELLED as terminal, but EvaluationsModule.run derives passed only from error_rows and pending_rows. Terminal failure summaries can omit counts or return zeros, making the result pass. Derive success from both the terminal state and settled row counts, or raise EvaluationsError for unsuccessful terminal states. Preserve the intentionally documented treatment of failed_rows if that distinction is part of the evaluations contract, and add tests for each failure terminal state.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing duplicate PR opened from validation lane; the same commits have been pushed to the existing target PR #39. |
1 participant
Intent
Update existing PR #39 on its existing head branch devin/1786604824-evaluations-scaffold to fix evaluation summary polling against the real backend. The backend may omit run state, so summary polling should still complete when row counts prove processing is complete: total rows is greater than zero, pending rows is zero, and passed+failed+error rows account for total rows. Preserve protection against empty or malformed summaries: total_rows=0 or missing state with no meaningful row accounting must not be treated as complete. Add focused regression coverage for the real backend shape with no state, total_rows=10, pending_rows=0, and rows accounted by passed/failed/error terminating, while keeping existing coverage for missing-state empty or unaccounted summaries. Validate focused tests and deliver by updating PR 39 rather than opening a separate PR.
What Changed
launchdarkly_ai_server.evaluationsmodule (api,module,runner,types), a client-side evaluation runner that lists datasets, resolves handlers and tools, drives generation against provider handlers with a concurrency-controlled worker pool, and emits$ld:ai:offline-evals:generationevents for each row (including error payloads).stateor, when the backend omits state, row-count accounting (total_rows > 0,pending_rows == 0,passed + failed + error == total_rows); empty or unaccounted summaries remain pending soEvalRunResult.passedis only derived from a settled summary.EvaluationsModulefromlaunchdarkly_ai_server.__init__, refreshedAGENTS.md,CLAUDE.md, and thepackages/ai+packages/clientREADMEs/agents.mdfor the new API, and added focused coverage intests/test_evaluations.pyandtests/test_evaluations_run.pyfor the state-omission terminal case and for empty/unaccounted summaries staying pending.Risk Assessment
✅ Low: Small, well-bounded fix that adds a state-omitted terminal branch guarded by three conjuncts matching the intent verbatim; existing missing-state/unaccounted coverage remains valid and a focused regression test asserts the new path.
Testing
Ran the four focused summary-polling tests inpackages/client/tests/test_evaluations_run.py— all pass on the target commit. Confirmed the new regression test truly reproduces the bug by temporarily revertingmodule.pytodcb08f6; the new test failed there (poll loop kept issuing/summaryGETs) and passed again once the fix was restored, exercising both new behavior (state-less + accounted rows terminates) and preserved protections (missing state with no accounting keeps polling; timeout still fires with pending_rows>0). No transient artifacts left in the worktree.Evidence: Focused summary-polling test transcript (post-fix, 4 passed)
packages/client/tests/test_evaluations_run.py::test_summary_is_polled_until_terminal_state PASSED [ 25%] packages/client/tests/test_evaluations_run.py::test_summary_polling_completes_without_state_when_all_rows_are_accounted PASSED [ 50%] packages/client/tests/test_evaluations_run.py::test_summary_polling_ignores_missing_state_even_when_pending_is_zero PASSED [ 75%] packages/client/tests/test_evaluations_run.py::test_summary_polling_times_out_waiting_for_terminal_state PASSED [100%] == 4 passed, 7 deselected in 0.10s ===Evidence: Regression reproduction (pre-fix module.py fails the new test)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
uv run pytest packages/client/tests/test_evaluations_run.py -k 'test_summary_polling_completes_without_state_when_all_rows_are_accounted or test_summary_polling_ignores_missing_state_even_when_pending_is_zero or test_summary_is_polled_until_terminal_state or test_summary_polling_times_out_waiting_for_terminal_state' -v— 4 passedRegression reproduction: revertedpackages/client/src/launchdarkly_ai_server/evaluations/module.pyto commitdcb08f6(pre-fix_is_terminal_summarythat returned False when state is None) and re-ran the new test; it failed withAssertionError: unexpected request: GET .../summary, proving the fix is load-bearing. Restored the file to the target commit and reconfirmed the clean worktree withgit status --short.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Note
Overview
Adds
init_evaluationsandEvaluationsModule.run()so apps can run generation-only offline evals in-process: resolve datasets and tools via the management API (LD_API_TOKEN, separateLD_API_BASE_URI/LD_UI_BASE_URI), create evaluation + API-source run, invoke a provider handler per row (no handler retries), optionally emit$ld:ai:offline-evals:generationevents through the SDK, flush, then poll/summaryuntil the run is settled.Summary polling now finishes when the API returns a terminal
state, or—whenstateis omitted—whentotal_rows > 0,pending_rows == 0, and passed + failed + error rows equaltotal_rows; empty or unaccounted summaries keep polling (3-minute timeout).EvalRunResult.passedis true only when error and pending counts are both zero (failed assertion rows do not fail the harness result).Public exports, README/agent docs,
CLAUDE.md→AGENTS.md, and tests cover API client retry rules, full run orchestration, and the stateless summary regression cases.Reviewed by Cursor Bugbot for commit 4c79090. Bugbot is set up for automated code reviews on this repo. Configure here.