Repository navigation
Add reusable experiment support across runs - #1496
Cedric Vidal (cedricvidal) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8b89e529-c849-4715-8d82-4ad4baa9f14c
Test Results (Node.js 22)test: Run #256
🎉 All tests passed! |
Wassim Chegham (manekinekko)
left a comment
There was a problem hiding this comment.
Requesting changes for the Cosmos-incompatible index migration and the reserved experiment-key collision. The experiment badge placement is non-blocking. These findings came from the end-to-end review of dependent #1031 and belong on this originating PR.
| async up(db: Db): Promise<void> { | ||
| const collection = db.collection("requests"); | ||
| try { | ||
| await collection.createIndex({ experimentId: 1 }, { sparse: true }); |
There was a problem hiding this comment.
[P2] Create a Cosmos-supported index and propagate real migration failures
Cosmos DB for MongoDB does not support sparse indexes. This requests a sparse index, then catches every creation error and lets up() resolve successfully. The migration can therefore be recorded as applied, and readiness can pass, without the experiment index. A mocked unsupported-index rejection reproduces the false-success behavior; permission and transient failures are swallowed too.
Please use a supported non-sparse index on Cosmos, tolerate only recognized idempotency conditions, and verify the installed index before declaring success. The repository's migration 029 already handles this sparse-index compatibility distinction.
|
|
||
| // Phase 2: Full aggregation scoped to current page's groups only | ||
| const concretePageKeys = missingGroupKey | ||
| ? pageKeys.filter((key) => key !== missingGroupKey) |
There was a problem hiding this comment.
[P2] Prevent real experiment IDs from colliding with the missing-value sentinel
Creation accepts experimentId: "no-experiment", but this removes that real value from the phase-two keys and substitutes only missing/null predicates. For ?groupBy=experiment&experimentId=no-experiment, the resulting match requires both experimentId === "no-experiment" and an absent/null value, so the group disappears. Without the filter, the real experiment also collides with the missing-value bucket. The Portal member-fetch helper interprets this same key as __empty__.
Please reject reserved sentinel IDs at creation or use collision-free group keys consistently in pagination and member fetching. Add a case with a real experiment named no-experiment, not just documents lacking the field.
| > | ||
| {formatId(run.agentsMdPromptId)} | ||
| </TaskPromptBadge> | ||
| {run.experimentId && ( |
There was a problem hiding this comment.
[P3] Display experiment metadata independently of AGENTS.md
This badge is inside the enclosing run.agentsMdPromptId guard. Runs with an experiment ID but no AGENTS.md therefore never show their experiment on the detail page, even though those fields are independent. This includes the GEPA optimizer's empty seed, which deliberately omits AGENTS.md.
Please render the experiment ID in the general run metadata, outside the AGENTS.md card, so baseline and ordinary experiment runs expose the same correlation information.
|
Wassim Chegham (@manekinekko) this PR is not yet ready for review, I accidently opened it |
Summary
Split the reusable experiment-support layer from #1031 into a standalone change targeting
main.experimentIdrequest/response types and persistence, exact or missing-value filtering, two-phase server grouping, and bulk-resubmission preservation.requests.experimentIdindex, required-migration registration, focused tests, generated OpenAPI, and CLI command snapshots.Demos
N/A — no recording was produced for this extracted capability PR.
Before
N/A
After
N/A
Testing
Passed:
pnpm test apps/api/src/endpoints.test.ts apps/api/src/grouping.test.ts apps/api/src/routes/requests/run-query.test.ts packages/db-migrations/src/031-add-experiment-id-index.test.ts packages/db-migrations/src/check-migrations.test.ts— focused API/filter/grouping/migration tests passed (229 tests across the executed focused runs).pnpm test apps/cli/src/commands/run.test.ts apps/cli/src/cli-commands.test.ts apps/api/src/openapi-snapshot.test.ts— CLI and generated snapshot tests passed.pnpm --filter api generate:openapi && pnpm test -u apps/api/src/openapi-snapshot.test.ts apps/cli/src/cli-commands.test.ts && pnpm test apps/api/src/openapi-snapshot.test.ts apps/cli/src/cli-commands.test.ts— regenerated and verified OpenAPI and CLI snapshots.pnpm --filter shared buildpnpm --filter telemetry build && pnpm --filter api buildpnpm --filter cli buildpnpm --filter portal buildpnpm --filter db-migrations buildpnpm --filter portal lintpnpm headers:checkgit diff --checkorigin/main...HEAD: exactly 21 experiment-support files; noapps/gepa-optimizer/**, GEPA CI/config, root optimizer docs, or GEPA-specific content.Not runnable in this checkout:
pnpm --filter shared lintandpnpm --filter api lintbecause the repository install does not provide theeslintexecutable. The equivalent TypeScript builds and focused tests above passed.Documentation and compatibility
Updated
docs/architecture/app-design.mdwith the experiment correlation/filtering/grouping contract and migration 031 index behavior.This is additive and backward-compatible:
experimentIdis optional, existing runs remain unchanged, and the sparse index avoids entries for runs without an experiment. Deploy migration 031 before relying on experiment-filter query efficiency.Checklist
up()/down()and keep it CosmosDB-compatible.NOTICE/NOTICE-REVIEW.txtwithpnpm noticeas needed. — N/A: no dependency changes.