Skip to content
Draft
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
3 changes: 2 additions & 1 deletion tests_end_to_end/coverage/taxonomy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,13 @@ areas:
specs:
- experiments/experiments-smoke.spec.ts
- experiments/experiments-compare.spec.ts
- experiments/experiments-compare-json-sort.spec.ts
- experiments/experiment-logs-date-window.spec.ts
capabilities:
list-experiments: { covered: true, tier: t1-smoke }
per-item-scores: { covered: true, tier: t1-smoke }
compare-side-by-side: { covered: true, tier: t2-cuj }
compare-sort-search: { covered: true, tier: t2-cuj }
compare-sort-search: { covered: true, tier: t2-cuj, note: "feedback-score sort + search in experiments-compare; evaluation-task JSON-key sort in experiments-compare-json-sort" }
compare-feedback-tab: { covered: true, tier: t2-cuj }
compare-row-detail: { covered: true, tier: t2-cuj }
insights-tab: { covered: false }
Expand Down
66 changes: 66 additions & 0 deletions tests_end_to_end/e2e/core/backend/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,33 @@ export interface DatasetItemRef {
data: Record<string, unknown>;
}

/** A sort clause as the experiments-comparison grid sends it. */
export interface CompareSort {
/** e.g. `output.answer`, `feedback_scores.accuracy`, `id`. */
field: string;
direction: 'ASC' | 'DESC';
}

/**
* One row of the experiments-comparison grid: a dataset item plus each compared
* experiment's evaluation-task output.
*/
export interface ComparedDatasetItemRef {
id: string;
/**
* Evaluation-task output JSON keyed by experiment id, or null where the
* experiment recorded no output for this item — which is a real answer, not
* an empty one.
*/
outputByExperimentId: Record<string, Record<string, unknown> | null>;
Comment on lines +47 to +54

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared-row output contract drops valid JSON shapes

DatasetItemResultMapper.getJsonNodeOrNull() leaves ExperimentItem.output as a broad JsonNode, and listComparedDatasetItems() only asserts it to the narrower outputByExperimentId type, so valid scalar and array responses pass through with an unrepresentable runtime shape — should we type this field as JsonListStringCompare | null (or an equivalent local union) and preserve undefined as null?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/core/backend/client.ts` around lines 47-54 and 445-451, update
`ComparedDatasetItemRef.outputByExperimentId` and the `listComparedDatasetItems` mapping
so they accurately represent the backend’s `JsonListString` values. Replace the
object-only type assertion with `JsonListStringCompare | null` or an equivalent local
union that includes objects, object arrays, and strings, while continuing to convert
`undefined` outputs to `null`.

}

export interface ComparedDatasetItemsPage {
/** Null when the server omitted the count entirely. */
total: number | null;
items: ComparedDatasetItemRef[];
}

/** One row of the dataset's Version history tab. */
export interface DatasetVersionRef {
versionName: string;
Expand Down Expand Up @@ -388,6 +415,45 @@ export function makeBackendClient(apiKey: string | null = null) {
}
},

/**
* The server read behind the experiments-comparison grid: dataset items
* joined with each compared experiment's last trial, in the order the
* server sorted them.
*
* `sorting` is the same JSON the grid's sort header writes. For an
* `output.<key>` / `input.<key>` / `metadata.<key>` field the backend has
* to bind `<key>` as a query parameter rather than interpolate it into the
* ClickHouse `JSONExtractRaw` call (OPIK-8023), so this read is what tells
* a correctly ordered grid apart from one the browser happened to render
* in a plausible order.
*/
async listComparedDatasetItems(args: {
datasetId: string;
experimentIds: string[];
sorting?: CompareSort[];
}): Promise<ComparedDatasetItemsPage> {
const page = await opik.api.datasets.findDatasetItemsWithExperimentItems(args.datasetId, {
experimentIds: JSON.stringify(args.experimentIds),
size: 100,
...(args.sorting ? { sorting: JSON.stringify(args.sorting) } : {}),
});
return {
// Left null rather than defaulted to 0: a caller checking that a sorted
// read returned the whole collection has to be able to tell "the server
// said none" from "the server said nothing".
total: page.total ?? null,
items: (page.content ?? []).map((item) => ({
id: String(item.id ?? ''),
outputByExperimentId: Object.fromEntries(
(item.experimentItems ?? []).map((experimentItem) => [
String(experimentItem.experimentId),
(experimentItem.output as Record<string, unknown> | undefined) ?? null,
]),
),
})),
};
},

/**
* Stats for the projects whose name matches `name`, optionally scoped to a
* time window — the exact call the v2 Projects table makes to fill its
Expand Down
3 changes: 3 additions & 0 deletions tests_end_to_end/e2e/core/backend/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ export {
type DatasetRef as BackendDatasetRef,
type DatasetItemRef,
type DatasetVersionRef,
type CompareSort,
type ComparedDatasetItemRef,
type ComparedDatasetItemsPage,
type ProjectStatsRef,
type ExperimentRefDetail,
type TestSuiteRef as BackendTestSuiteRef,
Expand Down
14 changes: 13 additions & 1 deletion tests_end_to_end/e2e/fixtures/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export { test, expect } from './evaluated-thread.fixture';
export { test, expect } from './json-sortable-comparison.fixture';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared test entrypoint gains feature coupling

fixtures/index.ts now re-exports test from json-sortable-comparison.fixture, so every generic test import transitively depends on comparison-specific fixture setup — should we source the shared export from the generic fixture base and expose the specialized test through a dedicated module?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/fixtures/index.ts` around lines 1-1, change the shared `test` and
`expect` export so it comes from the generic `evaluated-thread.fixture` rather than the
JSON-sortable comparison fixture. Keep the JSON-sortable types and constants exported as
needed, but expose that fixture’s specialized `test` through its own narrowly consumed
module or import path so generic consumers do not depend on comparison-specific setup.

export type { ProjectFixtures } from './project.fixture';
export type { ScratchDir, ScratchDirFixtures } from './scratch-dir.fixture';
export type {
Expand Down Expand Up @@ -65,4 +65,16 @@ export type {
ThreadEvaluationRunRef,
EvaluatedThreadFixtures,
} from './evaluated-thread.fixture';
export type {
JsonSortableComparisonRef,
JsonSortableComparisonFixtures,
JsonSortableItemSeed,
JsonSortableExperimentRef,
} from './json-sortable-comparison.fixture';
export {
PLAIN_OUTPUT_KEY,
QUOTED_OUTPUT_KEY,
BACKSLASHED_OUTPUT_KEY,
JSON_SORTABLE_OUTPUT_KEYS,
} from './json-sortable-comparison.fixture';
export type { ProjectRef } from '../core/backend';
221 changes: 221 additions & 0 deletions tests_end_to_end/e2e/fixtures/json-sortable-comparison.fixture.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
import { test as baseTest } from './evaluated-thread.fixture';
import { shouldLeaveArtifacts } from '../core/artifacts';
import { uuid7 } from '../core/backend';

/** The plain JSON key every evaluation-task output carries. */
export const PLAIN_OUTPUT_KEY = 'output';
/** A key carrying a single quote — the character that used to break the SQL. */
export const QUOTED_OUTPUT_KEY = "a'b";
/** A key carrying a backslash — the other character an interpolated key mangles. */
export const BACKSLASHED_OUTPUT_KEY = 'c\\d';

export const JSON_SORTABLE_OUTPUT_KEYS = [
PLAIN_OUTPUT_KEY,
QUOTED_OUTPUT_KEY,
BACKSLASHED_OUTPUT_KEY,
] as const;

export interface JsonSortableItemSeed {
input: string;
expected_output: string;
/** The evaluation-task output JSON, written identically to both experiments. */
output: Record<string, string>;
}

export interface JsonSortableExperimentRef {
experimentId: string;
experimentName: string;
}

export interface JsonSortableComparisonRef {
datasetId: string;
datasetName: string;
projectName: string;
items: JsonSortableItemSeed[];
/** Dataset item ids, aligned by index with `items`. */
itemIds: string[];
/** Evaluation-task output JSON keyed by dataset item id. */
outputByItemId: Record<string, Record<string, string>>;
/**
* Item ids in the order an ascending sort on that output key must produce,
* derived from the seed rather than hand-written so the expectation cannot
* drift away from the values actually written.
*/
ascOrderByKey: Record<string, string[]>;
experiments: JsonSortableExperimentRef[];
}

export interface JsonSortableComparisonFixtures {
jsonSortableComparison: JsonSortableComparisonRef;
}

/**
* Four shared dataset items, two experiments, and three JSON keys per
* evaluation-task output. The values are chosen so each key induces a
* *different* ordering, and none of them is the order the items were created
* in:
*
* item output a'b c\d asc by key
* q1 delta kilo romeo output -> q2 q4 q3 q1
* q2 alpha zulu papa a'b -> q3 q1 q4 q2
* q3 charlie hotel tango c\d -> q2 q4 q1 q3
* q4 bravo yankee quebec
*
* That is what stops a sort which silently fell back to item id, insertion
* order or the wrong JSON key from passing by luck.
*
* Both experiments write the SAME output for a given item on purpose: the
* comparison DAO picks one experiment's last trial with `argMax(..., created_at)`
* to sort on, and identical values make that pick irrelevant to the expected
* order instead of a race.
*/
const SEED_ITEMS: JsonSortableItemSeed[] = [
{
input: 'q1',
expected_output: 'A',
output: { [PLAIN_OUTPUT_KEY]: 'delta', [QUOTED_OUTPUT_KEY]: 'kilo', [BACKSLASHED_OUTPUT_KEY]: 'romeo' },
},
{
input: 'q2',
expected_output: 'B',
output: { [PLAIN_OUTPUT_KEY]: 'alpha', [QUOTED_OUTPUT_KEY]: 'zulu', [BACKSLASHED_OUTPUT_KEY]: 'papa' },
},
{
input: 'q3',
expected_output: 'C',
output: { [PLAIN_OUTPUT_KEY]: 'charlie', [QUOTED_OUTPUT_KEY]: 'hotel', [BACKSLASHED_OUTPUT_KEY]: 'tango' },
},
{
input: 'q4',
expected_output: 'D',
output: { [PLAIN_OUTPUT_KEY]: 'bravo', [QUOTED_OUTPUT_KEY]: 'yankee', [BACKSLASHED_OUTPUT_KEY]: 'quebec' },
},
];

const EXPERIMENT_SUFFIXES = ['expA', 'expB'];

/**
* Two experiments over one dataset whose evaluation-task outputs are sortable
* by JSON key.
*
* Seeded through the SDK bridge (dataset + traces) and then linked with the
* backend client, the same split `aged-experiment.fixture.ts` uses: the bridge's
* `compare-seed` route writes one fixed `output` key per item, and this fixture
* needs several keys per item — including keys the public SDK can carry but that
* route cannot express.
*
* Teardown lives here rather than in the spec so it still runs when an
* assertion throws. Experiments and datasets do not cascade with the project,
* so both are deleted explicitly; the traces go with the project fixture.
*/
export const test = baseTest.extend<JsonSortableComparisonFixtures>({
jsonSortableComparison: async (
{ sdkClient, backendClient, project, testNamespace },
use,
testInfo,
) => {
const datasetName = `${testNamespace}-jsonsort-ds`;

const dataset = await sdkClient.python.createDataset({
project_name: project.name,
name: datasetName,
description: 'evaluation-task outputs with sortable JSON keys',
items: SEED_ITEMS.map(({ input, expected_output }) => ({ input, expected_output })),
});

const stored = await backendClient.getDatasetItems(dataset.id);
Comment on lines +119 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setup failure leaves comparison artifacts

A rejection from createDataset, getDatasetItems, trace creation, createExperiment, or createExperimentItems before await use(ref) skips fixture cleanup, so partial datasets, traces, or experiments survive until best-effort global teardown and may remain if the runner exits early — should we wrap setup and teardown in try/finally, record the experiment ID on creation, and delete resources in dependency order?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/fixtures/json-sortable-comparison.fixture.ts` around lines 119-126
and the fixture setup/teardown through lines 202-217, refactor `jsonSortableComparison`
to wrap dataset creation, experiment setup, and `await use(ref)` in a `try/finally`.
Track each experiment ID immediately when it is generated, including the currently
incomplete experiment, and make the `finally` cleanup delete all partially created
experiments before deleting the dataset, while preserving `shouldLeaveArtifacts`
behavior and safe best-effort deletion.

const idByInput: Record<string, string> = {};
for (const item of stored) {
idByInput[String(item.data.input)] = item.id;
}
const missing = SEED_ITEMS.filter((item) => !idByInput[item.input]).map((item) => item.input);
if (missing.length > 0) {
throw new Error(
`[jsonSortableComparison] dataset ${datasetName} is missing seeded item(s) ${missing.join(', ')} — ` +
`stored inputs: ${stored.map((s) => String(s.data.input)).join(', ')}`,
);
}
const itemIds = SEED_ITEMS.map((item) => idByInput[item.input]);

const experiments: JsonSortableExperimentRef[] = [];
for (const suffix of EXPERIMENT_SUFFIXES) {
const experimentId = uuid7();
const experimentName = `${testNamespace}-jsonsort-${suffix}`;

const links: Array<{ experimentId: string; datasetItemId: string; traceId: string }> = [];
for (let i = 0; i < SEED_ITEMS.length; i++) {
const trace = await sdkClient.python.createNestedTrace({
project_name: project.name,
name: `${experimentName}-${SEED_ITEMS[i].input}`,
input: { question: SEED_ITEMS[i].input },
output: SEED_ITEMS[i].output,
spans: [],
});
links.push({ experimentId, datasetItemId: itemIds[i], traceId: trace.id });
}

await backendClient.createExperiment({
id: experimentId,
name: experimentName,
datasetName,
projectName: project.name,
});
await backendClient.createExperimentItems(links);

experiments.push({ experimentId, experimentName });
}

const outputByItemId: Record<string, Record<string, string>> = {};
SEED_ITEMS.forEach((item, i) => {
outputByItemId[itemIds[i]] = item.output;
});

// Codepoint order, not localeCompare: ClickHouse orders the extracted JSON
// values bytewise, and every seeded value is lowercase ASCII, so the two
// agree — a locale-aware comparison would not necessarily.
const ascOrderByKey: Record<string, string[]> = {};
for (const key of JSON_SORTABLE_OUTPUT_KEYS) {
ascOrderByKey[key] = [...itemIds].sort((a, b) => {
const left = outputByItemId[a][key];
const right = outputByItemId[b][key];
return left < right ? -1 : left > right ? 1 : 0;
});
}

const ref: JsonSortableComparisonRef = {
datasetId: dataset.id,
datasetName,
projectName: project.name,
items: SEED_ITEMS,
itemIds,
outputByItemId,
ascOrderByKey,
experiments,
};
await testInfo.attach('opik.jsonSortableComparison', {
body: JSON.stringify(ref, null, 2),
contentType: 'application/json',
});

await use(ref);

if (!shouldLeaveArtifacts(testInfo)) {
const safe = async (what: string, fn: () => Promise<unknown>): Promise<void> => {
try {
await fn();
Comment on lines +200 to +205

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failure artifacts are deleted anyway

global-teardown.ts unconditionally deletes current-run cuj-${runId}- experiments and datasets even when OPIK_LEAVE_FAILURES=true, so the fixture's shouldLeaveArtifacts(testInfo) retention path cannot preserve them for post-failure inspection — should we make the run-end sweep honor the same retention flag?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/fixtures/json-sortable-comparison.fixture.ts` around lines
200-205, preserve the fixture’s failure-retention behavior and update
`global-teardown.ts` so its current-run experiment and dataset cleanup honors the same
`OPIK_LEAVE_FAILURES`/`shouldLeaveArtifacts` flag. When retention is enabled, skip
deleting `cuj-${runId}-` entities—including the `*-jsonsort-*` resources—while
retaining the existing cleanup behavior otherwise; do not limit the flag to scratch
files only.

} catch (err) {
console.warn(`[jsonSortableComparison fixture] delete warning for ${what}:`, err);
}
};
// Experiments before the dataset they reference.
for (const experiment of experiments) {
await safe(`experiment ${experiment.experimentName}`, () =>
backendClient.deleteExperiment(experiment.experimentId),
);
}
await safe(`dataset ${datasetName}`, () => backendClient.deleteDataset(dataset.id));
}
},
});

export { expect } from './evaluated-thread.fixture';
Loading
Loading