-
Notifications
You must be signed in to change notification settings - Fork 0
feat(AIC-3363): Support inline datasets #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,7 +131,9 @@ Handlers may return any of these — the client normalizes them before emitting | |
|
|
||
| `init_evaluations()` creates an evaluations harness using `LD_API_TOKEN` and the management API host `LD_API_BASE_URI`. Do not reuse `LD_BASE_URI`: that variable configures SDK delivery and may point at a relay proxy. Evaluation-run links use the separate `ui_base_uri` option, then `LD_UI_BASE_URI`, then `https://app.launchdarkly.com`; do not derive their host from `LD_API_BASE_URI`. An event transport is resolved in `init_evaluations()`, which raises before any network I/O when it finds neither an SDK key (`sdk_key` or `LD_SDK_KEY`) nor an already-initialized event-capable client: generation events are the only ingest path for row results, so a run without a transport could never complete. The lifecycle module's bring-your-own-client path (`init_client(client=...)`) therefore satisfies the check on its own, and `run()` reuses that singleton through `_resolve_client`; `run()` raises if the client disappears before it emits. Both polling arguments reject NaN, which would otherwise never compare past a deadline and hang the run. The harness always queues one `$ld:ai:offline-evals:generation` custom event per row through the standard SDK event transport and flushes before returning. No feature flag gates event emission. The harness polls the run summary endpoint until a nonzero `total_rows` has `pending_rows == 0` and `passed + failed + error` rows accounting for the total, polling every `poll_interval_seconds` (default 2s) until `poll_timeout_seconds` (default 180s); both are `run()` arguments so large datasets can widen them. The summary endpoint does not return run state, so `RunSummary` exposes row counts only. | ||
|
|
||
| `await EvaluationsModule.run(...)` takes `project_key` per call. Dataset lookup/row pagination, evaluation creation, and run creation are private helpers; only `run()` is public. Each call creates a new evaluation with `POST` and a run with `source="api"`, so its key must be unique. The harness directly invokes the supplied handler once per row and never retries it — event delivery is never a reason to rerun a handler because that would repeat tool side effects; retries apply only to management API requests. A 429 is replayed for any method, but 5xx responses and transport failures are replayed only for `GET`/`HEAD`, so an evaluation or run `POST` that the server may already have applied is never duplicated. Management API calls run in a worker thread (`asyncio.to_thread`) because the client is synchronous; the caller's event loop stays free. Generation events go through the already-initialized SDK client when the application has one — `init_client` is idempotent, so an existing singleton wins and the evaluations SDK key is ignored with a warning. Dataset-owned `input`, `expected_output`, `metadata`, and `variables` are deliberately excluded from the event payload. The harness flushes events, polls the run summary endpoint until row accounting is complete (`total_rows > 0`, `pending_rows == 0`, and `passed + failed + error == total_rows`), and raises a timeout once `poll_timeout_seconds` elapses if the backend never reaches one. `RunSummary` includes row counts only, and `EvalRunResult.passed` is true only when error and pending row counts are both zero. | ||
| `await EvaluationsModule.run(...)` takes `project_key` per call, and exactly one of `dataset` (an LD-hosted dataset key) or `rows` (rows supplied from code). Dataset lookup/row pagination, evaluation creation, and run creation are private helpers; only `run()` is public. Each call creates a new evaluation with `POST` and a run with `source="api"`, so its key must be unique. The harness directly invokes the supplied handler once per row and never retries it — event delivery is never a reason to rerun a handler because that would repeat tool side effects; retries apply only to management API requests. A 429 is replayed for any method, but 5xx responses and transport failures are replayed only for `GET`/`HEAD`, so an evaluation or run `POST` that the server may already have applied is never duplicated. Management API calls run in a worker thread (`asyncio.to_thread`) because the client is synchronous; the caller's event loop stays free. Generation events go through the already-initialized SDK client when the application has one — `init_client` is idempotent, so an existing singleton wins and the evaluations SDK key is ignored with a warning. | ||
|
|
||
| Which row source a run uses changes exactly three things. A `rows=` run reads no dataset, so it issues neither dataset GET; its run-creation body is exactly `{"source": "api"}` with `datasetId` omitted rather than nulled; and `datasetId`/`datasetKey` drop off both event payloads, which shortens the generation event's identity set from six fields to five and therefore changes its `eventId` (ingest keys such a row off `(run, rowIndex)` alone and never reads `eventId`). Because LaunchDarkly holds no copy of an inline row, the generation event carries its `input`, `expectedOutput`, `variables`, and `metadata` — the one case where those are not excluded. For a `dataset=` run they stay excluded because the dataset owns them, and the criterion event excludes them in both modes. Rendering and the `input`/`expected_output` variable injection go through one shared helper (`_render_row`) for both sources, with `_row_from_api_item` coercing bad server data and `_validate_rows` rejecting bad caller data outright; keep that split rather than unifying it. The `MAX_ROWS` and `MAX_INLINE_TEXT_BYTES` ceilings are inline-only for the same reason: an inline row is only bounded because it travels inside its own generation event, whereas a hosted dataset is LaunchDarkly's to bound and a caller could not shrink one from their process — enforcing the caps there would fail a run over data the caller cannot reach. `MAX_INLINE_TEXT_BYTES` is measured on the UTF-8 encoding of all fields individually, not on `len()` and not on the row as a whole, and on the *rendered* row rather than the caller's: expanding a `{{...}}` placeholder can grow `input` or `expected_output` past the cap, and the injected `input`/`expected_output` keys always grow `variables`, so a row measured before rendering can pass the cap and still produce an event that ingest rejects on the SDK's background flush thread, where nothing can report it and the run only shows up as a polling timeout. That is why `MAX_ROWS` and the shape checks sit in `_validate_rows` while the byte cap sits in `_prepare_inline_rows`, which renders first; `run()` calls it ahead of all I/O so both still report with zero requests issued, and it is the single render per run — the rows it returns are what `_run_rows` and the generation events both use. The harness flushes events, polls the run summary endpoint until row accounting is complete (`total_rows > 0`, `pending_rows == 0`, and `passed + failed + error == total_rows`), and raises a timeout once `poll_timeout_seconds` elapses if the backend never reaches one. `RunSummary` includes row counts only, and `EvalRunResult.passed` is true only when error and pending row counts are both zero. | ||
|
|
||
| --- | ||
|
|
||
|
|
@@ -250,6 +252,14 @@ When `enabled` is `False`, `config` is always `None`. When `enabled` is `True` b | |
|
|
||
| `execute_and_track` expects the handler to return a plain `dict` with at least `output` and `usage` keys. Do not return a custom class — `parse_usage` and the telemetry pipeline both access dict keys. | ||
|
|
||
| ### 3. Letting an unserializable value into an evaluation event payload | ||
|
|
||
| An SDK event buffer is drained and serialized on a background thread, so a value the JSON encoder cannot encode is not reported back to the caller — the events are simply lost, and the run ends in a polling timeout with nothing to explain it. This is only reachable through `run(rows=[...])`, where `variables`/`metadata` hold arbitrary caller objects, which is why `_validate_rows` serialization-checks them up front with `allow_nan=False` and refuses to coerce. Do not relax that into a `default=str` rescue: silently stringifying a caller's value changes what a judge renders and what LaunchDarkly stores, and is unrecoverable once the row is persisted. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment here is a good example on why validation before event ingestion makes so much sense. |
||
|
|
||
| ### 4. Rendering an evaluation row in place | ||
|
|
||
| `DatasetRow` is a mutable dataclass and, for `run(rows=[...])`, the instances belong to the caller. Rendering writes the injected `input`/`expected_output` keys into `variables`, so doing it in place would make a second run over the same list resolve `{{input}}` against the first run's already-rendered value — a CI retry silently evaluating different data. `_normalize_inline_rows` builds fresh rows with fresh variable maps; keep it that way. The `dict()` copy there is also load-bearing for a second reason: `parse_template` resolves placeholders via `isinstance(value, dict)`, so any other `Mapping` would leave every placeholder literal and send raw mustache text to the model. | ||
|
|
||
| --- | ||
|
|
||
| ## Adding a New Export | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary to have the user specify the
row_index. This can be inferred by the index in the array.