Benchmarks: Micro benchmark - add nvbench based kernel-launch, sleep-kernel & auto-throughput - #750
Open
WenqingLan1 wants to merge 57 commits into
Open
Benchmarks: Micro benchmark - add nvbench based kernel-launch, sleep-kernel & auto-throughput#750WenqingLan1 wants to merge 57 commits into
WenqingLan1 wants to merge 57 commits into
Conversation
WenqingLan1
added
benchmarks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
+ Coverage 86.02% 86.29% +0.27%
==========================================
Files 103 107 +4
Lines 7950 8225 +275
==========================================
+ Hits 6839 7098 +259
- Misses 1111 1127 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
superbench/benchmarks/micro_benchmarks/nvbench_base.py:19
- parse_time_to_us() doesn't handle the microsecond symbols "μs" / "µs" even though the NVBench parsers' regexes accept them. If NVBench emits "μs" in table output, parsing will fail with ValueError and the benchmark will report MICROBENCHMARK_RESULT_PARSING_FAILURE. Consider normalizing "μ"/"µ" to "u" before unit handling.
raw = raw.strip()
m = re.match(r'^([\d.]+)\s*([mun]?s)?$', raw)
if not m:
raise ValueError(f'Invalid time string: {raw!r}')
val, unit = float(m.group(1)), (m.group(2) or 'us')
.github/workflows/codeql-analysis.yml:59
- The workflow uses
lukka/get-cmake@latest, which is not version-pinned and can change behavior over time (supply-chain/reproducibility risk). Pin to a specific tag or commit SHA so CodeQL runs remain deterministic.
- name: Setup CMake
uses: lukka/get-cmake@latest
with:
cmakeVersion: '3.20.0'
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/codeql-analysis.yml:59
- The CodeQL C++ job pins CMake to 3.20.0, but the new nvbench micro-benchmarks explicitly skip configuration/build when CMake < 3.30.4 (see superbench/benchmarks/micro_benchmarks/nvbench/CMakeLists.txt). As a result, the new CUDA/C++ nvbench sources won't be built (and therefore won't be included in CodeQL analysis) in this workflow. Also,
lukka/get-cmake@latestis not pinned, which is brittle and a supply-chain risk for CI.
- name: Setup CMake
uses: lukka/get-cmake@latest
with:
cmakeVersion: '3.20.0'
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
superbench/benchmarks/micro_benchmarks/init.py:55
- This change reformats the entire
__all__list into a few very long lines, creating avoidable diff churn and making future edits harder. Keep the previous one-item-per-line style (and just append the new NVBench exports).
__all__ = [
'BlasLtBaseBenchmark', 'ComputationCommunicationOverlap', 'CpuMemBwLatencyBenchmark', 'CpuHplBenchmark',
'CpuStreamBenchmark', 'CublasBenchmark', 'CublasLtBenchmark', 'CudaGemmFlopsBenchmark', 'CudaMemBwBenchmark',
'CudaNcclBwBenchmark', 'CudnnBenchmark', 'DiskBenchmark', 'DistInference', 'HipBlasLtBenchmark', 'GPCNetBenchmark',
'GemmFlopsBenchmark', 'GpuBurnBenchmark', 'GpuCopyBwBenchmark', 'GpuStreamBenchmark', 'IBBenchmark',
'IBLoopbackBenchmark', 'KernelLaunch', 'MemBwBenchmark', 'MicroBenchmark', 'MicroBenchmarkWithInvoke',
'ORTInferenceBenchmark', 'RocmGemmFlopsBenchmark', 'RocmMemBwBenchmark', 'ShardingMatmul',
'TCPConnectivityBenchmark', 'TensorRTInferenceBenchmark', 'DirectXGPUEncodingLatency', 'DirectXGPUCopyBw',
'DirectXGPUMemBw', 'DirectXGPUCoreFlops', 'NvBandwidthBenchmark', 'NvbenchKernelLaunch', 'NvbenchSleepKernel',
'NvbenchAutoThroughput'
superbench/benchmarks/micro_benchmarks/nvbench_base.py:265
- NVBench JSON output paths are appended to the command without shell-quoting. Since
run_commandexecutes withshell=Trueby default, a user-supplied--output_dir(or even paths with spaces) can break the command or be used for command injection. Shell-quote the computedjson_pathbefore appending.
for idx, command in enumerate(commands):
json_path = os.path.join(base_dir, f'{self._bin_name}_{idx}.json')
self._json_paths.append(json_path)
finalized.append(f'{command} --json {json_path}')
return finalized
| return val | ||
|
|
||
|
|
||
| _NVBENCH_INT_VALUES_PATTERN = re.compile( |
Contributor
There was a problem hiding this comment.
To pass the lint check, pls use 1 line here.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (3)
superbench/benchmarks/micro_benchmarks/nvbench_auto_throughput.py:41
--block_sizeusesparse_nvbench_int_values, which also accepts NVBench range formats (e.g.[128:1024]/[128:1024:128]), but the help text only documents single-value and list formats. Please either document the supported range formats or restrict parsing for this argument.
self._parser.add_argument(
'--block_size',
type=parse_nvbench_int_values,
default='[128,256,512,1024]',
help='Block size (threads per block). Supports: "256" (single), "[128,256,512,1024]" (list).',
superbench/benchmarks/micro_benchmarks/nvbench_base.py:23
parse_time_to_usallows multiple dots in the numeric portion via([\d.]+), so inputs like"1..2 us"match the regex but then fail with a genericfloat()conversion error. Tightening the regex and normalizing the error makes invalid inputs fail consistently withInvalid time string: ....
raw = raw.strip()
m = re.match(r'^([\d.]+)\s*([mun]?s)?$', raw)
if not m:
raise ValueError(f'Invalid time string: {raw!r}')
val, unit = float(m.group(1)), (m.group(2) or 'us')
superbench/benchmarks/micro_benchmarks/nvbench_sleep_kernel.py:52
- The
_process_raw_resultdocstring saysself._result.add_raw_data()needs to be called, but raw JSON is already recorded byNvbenchBase._load_result_json(). This is misleading for anyone implementing new NVBench benchmarks based on this example.
"""Function to parse raw results and save the summarized results.
self._result.add_raw_data() and self._result.add_result() need to be called to save the results.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
superbench/benchmarks/micro_benchmarks/nvbench_base.py:44
parse_nvbench_int_valuescurrently accepts whitespace inside bracketed list/range forms (e.g. "[0, 1]") and returns the value unchanged. That can later produce invalid CLI tokens (especially for--devices) if spaces are present, and also makes it easier to accidentally generate commands that won’t parse as intended. Consider normalizing by stripping all whitespace after validation and returning the normalized value.
def parse_nvbench_int_values(value):
"""Validate an NVBench integer value specification."""
# Accepted formats: '0', '[0,1,2]', '[0:4]', and '[0:4:2]' (range with step).
if not _NVBENCH_INT_VALUES_PATTERN.fullmatch(value):
raise ValueError(
'Invalid NVBench integer values. Use a single value like "0", '
'a list like "[0,1,2]", or a range like "[0:4]" or "[0:4:2]".'
)
return value
superbench/benchmarks/micro_benchmarks/nvbench_base.py:184
--devicesvalues like "[0,1,2]" / "[0:4]" contain shell glob metacharacters ([and]). Since commands are executed withshell=True(seerun_command), passing these unquoted can trigger glob expansion based on the working directory contents, producing unexpected arguments. Quoting/escaping the--devicesvalue when building the command would make behavior deterministic.
def _add_device_args(self, parts):
"""Add device configuration arguments to command parts."""
if hasattr(self._args, 'devices') and self._args.devices is not None:
if self._args.devices == 'all':
parts.extend(['--devices', 'all'])
else:
parts.extend(['--devices', self._args.devices])
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
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.
This pull request adds support for NVBench-based GPU micro-benchmarks to SuperBench.
nvbench-sleep-kernelnvbench-kernel-launchnvbench-auto-throughputExample config: