feat: report OpenAI finish reason distribution#422
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage ? 80.69%
=======================================
Files ? 133
Lines ? 17657
Branches ? 0
=======================================
Hits ? 14249
Misses ? 3408
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| TRACKED_FINISH_REASON_TOOL_CALLS = "tracked_finish_reason_tool_calls" | ||
| TRACKED_FINISH_REASON_CONTENT_FILTER = "tracked_finish_reason_content_filter" | ||
| TRACKED_FINISH_REASON_FUNCTION_CALL = "tracked_finish_reason_function_call" | ||
| TRACKED_FINISH_REASON_OTHER = "tracked_finish_reason_other" |
There was a problem hiding this comment.
Would the server disconnection (or timeout) be categorized in OTHER? that was one of the pain points of endpoints collection earlier - not sure if there's a way to expose it to help debug server. cc: @nv-alicheng
There was a problem hiding this comment.
I believe that would be handled by the accumulator or worker on a disconnect. Would be nice to integrate the errors counted there with this set in a followup.
There was a problem hiding this comment.
Agree with Rashid, if a server disconnects, then it's not finished cleanly and therefore would not have a reason from the server - that should be surfaced in the HTTP error code by accumulator / client worker.
edit for clarity - this would be surfaced in MetricsAggregator via TRACKED_SAMPLES_FAILED, which requires the accumulator to send an error event - handled by this block below in the same file:
# --- Error events ---
# Counted BEFORE the COMPLETE event (session.py emits ERROR
# first), so the tracked row still exists for tracked-failed
# detection.
if isinstance(ev, ErrorEventType):
registry.increment(MetricCounterKey.TOTAL_SAMPLES_FAILED.value)
if record.sample_uuid and table.get_row(record.sample_uuid) is not None:
registry.increment(MetricCounterKey.TRACKED_SAMPLES_FAILED.value)
logger.debug("Error event: %s", record)
continue
There was a problem hiding this comment.
The existing setup does count disconnections as failed request. Overall stats would show as:
Do yoy want me to modify this behavior in some way?
9f1c948 to
ef15278
Compare
5 participants
What does this PR do?
Capture OpenAI-compatible
finish_reasonvalues and surface their distribution in existing benchmark artifacts. This does not introduce a new artifact.Report visibility
Finish reasons are available in:
events.jsonl: the raw reason is added as a top-levelfinish_reasonfield on applicablesample.completeevents.metrics/final_snapshot.json: fixed counters recordstop,length,tool_calls,content_filter,function_call, andother.performance/result_summary.json: all supported finish reasons are exposed underfinish_reason_counts, including zero counts.report.txtis unchanged. Unknown values remain unchanged inevents.jsonland are aggregated underother. If an OpenAI-compatible response has no string finish reason, the event field is omitted and no distribution counter is incremented.Experiment data
Validated against Kimi-K2.6 served with SGLang through its OpenAI-compatible endpoint:
tool_calls: 36,length: 2stop: 1stop: 100For every experiment,
events.jsonl,metrics/final_snapshot.json, andperformance/result_summary.jsonreported matching counts for observed reasons.Type of change
Related issues
None.
Testing
1,317 passed, 5 skippedwithpytest -m unit)Checklist