feat(experiments): sort the experiments list by rollup metrics#448
Merged
Conversation
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
📝 WalkthroughWalkthroughThe experiments list endpoint now accepts free-form sort strings, validates them at runtime, hydrates rollups, and sorts/paginates the hydrated list in memory to support entity fields and aggregate metrics. The OpenAPI contracts and tests were updated for the new sort expressions. ChangesExperiment sort expansion
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 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: 3
🧹 Nitpick comments (1)
services/intake/tests/integration/spans/test_experiment_metric_sort.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop postponed annotations here.
This file already imports every annotated type directly, so
from __future__ import annotationsjust turns them into deferred/string annotations for no gain.As per coding guidelines,
**/*.py: Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible.🤖 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/spans/test_experiment_metric_sort.py` at line 6, Remove the unnecessary postponed-annotations import from test_experiment_metric_sort since all annotated types are already imported directly, so the module should use normal concrete type hints instead of deferred string annotations. Update the top of the file by dropping the from __future__ import annotations line and keep the existing direct type imports in place; no TYPE_CHECKING-based workaround is needed.Source: Coding guidelines
🤖 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/openapi.yaml`:
- Around line 3716-3726: Add the missing HTTP 400 response to this operation’s
OpenAPI definition because sort validation now happens at runtime instead of
schema validation. Update the response block alongside the existing 200/422
entries in the sort-related operation, and make the same change in the mirrored
GA OpenAPI specs so generated clients match the new contract.
In `@services/intake/src/nmp/intake/api/v2/experiments/endpoints.py`:
- Around line 389-390: The experiments sort flow is silently falling back to
name ordering when rollup hydration cannot run, so metric-based sorts can return
a misleading 200 instead of being rejected. In the endpoint that calls
_hydrate_rollups and then _sort_experiments, detect when the requested sort
field depends on rollups and hydration was skipped or unavailable (for example
when rollup_repository is None or ClickHouse is down), and fail the request
rather than continuing to sort. Use the existing _hydrate_rollups,
_sort_experiments, and sort_field handling in this endpoint to gate
rollup-backed sorts before ordering the responses.
In `@services/intake/tests/integration/spans/test_experiment_metric_sort.py`:
- Around line 18-23: Scope the integration test to isolated data so it does not
depend on shared workspace state. Update _ensure_group(), _create_experiment(),
and the list/assertion path in test_experiment_metric_sort.py to generate unique
names per run and to query experiments with experiment_group_id filtering
instead of relying on the global list. Keep the helper behavior tied to the
current test’s group so reruns cannot collide with existing groups or
experiments. Ensure the final name comparison only inspects experiments created
by this test.
---
Nitpick comments:
In `@services/intake/tests/integration/spans/test_experiment_metric_sort.py`:
- Line 6: Remove the unnecessary postponed-annotations import from
test_experiment_metric_sort since all annotated types are already imported
directly, so the module should use normal concrete type hints instead of
deferred string annotations. Update the top of the file by dropping the from
__future__ import annotations line and keep the existing direct type imports in
place; no TYPE_CHECKING-based workaround is needed.
🪄 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: 02c4079b-3569-4aa6-8284-b293a9f2afce
⛔ Files ignored due to path filters (5)
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/experiments/experiment_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiments.pyis excluded by!sdk/**web/packages/sdk/generated/agents/schema/DeploymentLogsResponse.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/tests/integration/spans/test_experiment_metric_sort.pyservices/intake/tests/test_experiment_sort.py
Contributor
|
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
callingmedic911
approved these changes
Jun 24, 2026
callingmedic911
left a comment
callingmedic911
left a comment
Member
There was a problem hiding this comment.
I think we should fail loudly than work with subset of data. Looks good otherwise!
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_sort_endpoint.py (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concrete type for
monkeypatch.Annotate
monkeypatchaspytest.MonkeyPatchto satisfy the Python typing guideline.Suggested patch
+from pytest import MonkeyPatch ... -def test_too_many_experiments_to_sort_returns_413(client: TestClient, monkeypatch) -> None: +def test_too_many_experiments_to_sort_returns_413(client: TestClient, monkeypatch: MonkeyPatch) -> None:🤖 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_sort_endpoint.py` at line 51, The test function test_too_many_experiments_to_sort_returns_413 is using an untyped monkeypatch fixture; update the function signature to annotate monkeypatch with pytest.MonkeyPatch so it follows the typing guideline. Keep the change localized to this test function and use the existing pytest import or add it if needed to reference the MonkeyPatch type.Source: Coding guidelines
🤖 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 `@services/intake/tests/test_experiment_sort_endpoint.py`:
- Around line 58-60: The test reads group_resp.json()["id"] immediately after
client.post(GROUPS, ...) without verifying the group creation succeeded. Update
the test around group_resp so it asserts the POST response is successful before
accessing id, using the existing client.post and group_resp variable in
test_experiment_sort_endpoint, so failures surface as a clear API assertion
instead of a KeyError/JSON error.
---
Nitpick comments:
In `@services/intake/tests/test_experiment_sort_endpoint.py`:
- Line 51: The test function test_too_many_experiments_to_sort_returns_413 is
using an untyped monkeypatch fixture; update the function signature to annotate
monkeypatch with pytest.MonkeyPatch so it follows the typing guideline. Keep the
change localized to this test function and use the existing pytest import or add
it if needed to reference the MonkeyPatch type.
🪄 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: f1397339-27d8-4e58-b5f3-d8935a763329
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**
📒 Files selected for processing (6)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/tests/integration/spans/test_experiment_metric_sort.pyservices/intake/tests/test_experiment_sort_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (5)
- openapi/ga/individual/platform.openapi.yaml
- openapi/ga/openapi.yaml
- openapi/openapi.yaml
- services/intake/tests/integration/spans/test_experiment_metric_sort.py
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
shanaiabuggy
enabled auto-merge
June 24, 2026 23:19
This was referenced Jun 25, 2026
This was referenced Jul 6, 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
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.
Sort the experiments list by rollup metrics (avg evaluator score, cost, latency, run count), not just entity columns.
Since those metrics live in ClickHouse and entity rows live in Postgres, the list endpoint now does a compute-on-read merge: fetch the group's experiments, hydrate their rollups, then sort/paginate in the app.
sortacceptsrun_count,cost_usd.<stat>,latency_ms.<stat>,evaluators.<name>.<stat>(stat: mean/median/p90/p95/p99/sum/count), plus the existing entity columns; - prefixes descending, missing metrics sort last.Bounded to ~1000 experiments per group (logged if exceeded) — the point at which we'd switch to denormalizing metrics for entity-store sorting instead.
Summary by CodeRabbit
sortexpression, includingrun_count,cost_usd.<stat>,latency_ms.<stat>, andevaluators.<name>.<stat>(default:-created_at).400for unsupported sort expressions,503when telemetry rollups are unavailable, and413when too many experiments are selected.