Add autodl metrics for Claude eval jobs - #83023
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe evaluation step parses Claude stream data and harness results into AutoDL session metrics. It captures per-evaluation stream logs, handles missing inputs, writes metrics atomically, and documents the new outputs. ChangesClaude evaluation metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Evaluation
participant Claude
participant Harness
participant write_eval_metrics
participant SharedMetrics
Evaluation->>Claude: capture evaluation stream log
Evaluation->>Harness: locate matching run_result.json
Evaluation->>write_eval_metrics: provide log and harness result
write_eval_metrics->>SharedMetrics: atomically append AutoDL metrics
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.sh (1)
223-224: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider recording models with zero reported cost.
Line 223 drops any model whose
cost_usdis zero or missing. A model that consumed tokens but has no price entry in the harness produces no row. The token totals for that model are then lost from the aggregate.If the intent is only to suppress fully idle models, test the token counts instead of the cost.
🤖 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 `@ci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.sh` around lines 223 - 224, Update the filtering condition in the model-recording loop to skip only fully idle models by checking their token counts, rather than treating zero or missing cost_usd as grounds for exclusion. Preserve records for models that consumed tokens even when their reported cost is zero or unavailable.
🤖 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
`@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh`:
- Around line 240-249: Update the Claude pipeline around the timeout/tee
invocation to preserve the exit status from claude or timeout rather than tee.
Enable pipefail for the script or explicitly capture PIPESTATUS[0], while
retaining the assignment to THIS_EXIT so evaluation failures are propagated.
In
`@ci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.sh`:
- Line 44: Contain all metrics-extraction failures in extract_session_metrics so
they cannot prevent continue-session page generation: at
ci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.sh
lines 44-44, wrap the eval-harness python3 heredoc in an if ! block that emits a
warning; at lines 279-295, similarly guard the merge heredoc and row_count read,
assigning row_count a fallback when the read fails.
- Around line 202-218: Handle null or non-dictionary values when reading
per_model_turns and eval_params in the result-processing flow: normalize each to
an empty dictionary before calling values() or get(). Keep the existing
aggregation and prompt-generation behavior unchanged for valid dictionaries,
matching the defensive pattern already used for per_case.
---
Nitpick comments:
In
`@ci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.sh`:
- Around line 223-224: Update the filtering condition in the model-recording
loop to skip only fully idle models by checking their token counts, rather than
treating zero or missing cost_usd as grounds for exclusion. Preserve records for
models that consumed tokens even when their reported cost is zero or
unavailable.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fbab831f-7e0f-469c-b1d6-0072e26667ed
📒 Files selected for processing (3)
ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.shci-operator/step-registry/openshift/claude/post/openshift-claude-post-commands.shci-operator/step-registry/openshift/claude/post/openshift-claude-post-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh (2)
377-381: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winZero-cost models are dropped with their token counts.
Line 380 skips any model whose
cost_usdis missing or zero. If a harness result reports token usage but no cost (for example, a cached or free-tier model, or a schema that omitscost_usd), the token data never reaches AutoDL. Consider emitting the row when token counts are non-zero, and skipping only fully empty entries.♻️ Proposed change
for model, usage in usages.items(): turns = int(model_turns.get(model, 0) or 0) cost = float(usage.get("cost_usd", 0) or 0) - if cost <= 0: - continue + tokens = sum( + int(usage.get(key, 0) or 0) + for key in ("input", "output", "cache_read", "cache_creation") + ) + if cost <= 0 and tokens <= 0: + continue🤖 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 `@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh` around lines 377 - 381, Update the usage-row filtering in the usages loop to skip only entries with both zero cost and zero token turns. Preserve rows when turns is non-zero even if cost_usd is missing or zero, while continuing to omit fully empty entries.
427-434: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHarden the append path against a foreign or truncated artifact.
Line 431 assumes the existing file contains a
rowslist that matchesmetrics.SCHEMA. A file written by a different producer, or a truncated file, raisesKeyErroror silently mixes two schemas in one table. Usesetdefaultfor the key and validate the schema before extending.The atomic write with
with_suffixplusreplaceat Lines 435-438 is correct.♻️ Proposed change
output = pathlib.Path(output_path) -if output.is_file(): - with output.open() as stream: - document = json.load(stream) - document["rows"].extend(rows) -else: +document = None +if output.is_file(): + try: + with output.open() as stream: + document = json.load(stream) + except (json.JSONDecodeError, OSError): + document = None +if isinstance(document, dict) and document.get("schema") == metrics.SCHEMA: + document.setdefault("rows", []).extend(rows) +else: document = metrics.build_autodl(rows[0]) document["rows"] = rows🤖 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 `@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh` around lines 427 - 434, Harden the existing-file branch around document["rows"]: use setdefault to ensure the rows key exists, then validate the loaded document against metrics.SCHEMA before extending it. Reject foreign or truncated artifacts rather than appending incompatible rows, while preserving the current atomic write behavior.
🤖 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
`@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh`:
- Around line 321-322: Update the ttft_ms and num_turns mappings in the
result-row construction to use the same null-safe `or 0` fallback as the other
numeric fields, while preserving the is_primary conditional so non-primary
results remain zero.
---
Nitpick comments:
In
`@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh`:
- Around line 377-381: Update the usage-row filtering in the usages loop to skip
only entries with both zero cost and zero token turns. Preserve rows when turns
is non-zero even if cost_usd is missing or zero, while continuing to omit fully
empty entries.
- Around line 427-434: Harden the existing-file branch around document["rows"]:
use setdefault to ensure the rows key exists, then validate the loaded document
against metrics.SCHEMA before extending it. Reject foreign or truncated
artifacts rather than appending incompatible rows, while preserving the current
atomic write behavior.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e158f09c-70d6-47f2-a94e-a957618d77de
📒 Files selected for processing (2)
ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.shci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-ref.yaml
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.sh`:
- Around line 376-384: The per-model row construction must not substitute
aggregate turns when per_model_turns exists but a model’s count is zero or
missing. Update the turns fallback near the usages loop and the result.num_turns
handling around the per-model metric creation so aggregate turns are used only
when per_model_turns is absent and exactly one usage model exists; otherwise
preserve zero or the model-specific count.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9c071bea-27f8-4589-a4a7-ba2686284169
📒 Files selected for processing (2)
ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-commands.shci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/openshift/claude/agent-eval/openshift-claude-agent-eval-ref.yaml
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse pull-ci-openshift-eng-ai-helpers-main-eval-payload-analysis-minimal |
|
@stbenjam: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@stbenjam: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@stbenjam: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Summary
claude-session-metrics-autodl.jsondirectly in the Claude eval step's artifact directory/eval-runorchestrator and the much larger per-model agent-eval-harness spendSHARED_DIRentirely for metrics, so the 1 MiB shared-data limit is not involvedThe edge-tooling eval command is a symlink to the shared OpenShift Claude eval command, so it receives the same coverage.
Why
Eval jobs retain enough data to measure spend but do not currently publish a session-metrics AutoDL artifact. A 30-day artifact audit recovered $2,452.71 from the affected jobs; $2,328.28 was in harness case runs and only $124.44 was in the top-level orchestrators.
Validation
bash -nmake registry-metadatamake ci-operator-checkconfigThe separate
openshift-api-evalworkflow is not covered because its historical jobs retained neither Claude stream logs nor agent-eval-harness run results.Summary by CodeRabbit
This PR adds AutoDL metrics generation to Claude evaluation jobs in OpenShift CI.
claude-session-metrics-autodl.json./eval-runorchestrator spend from per-model harness spend.SHARED_DIR.openshift-api-evalworkflow remains unvalidated because it lacks retained Claude logs and harness results.