feat(experiments): Require experiments to be associated with an experiment group#268
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughAdds soft-delete flags and filtering for experiments and groups, makes experiment_group_id required in contracts and entities, converts deletes to soft-delete with name-mangling and cascade, enforces group existence/non-deletion on create/update, and updates integration tests and ingest validation accordingly. ChangesSoft-delete implementation for experiments and experiment groups
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openapi/ga/individual/platform.openapi.yaml (1)
3664-3677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the new
400response contracts.These endpoints now have reachable
400cases per the PR scope, but the spec still only advertises201/200/409/422. That leaves generated clients and docs behind the implementation.Proposed OpenAPI fix
responses: + '400': + description: Referenced experiment group does not exist or is soft-deleted '201': description: Successful Responseresponses: + '400': + description: Referenced experiment group does not exist or is soft-deleted '200': description: Successful Responseresponses: + '400': + description: Referenced experiment is soft-deleted '201': description: Successful ResponseAlso applies to: 3809-3824, 3939-3946
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/ga/individual/platform.openapi.yaml` around lines 3664 - 3677, Add a '400' response object to the affected OpenAPI operations so the spec advertises the new reachable bad-request cases: update the response blocks that currently list '201'/'200'/'409'/'422' (e.g., the operation returning ExperimentResponse) to include a '400' entry with a descriptive "Bad Request" description and a content schema (e.g., reference the existing HTTPValidationError or a BadRequest schema) so generated clients/docs match implementation; apply the same change to the other occurrences mentioned (around the blocks at 3809-3824 and 3939-3946).openapi/openapi.yaml (2)
3526-3527:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilter parameter descriptions are stale after adding
is_deleted.Line 3526-3527 and Line 3734-3737 still describe old filter fields and omit
is_deleted, which now exists inExperimentGroupFilterandExperimentFilter.Also applies to: 3734-3737
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 3526 - 3527, Update the stale filter parameter descriptions to reflect the new is_deleted field: in the descriptions associated with ExperimentGroupFilter and ExperimentFilter (the parameter text that currently reads like "Filter experiment groups by name." and the similar block later), mention the new boolean is_deleted filter, its purpose (e.g., include/exclude deleted records), accepted values (true/false), and any default behavior; ensure both occurrences that document these filters (the ExperimentGroupFilter and ExperimentFilter parameter/description blocks) are edited consistently so the OpenAPI docs accurately list name, is_deleted, and any other existing fields.
3664-3677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpenAPI is missing new HTTP 400 contracts.
Line 3664-3677 and Line 3809-3824 still document only
409/422, but create/update now reject missing/deleted group references with400.
Line 3939-3946 also needs400for ATIF ingest against deleted experiments.Also applies to: 3809-3824, 3939-3946
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 3664 - 3677, The OpenAPI responses for the experiment create/update and ATIF ingest endpoints are missing the new 400 response for missing/deleted group references; update the response blocks that currently list '201'/'409'/'422' (the block that references ExperimentResponse and HTTPValidationError) and the analogous blocks for ATIF ingest to include a '400' response with description "Bad Request" and an application/json content schema (e.g., $ref: '`#/components/schemas/HTTPValidationError`' or the existing error schema used for validation responses) so callers can receive structured errors for missing/deleted group references.
🧹 Nitpick comments (3)
services/intake/tests/integration/test_experiments_crud.py (2)
18-18: ⚡ Quick winUse parameterized dict return type.
Return type should be
dict[str, Any]instead of baredict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/intake/tests/integration/test_experiments_crud.py` at line 18, Update the return type annotation of the helper function _experiment_body to use a parameterized dict return type (dict[str, Any]) instead of a bare dict; locate the function definition for _experiment_body and change its signature return annotation accordingly and ensure Any is imported from typing if not already present.Source: Coding guidelines
33-33: ⚡ Quick winUse parameterized dict return type.
Return type should be
dict[str, Any]instead of baredict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/intake/tests/integration/test_experiments_crud.py` at line 33, Update the return type annotation of the helper function _create_group to use a parameterized dict: change the signature from dict to dict[str, Any]; import Any from typing if not already imported so the annotation resolves. Ensure the function definition _create_group(client: TestClient, name: str = "default-test-group") -> dict[str, Any] is updated accordingly.Source: Coding guidelines
services/intake/src/nmp/intake/api/v2/experiments/endpoints.py (1)
586-594: 💤 Low value
_to_base36loops forever on negative input.Current callers always pass positive timestamps, but a defensive guard prevents misuse if the function is reused elsewhere.
def _to_base36(value: int) -> str: + if value < 0: + raise ValueError("value must be non-negative") if value == 0: return "0"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/intake/src/nmp/intake/api/v2/experiments/endpoints.py` around lines 586 - 594, The _to_base36 function currently loops forever for negative integers; add a defensive guard at the start of _to_base36 to handle negative input (e.g., if value < 0) and raise a clear ValueError (e.g., "value must be non-negative") so callers cannot pass negatives silently and trigger an infinite loop; keep the rest of the logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 9831-9834: The is_deleted boolean property must be added to the
JSON Schema "required" list so responses always include it; update each response
object schema that defines is_deleted (the property named is_deleted) to include
a required: [ "is_deleted" ] entry (you can keep default: false) so the field is
mandatory in the OpenAPI responses—apply this change to all occurrences that
currently define is_deleted (including the other instances referenced in the
comment).
---
Outside diff comments:
In `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 3664-3677: Add a '400' response object to the affected OpenAPI
operations so the spec advertises the new reachable bad-request cases: update
the response blocks that currently list '201'/'200'/'409'/'422' (e.g., the
operation returning ExperimentResponse) to include a '400' entry with a
descriptive "Bad Request" description and a content schema (e.g., reference the
existing HTTPValidationError or a BadRequest schema) so generated clients/docs
match implementation; apply the same change to the other occurrences mentioned
(around the blocks at 3809-3824 and 3939-3946).
In `@openapi/openapi.yaml`:
- Around line 3526-3527: Update the stale filter parameter descriptions to
reflect the new is_deleted field: in the descriptions associated with
ExperimentGroupFilter and ExperimentFilter (the parameter text that currently
reads like "Filter experiment groups by name." and the similar block later),
mention the new boolean is_deleted filter, its purpose (e.g., include/exclude
deleted records), accepted values (true/false), and any default behavior; ensure
both occurrences that document these filters (the ExperimentGroupFilter and
ExperimentFilter parameter/description blocks) are edited consistently so the
OpenAPI docs accurately list name, is_deleted, and any other existing fields.
- Around line 3664-3677: The OpenAPI responses for the experiment create/update
and ATIF ingest endpoints are missing the new 400 response for missing/deleted
group references; update the response blocks that currently list
'201'/'409'/'422' (the block that references ExperimentResponse and
HTTPValidationError) and the analogous blocks for ATIF ingest to include a '400'
response with description "Bad Request" and an application/json content schema
(e.g., $ref: '`#/components/schemas/HTTPValidationError`' or the existing error
schema used for validation responses) so callers can receive structured errors
for missing/deleted group references.
---
Nitpick comments:
In `@services/intake/src/nmp/intake/api/v2/experiments/endpoints.py`:
- Around line 586-594: The _to_base36 function currently loops forever for
negative integers; add a defensive guard at the start of _to_base36 to handle
negative input (e.g., if value < 0) and raise a clear ValueError (e.g., "value
must be non-negative") so callers cannot pass negatives silently and trigger an
infinite loop; keep the rest of the logic unchanged.
In `@services/intake/tests/integration/test_experiments_crud.py`:
- Line 18: Update the return type annotation of the helper function
_experiment_body to use a parameterized dict return type (dict[str, Any])
instead of a bare dict; locate the function definition for _experiment_body and
change its signature return annotation accordingly and ensure Any is imported
from typing if not already present.
- Line 33: Update the return type annotation of the helper function
_create_group to use a parameterized dict: change the signature from dict to
dict[str, Any]; import Any from typing if not already imported so the annotation
resolves. Ensure the function definition _create_group(client: TestClient, name:
str = "default-test-group") -> dict[str, Any] is updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3f53c92-db89-4d58-8fa0-80280319446b
⛔ Files ignored due to path filters (11)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/pyproject.tomlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiment_groups.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiments.pyis excluded by!sdk/**
📒 Files selected for processing (10)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.pyservices/intake/src/nmp/intake/entities/experiments.pyservices/intake/src/nmp/intake/spans/ingest/experiment_context_validation.pyservices/intake/tests/integration/spans/test_experiment_rollups.pyservices/intake/tests/integration/spans/test_experiment_sessions.pyservices/intake/tests/integration/test_experiments_crud.py
Contributor
|
BrianNewsom
approved these changes
Jun 11, 2026
Signed-off-by: shanaiabuggy <[email protected]>
…has a side-effect' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
force-pushed
the
sbuggy/fp-222
branch
from
June 11, 2026 18:43
f2b7b2b to
5d09b23
Compare
shanaiabuggy
enabled auto-merge
June 11, 2026 18:43
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openapi/openapi.yaml (3)
3526-3527:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winList filter descriptions are stale after adding
is_deleted.Both list endpoint parameter descriptions omit
is_deletedeven though it is now part ofExperimentGroupFilterandExperimentFilter.Also applies to: 3734-3736
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 3526 - 3527, Update the list endpoint parameter descriptions to reflect the added is_deleted field: find the OpenAPI parameter descriptions that mention ExperimentGroupFilter and ExperimentFilter (the list endpoint "filter" parameter for experiment groups and experiments) and add is_deleted to the prose (e.g., "Filter by name and is_deleted" or "Filter by name, is_deleted") and any examples/schema descriptions so they match the ExperimentGroupFilter and ExperimentFilter definitions which now include is_deleted.
3664-3673:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate-experiment error contract is missing the 400 path.
POST /apis/intake/v2/workspaces/{workspace}/experimentsrejects deleted-group targets with HTTP 400, but 400 is undocumented here.Proposed OpenAPI patch
responses: '201': description: Successful Response content: application/json: schema: $ref: '`#/components/schemas/ExperimentResponse`' + '400': + description: Invalid experiment_group_id (e.g., group does not exist or is deleted) '409': description: Experiment already exists '422': description: Validation Error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 3664 - 3673, Add a 400 response to the POST /apis/intake/v2/workspaces/{workspace}/experiments operation: declare a '400' response with a short description (e.g., "Bad Request — rejected deleted-group targets") and include content application/json with a schema reference to the project’s standard error model (e.g., '`#/components/schemas/ErrorResponse`' or the existing error schema used by other endpoints). Update the operation alongside the existing '201', '409', and '422' responses so the OpenAPI contract documents the rejected deleted-group target case.
3939-3942:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument HTTP 400 for deleted-experiment ingest rejection.
POST /apis/intake/v2/workspaces/{workspace}/ingest/atifnow returns 400 for deleted experiment context, but the spec only declares 201/422. This breaks generated client/error handling contracts.Proposed OpenAPI patch
responses: '201': description: Successful Response + '400': + description: Experiment context invalid (e.g., target experiment is deleted) '422': description: Validation Error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/openapi.yaml` around lines 3939 - 3942, The OpenAPI operation for POST /apis/intake/v2/workspaces/{workspace}/ingest/atif currently documents only '201' and '422' responses but the implementation returns 400 for deleted-experiment ingest rejections; add a '400' response entry to that operation (POST /apis/intake/v2/workspaces/{workspace}/ingest/atif) with a clear description like "Bad Request - deleted experiment" and mirror the error response schema/headers used for 422 (or reference the same error model) so generated clients will handle this status consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 3526-3527: Update the list endpoint parameter descriptions to
reflect the added is_deleted field: find the OpenAPI parameter descriptions that
mention ExperimentGroupFilter and ExperimentFilter (the list endpoint "filter"
parameter for experiment groups and experiments) and add is_deleted to the prose
(e.g., "Filter by name and is_deleted" or "Filter by name, is_deleted") and any
examples/schema descriptions so they match the ExperimentGroupFilter and
ExperimentFilter definitions which now include is_deleted.
- Around line 3664-3673: Add a 400 response to the POST
/apis/intake/v2/workspaces/{workspace}/experiments operation: declare a '400'
response with a short description (e.g., "Bad Request — rejected deleted-group
targets") and include content application/json with a schema reference to the
project’s standard error model (e.g., '`#/components/schemas/ErrorResponse`' or
the existing error schema used by other endpoints). Update the operation
alongside the existing '201', '409', and '422' responses so the OpenAPI contract
documents the rejected deleted-group target case.
- Around line 3939-3942: The OpenAPI operation for POST
/apis/intake/v2/workspaces/{workspace}/ingest/atif currently documents only
'201' and '422' responses but the implementation returns 400 for
deleted-experiment ingest rejections; add a '400' response entry to that
operation (POST /apis/intake/v2/workspaces/{workspace}/ingest/atif) with a clear
description like "Bad Request - deleted experiment" and mirror the error
response schema/headers used for 422 (or reference the same error model) so
generated clients will handle this status consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96e1d1c8-25ba-4f15-80b0-0c9a056ffc75
⛔ Files ignored due to path filters (9)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiment_groups.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiments.pyis excluded by!sdk/**
📒 Files selected for processing (3)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- openapi/ga/individual/platform.openapi.yaml
- openapi/ga/openapi.yaml
This was referenced Jun 12, 2026
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
3 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.
We now require Experiments to be associated with an Experiment group.
Also added the following deletion behavior to Experiment and Experiment groups:
is_deleted=trueand renames the row to{name}-deleted-{base36-millis}{hex}— the original name is immediately free for reuse.filter[is_deleted]=truereturns only deleted rows.Summary by CodeRabbit
New Features
Bug Fixes