feat(eval): ab-test target-based run (create) - #2135
Conversation
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Reviewed the top commit's diff against the stack base (feat/eval-ab-test-config-bundle-run): the new eval ab-test target-based run handler, the createTargetBasedABTest core method, the shared createABTest refactor, and the consolidated tests.
Findings:
- The extraction of
EvalClient.createABTestpreserves the pre-existing semantics (GetGateway → account, role provisioning,retryWhileRolePropagatesgated on auto-provisioned role, rollback viadeleteAbTestRole).createConfigBundleABTestnow flows through it and produces the same request shape as before — no behavioral drift. createTargetBasedABTestcorrectly maps tovariantConfiguration.targetandevaluationConfig.perVariantOnlineEvaluationConfig(C/T1 keyed to variant names), with the sameclientToken,gatewayFilter, andenableOnCreatedefaults as the config-bundle path.- Handler validation (required flags, strict
{gateway-target, online-eval}schema, identical-target rejection, weight bounds 1–99,enable-on-createenum) is symmetric with config-bundle and reasonable.SourceResolver+parseJsonFlagusage matches the established pattern. - Tests use the real
TestCoreClientfake rather than mocks, exercise routing hierarchy, required-flag matrix, mis-shaped JSON, identical-target rejection, and flag→request mapping. No excessive mocking. - Telemetry omission is consistent with the sibling eval/ab-test handlers, so no new instrumentation is expected here.
- Deferred golden fixture (per PR description) is reasonable given the shared
createABTestseam is already golden-covered by #2113.
Nothing blocking — safe to merge once the stack base (#2113) lands.
91ce951 to
18c66cc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/eval-ab-test-config-bundle-run #2135 +/- ##
======================================================================
Coverage ? 97.10%
======================================================================
Files ? 477
Lines ? 29513
Branches ? 0
======================================================================
Hits ? 28658
Misses ? 855
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds `agentcore eval ab-test target-based run` — an A/B test between two
gateway targets and their per-variant online evaluations. --control/--treatment
take {gateway-target, online-eval} JSON (inline/file/stdin via SourceResolver);
variants use variantConfiguration.target and a perVariantOnlineEvaluationConfig.
Extract a shared EvalClient.createABTest helper (GetGateway -> account, role
provision + AccessDenied/assume retry + rollback) and drive both config-bundle
and target-based create through it, removing the duplicated role/retry block.
Consolidated ab-test.test.tsx covers the target-based hierarchy, validation
(required flags, mis-shaped JSON, identical targets), and flag->request
mapping. Golden fixture deferred (needs a gateway with two wired targets +
two online-evals).
18c66cc to
2c9274e
Compare
nborges-aws
left a comment
There was a problem hiding this comment.
One clarifying question on target ID/name passing. Otherwise PR looks clean!
| { | ||
| name: "C", | ||
| weight: 100 - treatmentWeight, | ||
| variantConfiguration: { target: { name: input.control.gatewayTarget } }, |
There was a problem hiding this comment.
Is target expecting a name or id here? src/handlers/eval/ab-test/target-based/run/index.tsx#L21 documents gateway-target as <id>; I'd expect input.control.gatewayTarget is then the target ID. But this field looks to expect target name?
| { | ||
| name: "T1", | ||
| weight: treatmentWeight, | ||
| variantConfiguration: { target: { name: input.treatment.gatewayTarget } }, |
There was a problem hiding this comment.
same question as above
Stacked on #2113 (config-bundle run). Adds the second A/B-test create mode.
Command structure
Splits gateway traffic between two gateway targets, each scored by its own online-eval. The target picks the runtime + endpoint, so those aren't passed separately.
Actual input → output
Input (exploratory account, us-west-2):
Actual output — resolves + validates the gateway before any create, so an unreachable gateway fails fast, creating nothing:
Output on success (shape from
CreateABTest):{ "abTestId": "orders-v2-canary-abc123", "abTestArn": "arn:aws:bedrock-agentcore:us-west-2:…:ab-test/orders-v2-canary-abc123", "name": "orders-v2-canary", "status": "CREATING", "executionStatus": "RUNNING" }--enable-on-create false→NOT_STARTED(created PAUSED).How it works
variantConfiguration.target = { name }(vs config-bundle'sconfigurationBundle); evaluation isperVariantOnlineEvaluationConfig: [{ name: "C", … }, { name: "T1", … }]— one online-eval per variant.EvalClient.createABTest(GetGateway → account, provision role,retryWhileRolePropagateson assume/AccessDenied, rollback on failure). Both config-bundle and target-basedrunnow go through it — removes the duplicated role/retry block.--control/--treatment/--gateway-filterviaSourceResolver(inline /file:///-), same as config-bundle run.Tests
Consolidated
ab-test.test.tsxgains target-based coverage: hierarchy (ab-test → target-based → run), required-flag matrix, mis-shaped JSON, identical-target rejection, and flag→request mapping (C/T1 targets, per-variant evals, weights). 52 ab-test tests pass;typecheck0;oxlintclean.Deferred
createABTestseam is already golden-covered by feat(eval): ab-test config-bundle run (create) #2113's config-bundle run, so target-based rides the same recorded path. Follow-up.Review
Base =
feat/eval-ab-test-config-bundle-run(stack). Review after #2113 merges, or view just this branch's diff.