feat(experiments): Experiment group 'ranked fields' (default sort + basis for rank)#512
Merged
Conversation
…asis for rank) Signed-off-by: shanaiabuggy <[email protected]>
Contributor
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesExperiment Group Default Sort
Sequence Diagram(s)sequenceDiagram
participant list_experiments
participant _default_sort_keys
participant _sort_experiments
list_experiments->>_default_sort_keys: resolve default_sort fallback when sort is omitted
list_experiments->>_sort_experiments: sort rows with keys and pinned_first
_sort_experiments-->>list_experiments: ordered experiments
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi/openapi.yaml (1)
3711-3728: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
sortis now nullable at runtime, but the response schema still says string.When this parameter is omitted, the handler returns
Page(sort=sort)withsort=None.ExperimentResponsesPage.sortstill only allowstype: string, so the documented default-ranking path produces a response that strict OpenAPI clients will reject.🤖 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 3711 - 3728, The OpenAPI definition for the sort query parameter and the resulting ExperimentResponsesPage.sort field are inconsistent with runtime behavior because omitted sort now yields null. Update the schema in openapi/openapi.yaml so the sort response/property accepts null in addition to string, and make sure the documented default-ranking path matches the actual Page(sort=sort) behavior in the handler.
🤖 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 10375-10383: The ExperimentGroupRequest.ranking and
ExperimentGroupResponse.ranking schemas are currently array-only, but the
backend models allow null via list[RankingField] | None. Update the Ranking
property in the openapi schema for both ExperimentGroupRequest and
ExperimentGroupResponse to explicitly permit null so requests and responses can
use ranking: null without violating the contract.
In `@openapi/ga/openapi.yaml`:
- Around line 10375-10383: The GA OpenAPI contract for the Ranking schema
currently only allows an array, but the backend model and intake schema allow
ranking to be null, so generated clients will reject valid responses. Update the
Ranking definition in the openapi spec so the ranking property is nullable in
the components schema used by the experiments group, and make the same nullable
change anywhere the duplicate RankingField array definition appears. Keep the
existing ranking array structure and description, but ensure the schema
explicitly permits null for groups without ranking.
In `@openapi/openapi.yaml`:
- Around line 10375-10383: The Ranking schema for the group model is currently
array-only, but the backend uses RankingField list-or-none, so update the
ranking field in the referenced schema definition and the related duplicate
schema block to allow null in both request and response. Keep the existing array
item reference to RankingField, and make sure the nullable setting is applied
consistently wherever ranking is defined so updates can clear it and responses
can represent groups without a configured ranking.
In `@services/intake/tests/test_experiment_ranking.py`:
- Around line 112-123: The test in test_default_order_floats_pinned_first is not
strong enough because exp-b is both pinned and newer, so a newest-first fallback
would still pass. Change the setup in test_default_order_floats_pinned_first to
pin the older experiment (or create the pinned one first) and assert the full
returned order from the EXPERIMENTS listing so the default ordering path in the
experiment ranking logic is actually verified.
---
Outside diff comments:
In `@openapi/openapi.yaml`:
- Around line 3711-3728: The OpenAPI definition for the sort query parameter and
the resulting ExperimentResponsesPage.sort field are inconsistent with runtime
behavior because omitted sort now yields null. Update the schema in
openapi/openapi.yaml so the sort response/property accepts null in addition to
string, and make sure the documented default-ranking path matches the actual
Page(sort=sort) behavior in the handler.
🪄 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: a27da37b-9d95-40aa-8619-9e7099665036
⛔ Files ignored due to path filters (14)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/experiment_groups.pyis 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/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_create_params.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/experiment_groups/experiment_group_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/ranking_field.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/ranking_field_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiment_groups.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (8)
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/tests/test_experiment_ranking.pyservices/intake/tests/test_experiment_sort.py
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
commented
Jun 30, 2026
BrianNewsom
approved these changes
Jun 30, 2026
BrianNewsom
left a comment
BrianNewsom
left a comment
Contributor
There was a problem hiding this comment.
Discussed live, approved after updates!
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/intake/tests/test_experiment_default_sort.py (1)
130-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDegradation test doesn't assert fallback ordering.
Test confirms default path returns 200 and explicit sort returns 503, but never asserts the fallback actually orders by
-created_at. The core claim under test ("default sort degrades to-created_atinstead of 503") is left unverified beyond status code.Suggested strengthening
client.post(EXPERIMENTS, json={"name": "e1", "experiment_group_id": group["id"], "dataset_name": "ds"}) + client.post(EXPERIMENTS, json={"name": "e2", "experiment_group_id": group["id"], "dataset_name": "ds"}) default_sorted = client.get(EXPERIMENTS, params={"filter[experiment_group_id]": group["id"]}) assert default_sorted.status_code == 200, default_sorted.text + names = [e["name"] for e in default_sorted.json()["data"]] + assert names == ["e2", "e1"] # newest first via -created_at fallback🤖 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/test_experiment_default_sort.py` around lines 130 - 144, The test in test_default_metric_sort_degrades_without_rollups only checks the response codes, but it does not verify that the group’s default metric sort actually falls back to -created_at. Strengthen the test by asserting the ordering of the experiments returned from the EXPERIMENTS GET for the default path, using the existing setup in test_default_metric_sort_degrades_without_rollups and comparing the returned items against created_at order.
🤖 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/openapi.yaml`:
- Around line 10375-10383: The ExperimentGroupRequest.default_sort and
ExperimentGroupResponse.default_sort schemas are currently defined as
non-nullable arrays, but the generated models treat them as list[SortCriterion]
| None. Update the OpenAPI definitions for default_sort to allow null (for
example via anyOf or an explicit null type alongside the array schema) in both
ExperimentGroupRequest and ExperimentGroupResponse so clients can send and
receive unset sorts correctly.
---
Nitpick comments:
In `@services/intake/tests/test_experiment_default_sort.py`:
- Around line 130-144: The test in
test_default_metric_sort_degrades_without_rollups only checks the response
codes, but it does not verify that the group’s default metric sort actually
falls back to -created_at. Strengthen the test by asserting the ordering of the
experiments returned from the EXPERIMENTS GET for the default path, using the
existing setup in test_default_metric_sort_degrades_without_rollups and
comparing the returned items against created_at order.
🪄 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: eb43476e-6f1f-4fd0-b7a7-045b7140c3f8
⛔ Files ignored due to path filters (14)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/experiment_groups.pyis 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/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_create_params.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/experiment_groups/experiment_group_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/sort_criterion.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/sort_criterion_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiment_groups.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (7)
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/tests/test_experiment_default_sort.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/intake/src/nmp/intake/entities/experiments.py
Signed-off-by: shanaiabuggy <[email protected]>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…asis for rank) (#512) * feat(experiments): Experiment group 'ranked fields' (default sort + basis for rank) Signed-off-by: shanaiabuggy <[email protected]> * bunny Signed-off-by: shanaiabuggy <[email protected]> * renamez Signed-off-by: shanaiabuggy <[email protected]> * rabbit Signed-off-by: shanaiabuggy <[email protected]> --------- Signed-off-by: shanaiabuggy <[email protected]>
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. No way to set a group's ranking, so users can't see the "winner" of a group at a glance.
Approach. Add a ranking: [{ field, direction }]| null config (ordered list, optional) on ExperimentGroup, settable at create/update. field is restricted to sortable (numeric) metrics — v1: cost_usd and latency_ms; evaluator-score fields follow once ASE-320 lands. The list is ordered by the fields in priority order (first = primary, rest = tiebreakers). This one config does double duty:
The experiments-list endpoint applies it as the default sort when no explicit sort is passed (explicit sort
still overrides). Pinned experiments float to top, then ranked-fields order. If no ranked field has resolved data, ordering falls back to -created_at.
It's the basis for the computed rank (see the rank ticket).
Acceptance. Setting a group's ranked fields changes the default order of its experiments list; an explicit sort
query overrides it; pinned-first is preserved; only numeric metrics / evaluator names are accepted; an expected evaluator with no data yet leaves those experiments unranked on that field.
Summary by CodeRabbit
New Features
default_sortfor experiments.created_atfallback).p99aggregate-stat variant.Bug Fixes
created_at.Tests
Documentation
default_sortandSortCriterionschema, and clarifiedsortparameter behavior.