refactor(evaluator): rename experiment_id to evaluation_name in publish-to-intake - #1144
refactor(evaluator): rename experiment_id to evaluation_name in publish-to-intake#1144shanaiabuggy wants to merge 1 commit into
Conversation
…sh-to-intake
`publish_to_intake` and the mapping helpers took an `experiment_id` argument
that was never an id and never an Experiment: Intake resolves the entity by
name, and after the Experiment -> Evaluation rename the thing being named is an
Evaluation. `client.experiments` now means Experiment *groups*, so the old name
pointed at the wrong entity as well as the wrong field. Passing a real entity id
gets a 400 ("Evaluation 'X' must be created before it can be logged").
Renames the argument on `publish_to_intake`, `trial_to_atif_ingest`, and
`run_task_to_evaluation_context`, and the `PublishReport.experiment_id` field,
to `evaluation_name`, and updates the docstrings to say which entity has to
exist first. The wire field stays `evaluation_id` — that is Intake's contract,
and the docstring now calls out that it carries a name despite the suffix.
No behavior change. Safe as a plain rename because the module has no production
callers yet; only its own tests use it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: shanaiabuggy <59746633+shanaiabuggy@users.noreply.github.com>
📝 WalkthroughWalkthrough
ChangesEvaluation naming migration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-evaluator/tests/intake/test_publish.py (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
PublishReport.evaluation_name.Add
assert report.evaluation_name == "exp-1"after the publish call. This verifies the renamed report field, not only the wire payload.🤖 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 `@plugins/nemo-evaluator/tests/intake/test_publish.py` at line 138, Add an assertion immediately after the publish_to_intake call verifying that report.evaluation_name equals "exp-1", so the test checks the renamed PublishReport field in addition to the wire payload.
🤖 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 `@plugins/nemo-evaluator/tests/intake/test_publish.py`:
- Line 138: Add an assertion immediately after the publish_to_intake call
verifying that report.evaluation_name equals "exp-1", so the test checks the
renamed PublishReport field in addition to the wire payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ce3239c-2f53-4295-9352-0ce2a5bea457
📒 Files selected for processing (5)
plugins/nemo-evaluator/src/nemo_evaluator/intake/mapping.pyplugins/nemo-evaluator/src/nemo_evaluator/intake/publish.pyplugins/nemo-evaluator/tests/intake/test_mapping.pyplugins/nemo-evaluator/tests/intake/test_publish.pyplugins/nemo-evaluator/tests/integration/test_publish_to_intake.py
What
publish_to_intakeand its mapping helpers took an argument calledexperiment_id. It was never an id, and after the Experiment → Evaluation rename it is not an Experiment either:validate_evaluation_contextdoesentity_client.get(Experiment, name=...)), so an id was always the wrong thing to pass. Passing one gets400 "Evaluation 'X' must be created before it can be logged".client.experimentsnow means Experiment groups, while the entity this argument names is an Evaluation (client.evaluations). The old name pointed at the wrong entity as well as the wrong field.Changes
Renamed
experiment_id→evaluation_nameon:publish_to_intake(...)mapping.trial_to_atif_ingest(...)mapping.run_task_to_evaluation_context(...)PublishReport.experiment_idfieldDocstrings updated to say which entity has to exist first and who creates it. The wire field stays
evaluation_idsince that is Intake's contract; the docstring now calls out that it carries a name despite the_idsuffix.Notes
No behavior change. Safe as a plain rename because the module has no production callers yet —
publish_to_intakeis referenced only by its own tests, so nothing outside this diff passes the keyword.experiment_ids=[group.id]in the integration test is deliberately untouched: that is the real SDK field linking an Evaluation to its Experiment groups.Verification
pytest plugins/nemo-evaluator/tests/intake→ 32 passedty checkon the changed source and tests → cleanruff check+ruff format --check→ cleanpytest plugins/nemo-evaluator/tests/integration/test_publish_to_intake.py --collect-only→ 2 collected (needs Docker to actually run)🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
Documentation
Tests