Repository navigation
[staging] Add per-turn metrics visualization for multi-turn benchmarks - #104
Merged
Harshith-umesh merged 17 commits intoOct 7, 2026
Merged
Harshith-umesh merged 17 commits into
Harshith-umesh merged 17 commits into
Conversation
Forge PR #215 adds per-turn CSV rows alongside aggregate rows for multi-turn benchmarks. The `turn` column is empty on aggregate rows and 0/1/2/… on per-turn breakdown rows. Without this filter, per-turn rows would be mixed into _combos() grouping and produce duplicate concurrency points, inflated request counts, and skewed percentiles. Filter keeps only aggregate rows (turn is empty/NaN). CSVs without a turn column are unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When per-turn rows are present (from Forge PR #215), a "Turn (Multi-turn)" option appears in the Select X-Axis dropdown. Selecting it plots turn_index on x-axis vs any Y-axis metric, with one line per concurrency level. "Show concurrency up to" filters which lines appear, same as for Concurrency. Replaces the earlier render_per_turn_breakdown expander with native integration into the existing plot controls. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Correctness: - filtered_per_turn_df now uses a shared _apply_filters() function that applies all 10 sidebar filters (was missing TP, profile, spec_decoding, DP, dataset, prefix_caching, and multi-turn masks) - per_turn_plot_df run_identifier now includes DP/spec_decoding/prefix_caching suffixes so per-turn legend labels match the main-chart labels - errored_requests/successful_requests column access now guarded; KeyError no longer crashes data prep for older CSV schemas - turn_index astype(float) replaced with pd.to_numeric(errors='coerce') to handle non-numeric strings like 'N/A' without crashing - efficiency_ratio inf guard: replace [inf, -inf] with NaN for TP=0 rows - output_tok/sec column access now guarded for latency-only schemas Cleanup: - Extract _is_turn_view boolean; evaluated once before all branches use it Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Remove orphaned mask variables (tp_mask, dp_mask, custom_mask, etc.) left behind after refactoring to _apply_filters() — ruff F841 flagged all 9 as assigned but unused. Run ruff format to fix formatting. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add turns/prefix_tokens/prefix_count suffixes to per_turn_plot_df run_identifier so it fully matches filtered_df's run_identifier format. All six configuration dimensions now included: DP, spec_decoding, prefix_caching, turns, prefix_tokens, prefix_count. Addresses coderabbitai comment at line 3845. The filtered_per_turn_df filter comment is already fixed by _apply_filters(). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fix turn_index staying float64: filter invalid rows before astype(int) instead of using reindex() which re-introduces NaN and forces float column. Turn labels now show 0/1/2 instead of 0.0/1.0/2.0. Suggested by coderabbitai (comment 4146691396). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When Turn (Multi-turn) is selected as x-axis, col3 now shows a Concurrency multiselect (defaulting to the highest value) instead of the "Show concurrency up to" selectbox. This lets engineers study one concurrency at a time while making it easy to add more for comparison. Also fix empty-list isin() bug for mt_turns/mt_prefix_tokens/ mt_prefix_count sidebar filters — empty selection now means no filter (pass-through) instead of filtering out all data. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Guard per_turn_df["turns"] against missing column using .get() with default Series(1), matching the pattern already used for prefix_tokens, prefix_count, spec_decoding, and prefix_caching. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add pp_turn_conc → perf_plots_turn_concurrency to SECTION_FILTER_KEYS and MULTISELECT_SESSION_KEYS (with NUMERIC_LIST for int parsing) so the selected concurrency level(s) survive copy-paste URL sharing. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
NUMERIC_LIST_SESSION_KEYS converts to float, but the Concurrency multiselect options are integers. Add INT_LIST_SESSION_KEYS and move perf_plots_turn_concurrency there so URL-restored selections correctly match the int options in the widget. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@st.fragment reruns dont trigger the parent encode_filters_to_url, so the concurrency selection was never written to st.query_params. Update it inline after the multiselect renders. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
pp_x, pp_y, pp_conc, and pp_turn_conc are all written directly to st.query_params after the chart renders, since @st.fragment reruns do not fire the parent from_dict call. This ensures the URL bar stays accurate for copy-paste sharing when any of these widgets change. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add turn_index as empty string for aggregate rows so manually-imported CSVs are schema-compatible with the dashboard's per-turn logic (PR openshift-psap#101). Per-turn rows (turn_index=0/1/2) are generated by the Forge pipeline when request-level turn data is preserved in the benchmark JSON. Update README: 52 → 53 columns, add turn_index to column table (openshift-psap#7), clarify that manual import produces aggregate rows only. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Row numbering was corrupted by the previous renumber script — TP and measured concurrency both landed at row 9. Fix: turn_index=7, TP=8, measured concurrency=9, continuing sequentially to request_type=53. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The script emits turn_index right before turns in the output dict. The README should reflect actual output order, not the RHAIIS schema field position (which pandas aligns by name on concat anyway). turn_index: openshift-psap#49, turns: openshift-psap#50, prefix_tokens: openshift-psap#51, prefix_count: openshift-psap#52, request_type: openshift-psap#53. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Correctness:
- Guard stale perf_plots_turn_concurrency session state: filter stored
list to only still-valid values, fall back to max if all stale
(previously raised StreamlitInvalidValueError when data reloads)
- Add .fillna("?") to run_identifier string concat for accelerator/
model/version, preventing NaN color labels in plotly turn chart
- Guard else-branch when x_axis='turn_index' but per_turn_plot_df is
empty — show info message instead of sorting aggregate df by NaN
turn_index column
- Narrow broad contextlib.suppress(Exception) in URL push block to a
try/except with a pass, keeping the intent but letting TypeError/
AttributeError from bad values surface during development
Simplification:
- Factor _str_norm into a proper function shared by both per_turn_df
and df preprocessing paths — eliminates duplicated logic
- Remove redundant reset_index(drop=True, inplace=True) in filtered
data section (concat branch already ends with .reset_index)
- Remove double per_turn_plot_df.copy() — plot_label can be assigned
directly (per_turn_df.copy() earlier in the block is sufficient)
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Harshith-umesh
approved these changes
Oct 7, 2026
Harshith-umesh
left a comment
Harshith-umesh
left a comment
Member
There was a problem hiding this comment.
Lgtm, but the manual runs fix needs to be extended based on our discussion.
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.
Summary
Staging variant of #101 — cherry-picks the same multi-turn per-turn metrics changes onto the
stagingbranch.Companion to openshift-psap/forge#215.
Changes (same as #101)
_combos()grouping; string normalization and column guards (includingturns) applied toper_turn_dfturn_indexhidden when off_apply_filters(d)closure, full run_identifier suffixes, empty-list filter fix for turns/prefix_tokens/prefix_countBackward compatibility
CSVs without
turn_indexare completely unaffected.Validated with
Forge CI run on H200 (Qwen3-0.6B, 3 turns): 3 aggregate + 9 per-turn rows end-to-end.
🤖 Generated with Claude Code