[AIC-3363] Add support for evaluation data to be defined in code - #127
aknight-ld wants to merge 1 commit into
Conversation
jeffdupont
left a comment
There was a problem hiding this comment.
Reviewed with the 1.0 freeze in mind. Inline rows are a useful addition, and since evals are additive and Python-only for 1.0 (the GA plan defers JS evals to 1.1), the lack of a JS counterpart isn't a blocker. JS has no eval code on main to add this to. make test passes at 196d0b1 (1418 passed, 11 skipped, exit 0) and make typecheck is clean.
Three things I'd like settled before merge:
- No spec, and the PR goes against the one on
main.TESTING.md§8.5 says the run create body is "exactly{source: \"api\", datasetId: <dataset id>}", and §8.6 listsdatasetIdas a run identity field on every event that an event type "must never drop". This PR drops it from the run body, the identity set, theeventIdhash and the payloads, and adds a new request (POST …/runs/{id}/dataset-rows). The spec for the earlier approach, launchdarkly/ai-sdks-monorepo#15, was closed with #95, and I didn't find a replacement. Event payloads are a data contract with ingest, and JS will build against the spec in 1.1. So I think this needs a spec PR covering the upload request, batch size, retry rule, event identity without a dataset, and row validation before it merges. - A bad row can still fail after the records exist. The README says "a malformed row fails the run before any records are created", but values inside
variables/metadataare never checked for JSON. I reproduced both cases at 196d0b1. Adatetimeinvariablesraises a bareTypeErrorfromjson.dumps, afterPOST evaluationsandPOST runshave both gone out. Amath.naninmetadatagoes out as"metadata": {"score": NaN}, which isn't valid JSON. §8.4 already requires this check, done up front, for inline tool schemas, for the same reason. #95 had it. - A failed upload leaves a run that never finishes. The upload happens after the run is created, one batch at a time. With 600 rows and a 400 on the second batch (reproduced), rows 0-499 are stored, the call raises, and the run stays open with a placeholder row count, so nothing will ever complete it. Should the SDK mark the run failed, or should the server time it out? Whichever it is should go in the spec. Retrying the batch POST on 5xx relies on the server applying each
rowIdxat most once. I haven't checked that in the server, and the spec should say it too, because §8.3 currently says POSTs are never retried on 5xx.
Smaller notes:
InlineDatasetRowis a new root export. It's only a type alias (DatasetRow | Mapping[str, Any]), so callers don't need it to userows=. If evals move toexperimental.evaluationsas the lifecycle draft proposes, it should go with them. Either way I'd keep it out of the root__all__.- Passing a
DatasetRowmeans writingrow_index=by hand, and it has to equal the row's position or the call raises.DatasetRowis also documented as "a rendered dataset row" for the handler. Reusing it as the input type freezes both roles together. A mapping-only input, orrow_indexmade optional for inline rows, would be easier to change later. - #95 also capped the row count (10k) and the rendered byte size. Does the server enforce limits on
dataset-rows? If it does, checking them up front avoids the partial-upload case above. I haven't checked the server. - The
DatasetRefdocstring says "the server drops any that name one as a mismatch". I haven't verified that. If it's true, it belongs in the spec, because it's the reason events can't carry a dataset id.
| raise EvaluationsError( | ||
| f"Inline dataset row {position} {field_name} must be a string" | ||
| ) | ||
| for field_name in ("variables", "metadata"): |
There was a problem hiding this comment.
This checks that variables / metadata are mappings, but not what's inside them. Reproduced at 196d0b1: variables={"when": datetime(2026, 1, 1)} raises a bare TypeError from json.dumps in api.py:154, after POST evaluations and POST runs. metadata={"score": math.nan} is sent as NaN, which isn't valid JSON. A strict json.dumps(..., allow_nan=False) per row here would keep the README's "fails before any records are created" promise true. §8.4 asks for the same check on inline tool schemas.
| # placeholder row count of 1, so a result counted before the rows | ||
| # land would mark the run complete. | ||
| await asyncio.to_thread( | ||
| self._runner._upload_dataset_rows, |
There was a problem hiding this comment.
If any batch fails here, the run already exists and has some rows. Reproduced with 600 rows and a 400 on the second batch: 500 rows stored, EvaluationsError raised, and nothing ever completes the run. Should this mark the run failed before re-raising, or does the server expire runs that never get events? Either way the spec should say which.
| "datasetId": dataset_id, | ||
| } | ||
| body: dict[str, Any] = {"source": "api"} | ||
| if dataset_id is not None: |
There was a problem hiding this comment.
TESTING.md §8.5 on main says this body is "exactly {source: \"api\", datasetId: <dataset id>}". The inline-dataset spec, ai-sdks-monorepo#15, was closed along with #95, so nothing describes the no-dataset run, the dataset-rows request or its batch size. That needs a spec PR before this freezes, so JS has something to build against in 1.1.
| ] | ||
| } | ||
| try: | ||
| self._api.post(path, body=body, idempotent=True) |
There was a problem hiding this comment.
Retrying this POST on 5xx and on timeouts is safe only if the server applies each rowIdx at most once and a replay doesn't add rows or fail with a conflict. I haven't checked that in the server. §8.3 says POSTs are never retried on 5xx, so idempotent=True needs a spec line saying which endpoints it covers.
| "rowIndex": result["row_index"], | ||
| } | ||
| if dataset.id is None: | ||
| del identity["datasetId"] |
There was a problem hiding this comment.
This changes the event identity set, and so the eventId hash, for inline runs. §8.6 says an event type may extend the identity set but "must never drop a field from it". I think the change is reasonable if ingest really rejects a dataset id it doesn't know, but it's a data contract change and should be in the spec. The same applies to the criterion identity at line 1114 and to datasetKey at 807.
| "EvaluationsError", | ||
| "EvaluationsModule", | ||
| "GenerationConfig", | ||
| "InlineDatasetRow", |
There was a problem hiding this comment.
This is a new root name for a type alias that callers don't need, since rows= accepts a DatasetRow or a plain dict either way. The lifecycle draft moves evals to launchdarkly_ai_server.experimental.evaluations. I'd leave it in launchdarkly_ai_server.evaluations only and keep it out of the root __all__, so the 1.0 surface trim doesn't have to remove it.
Adjusted approach for inline dataset definition. For users who wish to define their evaluation data directly from code, we now offer an alternate path where they provide the data directly by dropping the
datasetkey and instead providingrows.Sample shape:
Note
Overview
Adds code-defined evaluation datasets via a new
rowsargument onevals.run(), mutually exclusive with the existingdatasetkey. Rows can beDatasetRowinstances or wire-shaped mappings (input,expectedOutput,variables,metadata); list position defines row index, with validation failing before any LaunchDarkly records are created.For inline runs, the harness creates a run without a hosted
datasetId, uploads unrendered rows to…/runs/{id}/dataset-rowsin batches of up to 500 (before generation/events, to avoid premature run completion), then renders{{…}}templates the same way as hosted datasets via sharedrender_row. Generation and criterion events omitdatasetId/datasetKeywhen no stored dataset is attached.The management API client gains an
idempotentPOST option so dataset-row uploads retry safely on 5xx or transport errors.InlineDatasetRowis exported; README documents the inline path. Hosted-dataset behavior and event identities are covered by new tests.Reviewed by Cursor Bugbot for commit 196d0b1. Bugbot is set up for automated code reviews on this repo. Configure here.