feat(studio): API to expose test case row level data in experiments#198
Merged
Conversation
shanaiabuggy
changed the title
Contributor
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-198/pr-198/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GET /apis/intake/v2/workspaces/{workspace}/experiments/{name}/sessions: OpenAPI contracts and schemas, auth mapping, ClickHouse-backed repository and SQL, shared storage coercion helpers, endpoint wiring with optional repository DI, and integration tests for pagination and filters. ChangesExperiment Sessions Listing
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: 5
🧹 Nitpick comments (1)
services/intake/tests/integration/spans/test_experiment_sessions.py (1)
86-133: ⚡ Quick winAdd integration coverage for
statusfiltering and the 503 path.Current tests miss two new endpoint behaviors:
statusquery filtering and deterministic 503 when telemetry storage is unavailable. Add both cases to lock the contract.🤖 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_sessions.py` around lines 86 - 133, Add two integration tests: one to cover status filtering and one to cover deterministic 503 when telemetry storage is unavailable. For status filtering, create sessions (using the same pattern in test_list_experiment_sessions_filter_by_test_case via client.post to ATIF_INGEST and EXPERIMENTS) with different "status" values, then call GET f"{EXPERIMENTS}/{experiment_name}/sessions" with params={"status":"<value>"} and assert pagination total_results and returned data match the filtered status. For the 503 path, add a test (similar setup to test_list_experiment_sessions_returns_404_for_unknown_experiment) that simulates telemetry storage being unavailable (mock or configure the telemetry/storage client used by the service to raise or be None) before calling the sessions endpoint and assert the response.status_code == 503 and appropriate error shape; reference the TestClient, EXPERIMENTS, ATIF_INGEST, and the existing test patterns for setup and assertions.
🤖 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 15425-15429: The schema for the test_case_id property currently
disallows nulls but the description says it may be null; update the OpenAPI
schema for the test_case_id property (the YAML block defining test_case_id) to
include nullable: true so null values are accepted, keeping the existing title,
description, and type entries unchanged.
In `@openapi/ga/openapi.yaml`:
- Around line 15425-15429: The OpenAPI schema for the property test_case_id
declares it can be null in the description but omits the nullable marker; update
the test_case_id property in openapi.yaml to allow nulls by adding nullable:
true beneath the test_case_id definition (keeping title, description and type
intact) so clients will accept null values for test_case_id.
- Around line 15441-15443: Update the ExperimentSessionResponse schema in
openapi.yaml to mark fields that the backend can return as null as nullable:
true; specifically add nullable: true for ended_at, latency_ms, input,
input_tokens, output_tokens, cached_tokens, and cost_total_usd in the
ExperimentSessionResponse object so the OpenAPI spec accepts nulls consistent
with services/intake/src/nmp/intake/spans/experiment_session_repository.py.
In `@openapi/openapi.yaml`:
- Around line 15425-15430: The schema for the property test_case_id currently
lists only type: string but the description states it can be null; update the
test_case_id schema to allow nulls consistently with other fields by adding
nullable: true (or changing the type to a string/null union if your style
prefers) under the test_case_id entry in openapi/openapi.yaml so the spec and
description match.
In `@services/intake/src/nmp/intake/api/v2/experiments/endpoints.py`:
- Around line 432-439: Wrap the call to session_repository.list_sessions in a
try/except that catches ClickHouse-related backend exceptions and converts them
into an HTTP 503 response; specifically, around the block calling
session_repository.list_sessions capture exceptions from the ClickHouse client
(e.g., clickhouse_driver.errors.Error / connection-related exceptions) and raise
fastapi.HTTPException(status_code=503, detail="Telemetry store unavailable")
while logging the original exception for debugging. Ensure you import
fastapi.HTTPException (or use the existing FastAPI HTTPException) and only map
backend/unavailability errors to 503, re-raising other exceptions unchanged.
---
Nitpick comments:
In `@services/intake/tests/integration/spans/test_experiment_sessions.py`:
- Around line 86-133: Add two integration tests: one to cover status filtering
and one to cover deterministic 503 when telemetry storage is unavailable. For
status filtering, create sessions (using the same pattern in
test_list_experiment_sessions_filter_by_test_case via client.post to ATIF_INGEST
and EXPERIMENTS) with different "status" values, then call GET
f"{EXPERIMENTS}/{experiment_name}/sessions" with params={"status":"<value>"} and
assert pagination total_results and returned data match the filtered status. For
the 503 path, add a test (similar setup to
test_list_experiment_sessions_returns_404_for_unknown_experiment) that simulates
telemetry storage being unavailable (mock or configure the telemetry/storage
client used by the service to raise or be None) before calling the sessions
endpoint and assert the response.status_code == 503 and appropriate error shape;
reference the TestClient, EXPERIMENTS, ATIF_INGEST, and the existing test
patterns for setup and assertions.
🪄 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: c63875b3-6333-457d-a1e8-fca92fe0229f
⛔ Files ignored due to path filters (12)
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/experiments/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/sessions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_responses_page.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/session_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/experiments/test_sessions.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/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.pyservices/intake/src/nmp/intake/spans/experiment_session_repository.pyservices/intake/tests/integration/spans/test_experiment_sessions.py
Contributor
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openapi/openapi.yaml (1)
15436-15439: ⚡ Quick winAdd enum constraint for status field.
Backend validates
statusagainstSpanStatusenum (success, error, cancelled, unknown) but OpenAPI schema only declarestype: string. Add enum to improve contract and enable client-side validation.📋 Suggested fix
status: description: Filter by root-span status (success, error, cancelled, unknown). title: Status type: string + enum: + - success + - error + - cancelled + - unknown🤖 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 15436 - 15439, The OpenAPI schema currently defines the "status" property as type: string but lacks the enum constraint; update the "status" field in openapi/openapi.yaml to include an enum with the allowed values matching backend SpanStatus ("success", "error", "cancelled", "unknown") so clients can validate against the same contract; locate the "status" property block (title: Status, description: Filter by root-span status) and add enum: ["success","error","cancelled","unknown"] under it.
🤖 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/src/nmp/intake/spans/experiment_session_repository.py`:
- Around line 260-267: The current queries built via current_spans_sql and the
CTE current_page_spans (and related joins against span_versions / spans_table
and page_sessions) only constrain by (workspace, session_id), which allows
reused session_id to mix unrelated traces; update those WHERE/ON predicates and
any IN-subselects to also include trace identity (e.g., trace_id or
root_span_id) so joins are scoped by (workspace, session_id, trace_id). Find
usages of current_spans_sql, current_page_spans, and any joins against
span_versions/page_sessions (including the other occurrences around the same
block) and add the trace_id equality to the tuple predicates and join conditions
so aggregates filter by trace as well as session and workspace.
---
Nitpick comments:
In `@openapi/openapi.yaml`:
- Around line 15436-15439: The OpenAPI schema currently defines the "status"
property as type: string but lacks the enum constraint; update the "status"
field in openapi/openapi.yaml to include an enum with the allowed values
matching backend SpanStatus ("success", "error", "cancelled", "unknown") so
clients can validate against the same contract; locate the "status" property
block (title: Status, description: Filter by root-span status) and add enum:
["success","error","cancelled","unknown"] under it.
🪄 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: e84bab8d-331c-4211-ac3f-68e86d7a3fc4
⛔ Files ignored due to path filters (10)
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/experiments/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/sessions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/session_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/experiments/test_sessions.pyis excluded by!sdk/**sdk/stainless.yamlis 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/spans/experiment_rollup_repository.pyservices/intake/src/nmp/intake/spans/experiment_session_repository.pyservices/intake/src/nmp/intake/spans/storage.pyservices/intake/src/nmp/intake/spans/trace_repository.pyservices/intake/tests/integration/spans/test_experiment_sessions.py
🚧 Files skipped from review as they are similar to previous changes (5)
- services/intake/src/nmp/intake/api/v2/experiments/schemas.py
- openapi/ga/individual/platform.openapi.yaml
- openapi/ga/openapi.yaml
- services/intake/tests/integration/spans/test_experiment_sessions.py
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
force-pushed
the
sbuggy/fp-201
branch
from
June 8, 2026 20:39
53e5c75 to
e49c2f1
Compare
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/integration/spans/test_experiment_sessions.py`:
- Around line 127-130: Tests call
client.get("{EXPERIMENTS}/{experiment_name}/sessions", params=...) with
top-level keys like "test_case_id" and "status" which fails validation; change
the request params to use deep-object filter syntax expected by
validate_list_query_params (e.g., pass params with keys like
"filter[test_case_id]" and "filter[status]" instead of top-level
"test_case_id"/"status" in the client.get calls in this test file), updating
both the call around the EXPERIMENTS/experiment_name/sessions endpoint (the
client.get invocation) for the lines referenced and the similar invocation at
lines ~176-179 so the endpoint receives filter[...] params and returns 200.
- Line 6: Remove the module-level future import by deleting the line "from
__future__ import annotations" from the test module, and replace any remaining
string-based type hints in functions/classes in this file with concrete type
hints (or import the actual types under normal imports) so the code no longer
relies on postponed evaluation of annotations; verify tests still pass after
converting any quoted annotations.
🪄 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: c900e298-bf11-4ad1-9b60-4bedb012f977
⛔ Files ignored due to path filters (13)
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/experiments/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/sessions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_session_responses_page.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/session_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/experiments/test_sessions.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (11)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.pyservices/intake/src/nmp/intake/spans/experiment_rollup_repository.pyservices/intake/src/nmp/intake/spans/experiment_session_repository.pyservices/intake/src/nmp/intake/spans/storage.pyservices/intake/src/nmp/intake/spans/trace_repository.pyservices/intake/tests/integration/spans/test_experiment_sessions.py
🚧 Files skipped from review as they are similar to previous changes (10)
- services/core/auth/src/nmp/core/auth/assets/static-authz.yaml
- openapi/openapi.yaml
- services/intake/src/nmp/intake/api/v2/experiments/schemas.py
- services/intake/src/nmp/intake/spans/trace_repository.py
- openapi/ga/openapi.yaml
- services/intake/src/nmp/intake/spans/storage.py
- openapi/ga/individual/platform.openapi.yaml
- services/intake/src/nmp/intake/spans/experiment_session_repository.py
- services/intake/src/nmp/intake/api/v2/experiments/endpoints.py
- services/intake/src/nmp/intake/spans/experiment_rollup_repository.py
BrianNewsom
approved these changes
Jun 8, 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.
Summary by CodeRabbit
New Features
Tests