Skip to content

fix(intake): Derive evaluator_results name from Harbor reward keys#416

Merged
shanaiabuggy merged 1 commit into
mainfrom
sbuggy/ase-326
Jun 23, 2026
Merged

fix(intake): Derive evaluator_results name from Harbor reward keys#416
shanaiabuggy merged 1 commit into
mainfrom
sbuggy/ase-326

Conversation

@shanaiabuggy

@shanaiabuggy shanaiabuggy commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Linear issue: ASE-326

Problem

ATIF ingest hardcodes name="harbor.verifier" for every verifier result, and _evaluator_score collapses the verifier output to a single scalar. So every experiment shows one mislabeled harbor.verifier column, and multi-criterion verifiers lose their per-criterion breakdown.

Fix

Record one evaluator result per named reward the verifier reports, using each reward's own key as the metric name, instead of collapsing everything into a single harbor.verifier row. Multi-criterion verifiers keep each criterion as its own result; a single-score verifier shows up as reward. Each result gets a stable id derived from its span and reward name, so re-importing the same data updates in place rather than creating duplicates. If a verifier reports only a bare top-level score with no named rewards, fall back to a single result named reward.

Summary by CodeRabbit

  • Bug Fixes

    • Updated evaluator results naming convention to use per-reward identifiers instead of a single fixed label.
    • Improved reward handling to emit individual evaluator results for each reward entry.
  • Tests

    • Added comprehensive tests for per-reward evaluator results emission and deduplication behavior.

@shanaiabuggy
shanaiabuggy requested review from a team as code owners June 23, 2026 18:45
@github-actions github-actions Bot added the fix label Jun 23, 2026
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 29f309ba-ddd7-4c17-8ba7-6c16b2554fe5

📥 Commits

Reviewing files that changed from the base of the PR and between 6d742a3 and 81d6a03.

📒 Files selected for processing (5)
  • services/intake/src/nmp/intake/spans/ingest/atif_mapping.py
  • services/intake/tests/integration/spans/test_evaluator_results_atif.py
  • services/intake/tests/integration/spans/test_experiment_rollups.py
  • services/intake/tests/integration/spans/test_experiment_sessions.py
  • services/intake/tests/test_experiment_rollup_repository.py

📝 Walkthrough

Walkthrough

trajectory_to_evaluator_results is rewritten to emit one EvaluatorResult per key in verifier_result.rewards, with a per-key evaluator_result_id. A new _evaluator_rewards helper extracts (name, value) pairs, falling back to ("reward", score) for a bare scalar. All test fixtures and assertions are updated from harbor.verifier to reward.

Changes

Per-reward EvaluatorResult emission

Layer / File(s) Summary
Per-reward mapping logic and helper
services/intake/src/nmp/intake/spans/ingest/atif_mapping.py
trajectory_to_evaluator_results now iterates _evaluator_rewards(...) to emit one EvaluatorResult per reward key with a per-key evaluator_result_id. New _evaluator_rewards helper extracts (name, value) pairs from verifier_result.rewards, falling back to ("reward", score) for a bare scalar.
Test updates: reward naming and new scenarios
services/intake/tests/integration/spans/test_evaluator_results_atif.py, tests/integration/spans/test_experiment_rollups.py, tests/integration/spans/test_experiment_sessions.py, tests/test_experiment_rollup_repository.py
Existing assertion updated from harbor.verifier to reward. Three new integration tests cover one-row-per-key, re-ingest deduplication, and bare-score fallback. Rollup, session, and repository unit test assertions updated to the reward key.

Possibly related PRs

  • NVIDIA-NeMo/nemo-platform#154: Rollup hydration queries and aggregate score keys depend directly on the evaluator names being changed here from harbor.verifier to reward.

Suggested reviewers

  • asutermo
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title precisely describes the main change: refactoring evaluator results naming to derive from Harbor reward keys instead of using the hardcoded 'harbor.verifier' value.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sbuggy/ase-326

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 21176/27762 76.3% 61.2%
Integration Tests 12215/26531 46.0% 19.5%

@shanaiabuggy
shanaiabuggy added this pull request to the merge queue Jun 23, 2026
Merged via the queue into main with commit 373a68c Jun 23, 2026
53 checks passed
@shanaiabuggy
shanaiabuggy deleted the sbuggy/ase-326 branch June 23, 2026 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants