Skip to content

Bug Fix - Fix NVBandwidth benchmark results parsing bug (#748) #782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
NJX-njx wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −3
Open

Bug Fix - Fix NVBandwidth benchmark results parsing bug (#748) #782

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions superbench/benchmarks/micro_benchmarks/nvbandwidth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,17 @@ def _process_raw_line(self, line, parse_status):

# Parse summary results
if self.re_summary_pattern.match(line):
value = self.re_summary_pattern.match(line).group(2)
test_name = parse_status['test_name']
match = self.re_summary_pattern.match(line)
value = match.group(2)
Comment on lines 192 to +194
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re_summary_pattern.match(line) is evaluated twice (once in the if and again to assign match). Consider assigning once (e.g., match = ...; if match:) to avoid duplicated work and keep the control flow simpler.

Copilot uses AI. Check for mistakes.
# Use test_name from parse_status, fallback to group(1) from SUM line
test_name = parse_status['test_name'] or match.group(1).lower()
benchmark_type = parse_status['benchmark_type']
parse_status['results'][f'{test_name}_sum_{benchmark_type}'] = float(value)
# Infer benchmark_type from test_name if not set (e.g., after waived tests)
if benchmark_type is None:
benchmark_type = 'lat' if 'latency' in test_name else 'bw'
# Only add result when we have valid metric name (avoid _sum_None or sum_None)
if test_name and benchmark_type:
parse_status['results'][f'{test_name}_sum_{benchmark_type}'] = float(value)
Comment on lines +195 to +203
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new fallback/inference paths for SUM parsing (using match.group(1) when parse_status['test_name'] is empty, and inferring benchmark_type when it's None) aren’t covered by existing nvbandwidth tests (current tests only exercise the full header+matrix path). Adding a unit test with a minimal raw output containing a SUM ... line but no prior matrix header (and/or no preceding Running ...) would help prevent regressions of the _sum_None metric-name bug.

Copilot uses AI. Check for mistakes.

# Reset parsing state for next test
parse_status['test_name'] = ''
Expand Down
Loading