feat(experiments): update experiment serializer and service to handle excluded variants#60005
Merged
rodrigoi merged 7 commits intoMay 29, 2026
Conversation
Member
Author
This was referenced May 26, 2026
Merged
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
Size Change: +17.1 kB (+0.02%) Total Size: 80.8 MB 📦 View Changed
ℹ️ View Unchanged
|
rodrigoi
changed the title
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment-api-serializers
branch
from
63f348a to
6aa8131
Compare
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
This comment was marked as outdated.
This comment was marked as outdated.
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment
branch
from
ade310a to
8da70d1
Compare
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment-api-serializers
branch
from
18c4537 to
47c470a
Compare
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
products/experiments/backend/test/test_experiment_service.py:4921-4962
**Tests should be parametrised**
The team convention is to prefer parametrised tests. The seven "raises" cases and four "valid" cases all share identical structure — they differ only in the `excluded_variants` value and the expected error message. Keeping them as individual methods means each new edge case requires a full copy-paste rather than adding a tuple to a `@pytest.mark.parametrize` decorator, and the repetition obscures the intent.
The error cases in particular (`test_excluding_unknown_key_raises`, `test_excluding_control_raises`, `test_excluding_holdout_pseudo_key_raises`, `test_excluding_all_test_variants_raises`, `test_non_list_excluded_variants_raises`, `test_non_string_entries_raises`, `test_excluding_custom_baseline_raises`) could be collapsed into a single `@pytest.mark.parametrize` test, and the valid cases similarly.
### Issue 2 of 2
products/experiments/backend/experiment_service.py:193-218
**Validation cannot see existing variants during a PATCH that omits `feature_flag_variants`**
`validate_parameters` in the serializer is called with only the incoming request value. If a caller PATCHes with `{"parameters": {"excluded_variants": ["test-1"]}}` and omits `feature_flag_variants`, this function receives `parameters.get("feature_flag_variants", [])` → `[]`, so `variant_keys` is empty and every entry in `excluded_variants` is immediately rejected as "unknown variants for this experiment".
This is fine if the API contract requires callers to always send the full `parameters` object — but that constraint isn't stated anywhere in the help text or validation error messages. A comment clarifying the expected usage (or a friendlier error when `feature_flag_variants` is absent but `excluded_variants` is non-empty) would prevent confusing support cases.
Reviews (1): Last reviewed commit: "test(mcp): update unit test snapshots" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment
branch
from
8da70d1 to
ad7a5b6
Compare
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment-api-serializers
branch
2 times, most recently
from
7e8f2d9 to
3375e8e
Compare
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment
branch
from
ad7a5b6 to
0e94f30
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment
branch
from
0e94f30 to
5009ff6
Compare
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment-api-serializers
branch
from
3375e8e to
8484a73
Compare
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
jurajmajerik
approved these changes
May 29, 2026
Base automatically changed from
experiments/exclude-variant-from-experiment
to
master
May 29, 2026 16:01
… updates for the excluded variants.
…pport excluded variants.
rodrigoi
force-pushed
the
experiments/exclude-variant-from-experiment-api-serializers
branch
from
8484a73 to
467f5d6
Compare
rodrigoi
deleted the
experiments/exclude-variant-from-experiment-api-serializers
branch
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Problem
The frontend needs a backend contract for excluding test variants from experiment analysis. This PR adds that contract on the serializer and service layer so the API accepts, validates, and persists an
excluded_variantslist onExperiment.parameters, and downstream code paths (fingerprinting, recompute) thread it through.Changes
This PR is the API + service entry point of the excluded-variants feature. It validates user input, documents the new parameter on the generated TypeScript / MCP schemas, and propagates the list into metric fingerprint computation so cached results invalidate when the exclusion set changes.
Validation
ExperimentService.validate_experiment_parametersenforces the invariants that protect downstream code from bad input:excluded_variantsmust be a list of strings.control, overridable viastats_config.baseline_variant_key) cannot be excluded.holdout-*) cannot be excluded - they have their own opt-out mechanism.All rejections collect every offender in the message rather than stopping at the first, which is friendlier for API and MCP callers fixing a batch payload.
products/experiments/backend/experiment_service.pyproducts/experiments/backend/test/test_experiment_service.pySerializer
The
parametersfield'shelp_textis rewritten to enumerate all supported keys and documentexcluded_variantssemantics, so the new field surfaces correctly in the generated OpenAPI schema and downstream MCP tool definitions. The serializer also threadsexcluded_variantsinto_recompute_fingerprintsduring updates.products/experiments/backend/presentation/serializers.pyFingerprint propagation
_recompute_fingerprintsaccepts the newexcluded_variantskwarg and forwards it tocompute_metric_fingerprint. Every call site within the service (create, update, fingerprint recompute) passes the value pulled from(experiment.parameters or {}).get(\"excluded_variants\"). Without this, the fingerprint would not invalidate when the exclusion list changes, leaving staleExperimentMetricResultrows in the cache.Generated artifacts
Help text and validation propagate into the OpenAPI / MCP surface via
hogli build:openapi. Regenerated files:products/experiments/frontend/generated/api.schemas.ts(generated)services/mcp/src/api/generated.ts(generated)services/mcp/src/generated/experiments/api.ts(generated)services/mcp/tests/unit/__snapshots__/tool-schemas/common/experiment-create.json(generated)services/mcp/tests/unit/__snapshots__/tool-schemas/common/experiment-duplicate.json(generated)services/mcp/tests/unit/__snapshots__/tool-schemas/common/experiment-update.json(generated)How did you test this code?
Publish to changelog?
no
🤖 Agent context
Model: Opus 4.7
Manually refactored: yes
Skills used:
Relevant decisions:
Experiment.parameters.excluded_variants: string[], keyed by variantkeyrather than ID, so the schema travels through the planned deprecation offeature_flag_variants(variant keys remain the stable identifier).excluded_variants(sorted, for determinism) so changing the list invalidatesExperimentMetricResultrows. Threaded through_recompute_fingerprintsto every create / update / recompute path.