chore(intake): Deprecate intake ingest experiment_context#618
Merged
Conversation
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
📝 WalkthroughWalkthroughStandardizes evaluation identity on ChangesEvaluation identity canonicalization
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/intake/src/nmp/intake/spans/span_attribute_bags.py (1)
102-122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
nemo.experiment.metadataexcluded fromraw_attributes_json.KNOWN_BAG_KEYSdoesn’t cover it, so legacy rows that still carry this string key will surface it inraw_attributes.🤖 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/spans/span_attribute_bags.py` around lines 102 - 122, Keep nemo.experiment.metadata excluded from raw_attributes_json by updating SpanAttributeBags.raw_attributes_json to skip that legacy string key alongside atif.raw and KNOWN_BAG_KEYS. Make sure the string-attribute loop explicitly filters out nemo.experiment.metadata so it never gets added to the raw dict, while preserving the existing behavior for other string, number, and boolean attributes.
♻️ Duplicate comments (1)
openapi/openapi.yaml (1)
17830-17857: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
TraceFilterstill usesexperiment_id/test_case_id, notevaluation_id.Same gap as
Traceabove — filter parameters weren't updated to the canonical evaluation identity naming.🤖 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 17830 - 17857, Update the TraceFilter schema to use the canonical evaluation identity naming instead of the legacy experiment fields. In the TraceFilter object, replace the experiment_id and test_case_id filter properties with evaluation_id and the matching evaluation test case identifier used elsewhere in the API, and keep the descriptions/title metadata aligned with the new names. Verify the change against the Trace schema and any related filter/schema definitions so the naming is consistent across the OpenAPI spec.
🧹 Nitpick comments (1)
services/intake/src/nmp/intake/spans/ingest/evaluation_context_validation.py (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ExperimentContextbranch looks dead now.Both call sites (
atif.py,chat_completions.py) passbody.resolved_evaluation_context(), which always returnsEvaluationContext | None. Theisinstance(context, ExperimentContext)branch and the widerEvaluationContext | ExperimentContextunion onvalidate_evaluation_contextappear unreachable in practice.♻️ Optional simplification
-async def validate_evaluation_context( - *, - workspace: str, - context: EvaluationContext | ExperimentContext | None, - entity_client: EntityClient, -) -> None: +async def validate_evaluation_context( + *, + workspace: str, + context: EvaluationContext | None, + entity_client: EntityClient, +) -> None: if context is None: return - experiment_id = _evaluation_id(context) + experiment_id = context.evaluation_id🤖 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/spans/ingest/evaluation_context_validation.py` around lines 37 - 40, The `ExperimentContext` path in `_evaluation_id` is effectively unreachable because `validate_evaluation_context` is only called with `body.resolved_evaluation_context()` from `atif.py` and `chat_completions.py`, which yields `EvaluationContext | None`. Simplify `evaluation_context_validation.py` by removing the `ExperimentContext` branch from `_evaluation_id` and narrowing the `validate_evaluation_context`/`_evaluation_id` type annotations to `EvaluationContext | None`, keeping the logic aligned with the actual call sites.
🤖 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 `@services/intake/src/nmp/intake/spans/span_attribute_bags.py`:
- Around line 102-122: Keep nemo.experiment.metadata excluded from
raw_attributes_json by updating SpanAttributeBags.raw_attributes_json to skip
that legacy string key alongside atif.raw and KNOWN_BAG_KEYS. Make sure the
string-attribute loop explicitly filters out nemo.experiment.metadata so it
never gets added to the raw dict, while preserving the existing behavior for
other string, number, and boolean attributes.
---
Duplicate comments:
In `@openapi/openapi.yaml`:
- Around line 17830-17857: Update the TraceFilter schema to use the canonical
evaluation identity naming instead of the legacy experiment fields. In the
TraceFilter object, replace the experiment_id and test_case_id filter properties
with evaluation_id and the matching evaluation test case identifier used
elsewhere in the API, and keep the descriptions/title metadata aligned with the
new names. Verify the change against the Trace schema and any related
filter/schema definitions so the naming is consistent across the OpenAPI spec.
---
Nitpick comments:
In
`@services/intake/src/nmp/intake/spans/ingest/evaluation_context_validation.py`:
- Around line 37-40: The `ExperimentContext` path in `_evaluation_id` is
effectively unreachable because `validate_evaluation_context` is only called
with `body.resolved_evaluation_context()` from `atif.py` and
`chat_completions.py`, which yields `EvaluationContext | None`. Simplify
`evaluation_context_validation.py` by removing the `ExperimentContext` branch
from `_evaluation_id` and narrowing the
`validate_evaluation_context`/`_evaluation_id` type annotations to
`EvaluationContext | None`, keeping the logic aligned with the actual call
sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6dab1e02-3970-4840-b560-7a2485e73424
⛔ Files ignored due to path filters (21)
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/cli/commands/api/intake/ingest/atif.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/ingest/chat_completions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/api/intake/spans/groups.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/ingest/atif.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/intake/ingest/chat_completions.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/evaluation_context_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/experiment_context.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/experiment_context_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/ingest/atif_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/ingest/chat_completion_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/span_evaluation_context.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/span_filter_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/intake/trace.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/ingest/test_atif.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/ingest/test_chat_completions.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/spans/test_groups.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/intake/test_spans.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/ingest/atif.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/ingest/chat_completions.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/__init__.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/groups.pyservices/intake/src/nmp/intake/spans/api/spans.pyservices/intake/src/nmp/intake/spans/api/spans_schemas.pyservices/intake/src/nmp/intake/spans/ingest/atif.pyservices/intake/src/nmp/intake/spans/ingest/atif_mapping.pyservices/intake/src/nmp/intake/spans/ingest/chat_completions.pyservices/intake/src/nmp/intake/spans/ingest/evaluation_context.pyservices/intake/src/nmp/intake/spans/ingest/evaluation_context_validation.pyservices/intake/src/nmp/intake/spans/span_attribute_bags.pyservices/intake/src/nmp/intake/spans/span_attribute_catalog.pyservices/intake/src/nmp/intake/spans/span_semantic_attributes.pyservices/intake/tests/integration/spans/test_atif_ingest.pyservices/intake/tests/integration/spans/test_chat_completions_ingest.pyservices/intake/tests/integration/spans/test_traces_read.pyservices/intake/tests/test_atif_v17.pyservices/intake/tests/test_spans_schemas.py
💤 Files with no reviewable changes (7)
- services/intake/src/nmp/intake/spans/span_semantic_attributes.py
- services/intake/src/nmp/intake/spans/api/spans.py
- services/intake/tests/integration/spans/test_traces_read.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/groups.py
- services/intake/src/nmp/intake/spans/ingest/atif_mapping.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/api/intake/spans/init.py
- services/intake/src/nmp/intake/spans/span_attribute_catalog.py
Contributor
|
…adata key excluded - test_chat_completions_ingest: evaluation_context now takes precedence over the deprecated experiment_context, so the created entity lives in evaluation_context and experiment_context points at a non-existent ref to prove it is ignored (fixes the failing integration test). - span_attribute_bags: keep excluding the retired nemo.experiment.metadata key from raw_attributes so legacy rows that still carry it don't leak it, and restore the regression coverage in test_spans_schemas (CodeRabbit). - evaluation_context_validation: drop the now-dead ExperimentContext branch; resolved_evaluation_context() always yields EvaluationContext | None (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: shanaiabuggy <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/intake/src/nmp/intake/spans/span_attribute_bags.py (1)
109-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract retired key to a named constant.
"nemo.experiment.metadata"is hardcoded twice (pop + string exclusion set). A single module-level constant would avoid drift if the retired key needs updating again.🤖 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/spans/span_attribute_bags.py` around lines 109 - 115, Extract the retired span bag key into a single module-level constant and use it in both places in SpanAttributeBags so the value doesn’t drift. Update the parsed_atif_raw.pop call and the string exclusion check in the same class to reference that shared constant instead of hardcoding "nemo.experiment.metadata" twice. Keep the constant near the other bag key definitions so future retirements are easy to update.
🤖 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.
Nitpick comments:
In `@services/intake/src/nmp/intake/spans/span_attribute_bags.py`:
- Around line 109-115: Extract the retired span bag key into a single
module-level constant and use it in both places in SpanAttributeBags so the
value doesn’t drift. Update the parsed_atif_raw.pop call and the string
exclusion check in the same class to reference that shared constant instead of
hardcoding "nemo.experiment.metadata" twice. Keep the constant near the other
bag key definitions so future retirements are easy to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9d50190-04b6-49c2-9d4a-2b9da450aabd
📒 Files selected for processing (4)
services/intake/src/nmp/intake/spans/ingest/evaluation_context_validation.pyservices/intake/src/nmp/intake/spans/span_attribute_bags.pyservices/intake/tests/integration/spans/test_chat_completions_ingest.pyservices/intake/tests/test_spans_schemas.py
BrianNewsom
approved these changes
Jul 9, 2026
walston
added a commit
that referenced
this pull request
Jul 10, 2026
The intake ingest experiment_context deprecation (#618) removed evaluation_run_id, evaluation_sha, and metadata from the generated SpanEvaluationContext type. Update the evaluation context helpers and test fixture to the new shape. Signed-off-by: Nathan Walston <[email protected]>
walston
added a commit
that referenced
this pull request
Jul 10, 2026
The intake ingest experiment_context deprecation (#618) removed evaluation_run_id, evaluation_sha, and metadata from the generated SpanEvaluationContext type. Update the evaluation context helpers and test fixture to the new shape. Signed-off-by: Nathan Walston <[email protected]>
walston
added a commit
that referenced
this pull request
Jul 10, 2026
The intake ingest experiment_context deprecation (#618) removed evaluation_run_id, evaluation_sha, and metadata from the generated SpanEvaluationContext type. Update the evaluation context helpers and test fixture to the new shape. Signed-off-by: Nathan Walston <[email protected]>
walston
added a commit
that referenced
this pull request
Jul 13, 2026
The intake ingest experiment_context deprecation (#618) removed evaluation_run_id, evaluation_sha, and metadata from the generated SpanEvaluationContext type. Update the evaluation context helpers and test fixture to the new shape. Signed-off-by: Nathan Walston <[email protected]>
walston
added a commit
that referenced
this pull request
Jul 15, 2026
The intake ingest experiment_context deprecation (#618) removed evaluation_run_id, evaluation_sha, and metadata from the generated SpanEvaluationContext type. Update the evaluation context helpers and test fixture to the new shape. Signed-off-by: Nathan Walston <[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.
EvaluationContextis now{evaluation_id, test_case_id}withextra="ignore",evaluation_contextis canonical,experiment_contextdeprecated.Summary by CodeRabbit
New Features
evaluation_contextshape.evaluation_idandtest_case_idsupport across evaluation-related ingest requests and span records.Bug Fixes
experiment_contextis deprecated and treated as a fallback;evaluation_contexttakes priority when both are provided.evaluation_sha,evaluation_run_id, and metadata-style extras) are no longer returned or used for span filtering, reducing inconsistent results.Documentation
evaluation_contextbehavior and retired-field handling.