Allow using cuML benchmark tools on systems without cuML installed#7593
Merged
rapids-bot[bot] merged 24 commits intorapidsai:mainfrom Mar 3, 2026
Merged
Allow using cuML benchmark tools on systems without cuML installed#7593rapids-bot[bot] merged 24 commits intorapidsai:mainfrom
rapids-bot[bot] merged 24 commits intorapidsai:mainfrom
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
csadorf
reviewed
Dec 11, 2025
Contributor
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
I have a few suggestions for minor improvements, but overall this looks like a good improvement in function.
I think that we should tighten the API of the run_benchmarks CLI in a follow-up.
viclafargue
reviewed
Dec 15, 2025
Contributor
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks @dantegd! It looks good to me, here are just a few things I noticed.
Contributor
|
Any chance we can revive this PR? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded GPU/cuML availability detection and optional GPU paths across the benchmark package, including a package entry point, guarded imports with CPU fallbacks, refactored data generation and algorithm registry, modularized benchmark execution, updated runners to respect GPU availability, and added documentation for full and standalone modes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
python/cuml/cuml/benchmark/bench_helper_funcs.py (3)
240-268:⚠️ Potential issue | 🟠 MajorPre-existing bug:
returninside the outerfor layoutloop causes early exit after first layout.The
returnat Line 264 is indented inside thefor layout in allowed_layout_types:loop, so only"breadth_first"is ever tested —"depth_first"and"layered"are skipped. The return should be outside the loop to test all layouts.Proposed fix (dedent return)
fil_kwargs["layout"] = optimal_layout - return OptimizedFilWrapper( - m.load(model_path, **fil_kwargs), - optimal_chunk_size, - infer_type=infer_type, - ) + fil_kwargs["layout"] = optimal_layout + + return OptimizedFilWrapper( + m.load(model_path, **fil_kwargs), + optimal_chunk_size, + infer_type=infer_type, + )
313-313:⚠️ Potential issue | 🟡 MinorResource leak and unsafe deserialization via
pickle.
open(model_path, "wb")on Line 313 andopen(model_path, "rb")on Line 342 are never explicitly closed. Use awithstatement. Also,pickle.loadon untrusted data is flagged by the coding guidelines for unsafe deserialization.Proposed fix for the write path (line 313)
- pickle.dump(skl_model, open(model_path, "wb")) + with open(model_path, "wb") as f: + pickle.dump(skl_model, f)Proposed fix for the read path (line 342)
- skl_model = pickle.load(open(model_path, "rb")) + with open(model_path, "rb") as f: + skl_model = pickle.load(f) # noqa: S301 - trusted local model file
419-426:⚠️ Potential issue | 🟡 Minor
local_datamay be undefined ifdatais not a tuple/list.Line 421 references
local_dataoutside theif isinstance(data, (tuple, list)):guard at Line 419. Ifdatais not a tuple or list, this will raiseNameError. This appears to be a pre-existing issue, but since the surrounding function was modified in this PR, consider fixing it.Proposed fix
if isinstance(data, (tuple, list)): local_data = [x.compute() for x in data if x is not None] - if len(local_data) == 2: + else: + local_data = [data] + if len(local_data) == 2:python/cuml/cuml/benchmark/datagen.py (1)
560-561:⚠️ Potential issue | 🟡 Minor
@functools.lru_cacheongen_datawith mutable default and**kwargs.
lru_cacherequires all arguments to be hashable. This works for the current call sites, but if any caller passes an unhashable value via**kwargs(e.g., a list or dict), it will raiseTypeErrorat runtime with no helpful message. Consider documenting this constraint or adding a note.Additionally,
lru_cachecaches the returned data tuples. If callers mutate the returned arrays in-place, subsequent cache hits will return the mutated data — a potential correctness hazard in benchmarks where multiple algorithms process the same dataset.
🤖 Fix all issues with AI agents
In `@python/cuml/cuml/benchmark/algorithms.py`:
- Around line 21-30: The file uses bare imports for bench_helper_funcs and
gpu_check which fail unless the benchmark dir is on sys.path; update the top of
the module to use the dual-import pattern: try a relative import (e.g., try:
from .bench_helper_funcs import _training_data_to_numpy, fit, fit_kneighbors,
fit_predict, fit_transform, predict, transform except Exception: from
bench_helper_funcs import ...), and the same for gpu_check/HAS_CUML (try: from
.gpu_check import HAS_CUML except Exception: from gpu_check import HAS_CUML) so
the module works whether imported as a package or run standalone.
In `@python/cuml/cuml/benchmark/bench_helper_funcs.py`:
- Around line 9-14: The bare imports `import datagen` and `from gpu_check import
HAS_CUML` in bench_helper_funcs.py will fail when the package is imported;
change them to the dual-import pattern used in run_benchmarks.py: attempt
relative imports first (e.g., from . import datagen and from .gpu_check import
HAS_CUML), and fall back to absolute imports in an except ImportError to support
standalone execution. Update the top-level import block that references datagen
and HAS_CUML to use this try/except pattern so the module can be imported both
as a package (cuml.benchmark.bench_helper_funcs) and run standalone.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 11-14: The top-level imports in runners.py (datagen and
HAS_CUML/is_gpu_available from gpu_check) should use the same dual-import
try/except ImportError pattern as run_benchmarks.py so the module works both
standalone and as part of the cuml.benchmark package; wrap the plain imports for
datagen and for gpu_check symbols (datagen, HAS_CUML, is_gpu_available) in
try/except blocks and on ImportError re-import them with relative package
imports (e.g., from . import datagen and from .gpu_check import HAS_CUML,
is_gpu_available) to ensure correct resolution when imported as
cuml.benchmark.runners.
In `@wiki/BENCHMARK.md`:
- Line 66: The docstring for the `--csv` flag is inaccurate: update the table
entry in BENCHMARK.md to say "Save results to a CSV file" (or similar) instead
of "Append results..." because `run_benchmarks.py` currently calls
`results_df.to_csv(args.csv)` which overwrites by default; reference `--csv`,
`run_benchmarks.py`, `results_df.to_csv`, and `args.csv` when making the change.
If you prefer append behavior instead, modify the call in `run_benchmarks.py` to
use `results_df.to_csv(args.csv, mode='a', header=not os.path.exists(args.csv))`
and import `os`, but at minimum fix the documentation to reflect current
behavior.
In `@wiki/README.md`:
- Line 9: The sentence around the inserted "[BENCHMARK.md](BENCHMARK.md)." is
broken by a stray period and becomes a fragment; edit the README sentence that
contains the benchmark reference so it becomes two grammatically complete
sentences (for example: end the first sentence after the BENCHMARK.md link, then
start a new sentence like "This allows us to provide high performance,
maintainable and overall high quality implementations, while giving as much
transparency as possible about the status of our algorithms with our users.").
Ensure the clause following the benchmark reference is capitalized and
punctuated as a full sentence.
🧹 Nitpick comments (7)
python/cuml/cuml/benchmark/cli.py (1)
19-23:run_benchmark()callssys.exit()internally for some flags, bypassing the return code.For
--print-status,--print-algorithms, and--print-datasets,run_benchmark(args)inrun_benchmarks.pycallssys.exit()directly, soreturn 0is never reached. This works but is a bit surprising for callers who expectmain()to always return an exit code (e.g., in tests). Consider havingrun_benchmarkreturn early instead, letting the caller control process exit.python/cuml/cuml/benchmark/algorithms.py (2)
319-335:trustworthiness_fn = Nonedisables UMAP accuracy in CPU-only mode.When
HAS_CUMLis False,trustworthiness_fnis set toNone(Line 335), which is then passed asaccuracy_functionfor the UMAPAlgorithmPairentries (Lines 649, 657). This means UMAP benchmarks will silently skip accuracy computation in CPU-only mode, even thoughsklearn.manifold.trustworthinessis available and could serve as a CPU fallback.Proposed fix
- trustworthiness_fn = None + trustworthiness_fn = sklearn.manifold.trustworthiness
738-744: Addstacklevel=2towarnings.warn.Per static analysis and Python best practices,
warnings.warnshould specifystacklevel=2so the warning points to the caller rather than thewarnings.warncall site itself.Proposed fix
warnings.warn( "Not all dependencies required for `cuml.dask` are installed, the " - "dask algorithms will be skipped" + "dask algorithms will be skipped", + stacklevel=2, )python/cuml/cuml/benchmark/run_benchmarks.py (1)
29-48: Dual-import pattern is the right approach; remove unusednoqadirectives.The
try/except ImportErrorpattern correctly supports both package and standalone execution. However, the# noqa: E402comments on Lines 41–44 are unnecessary (Ruff confirms E402 is not enabled), adding visual clutter.Proposed cleanup
- import algorithms # noqa: E402 - import datagen # noqa: E402 - import runners # noqa: E402 - from gpu_check import ( # noqa: E402 + import algorithms + import datagen + import runners + from gpu_check import ( HAS_CUML, get_status_string, is_gpu_available, )python/cuml/cuml/benchmark/runners.py (2)
129-139: Duplicate warnings for missing CPU implementation — consolidate.Two separate
warnings.warncalls fire for the same condition. The first (Lines 130–136) is generic and verbose; the second (Lines 137–139) is specific. Consolidate into a single, clear warning.Proposed fix
elif run_cpu and not algo_pair.has_cpu(): warnings.warn( - "run_cpu argument is set to True but no CPU " - "implementation was provided. It's possible " - "an additional library is needed but one could " - "not be found. Benchmark will be executed with " - "run_cpu=False" - ) - warnings.warn( - f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)" + f"Skipping CPU benchmark for {algo_pair.name}: " + "no CPU implementation available (missing library?)", + stacklevel=2, )
355-364: Speedup logic is duplicated betweenSpeedupComparisonRunnerandAccuracyComparisonRunner.Lines 141–150 and 355–364 contain identical speedup calculation logic. Consider extracting it into a shared method on the base class.
python/cuml/cuml/benchmark/datagen.py (1)
56-103: Inconsistent return types between_gen_data_regressionand_gen_data_blobsGPU paths.
_gen_data_regression(Line 67) wraps GPU results incudf.DataFrame/cudf.Series, while_gen_data_blobs(Line 95) returns raw cupy arrays fromcuml_datasets.make_blobs. Downstream converters handle both types, but the inconsistency could be confusing and may cause subtle issues if a converter doesn't handle cupy arrays.Consider wrapping the blobs GPU output consistently:
Proposed fix for consistency
X, y = cuml_datasets.make_blobs( n_samples=n_samples, n_features=n_features, centers=centers, random_state=random_state, dtype=dtype, ) - return X, y + return cudf.DataFrame(X), cudf.Series(y)
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/benchmark/bench_helper_funcs.py (1)
2-2:⚠️ Potential issue | 🟡 MinorUpdate SPDX copyright year to 2026.
This file now has 2026 changes, so the header year range should be updated.✏️ Suggested header update
-# SPDX-FileCopyrightText: Copyright (c) 2019-2025, NVIDIA CORPORATION. +# SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION.As per coding guidelines: Ensure copyright headers are up-to-date and in the correct format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/bench_helper_funcs.py` at line 2, Update the SPDX copyright header year range at the top of the file: replace "2019-2025" with "2019-2026" in the existing SPDX line (the first line containing "SPDX-FileCopyrightText") so the header reflects the new modification year.
♻️ Duplicate comments (2)
wiki/README.md (1)
9-9:⚠️ Potential issue | 🟡 MinorHyphenate compound adjectives in the definition-of-done sentence.
Use “high-performance” and “high-quality” when they modify “implementations.” This is a minor doc polish issue. Based on learnings: “Flag spelling errors and typos in documentation as non-critical (not CRITICAL or HIGH).”
✏️ Proposed edit
-We have criteria for defining our [definition of done](DEFINITION_OF_DONE_CRITERIA.md) to allow us to provide high performance, maintainable and overall high quality implementations, while giving as much transparency as possible about the status of our algorithms with our users. For running the benchmark suite (with or without cuML installed), see [BENCHMARK.md](BENCHMARK.md). +We have criteria for defining our [definition of done](DEFINITION_OF_DONE_CRITERIA.md) to allow us to provide high-performance, maintainable and overall high-quality implementations, while giving as much transparency as possible about the status of our algorithms with our users. For running the benchmark suite (with or without cuML installed), see [BENCHMARK.md](BENCHMARK.md).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/README.md` at line 9, The sentence starting with "We have criteria for defining our [definition of done](DEFINITION_OF_DONE_CRITERIA.md)..." needs compound adjectives hyphenated: change "high performance" to "high-performance" and "high quality" to "high-quality" where they modify "implementations" to follow style rules; update that single sentence in the README to use "high-performance, maintainable and overall high-quality implementations" (preserving links and surrounding text).python/cuml/cuml/benchmark/algorithms.py (1)
94-103:⚠️ Potential issue | 🟠 MajorFix GPU helper imports to avoid package import failures.
The barefrom bench_helper_funcs import ...will fail when this module is imported ascuml.benchmark.algorithmsbecause the benchmark directory isn’t onsys.pathin that code path. Use the same dual-import pattern or package-qualified import to avoidModuleNotFoundError.🐛 Suggested fix
- from bench_helper_funcs import ( - _build_cpu_skl_classifier, - _build_fil_classifier, - _build_fil_skl_classifier, - _build_gtil_classifier, - _build_mnmg_umap, - _build_optimized_fil_classifier, - _treelite_fil_accuracy_score, - ) + try: + from cuml.benchmark.bench_helper_funcs import ( + _build_cpu_skl_classifier, + _build_fil_classifier, + _build_fil_skl_classifier, + _build_gtil_classifier, + _build_mnmg_umap, + _build_optimized_fil_classifier, + _treelite_fil_accuracy_score, + ) + except ImportError: + from bench_helper_funcs import ( + _build_cpu_skl_classifier, + _build_fil_classifier, + _build_fil_skl_classifier, + _build_gtil_classifier, + _build_mnmg_umap, + _build_optimized_fil_classifier, + _treelite_fil_accuracy_score, + )#!/bin/bash # Verify bench_helper_funcs module location(s) to confirm import resolution. fd 'bench_helper_funcs\.py' rg -n "from bench_helper_funcs import" python/cuml/cuml/benchmark/algorithms.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/algorithms.py` around lines 94 - 103, The GPU helper import block using "from bench_helper_funcs import ..." will raise ModuleNotFoundError when this module is imported as cuml.benchmark.algorithms; update the import to a package-qualified or dual-import pattern so resolution works regardless of sys.path (e.g., try an absolute import from cuml.benchmark.bench_helper_funcs and fall back to relative/package import), ensuring the same symbols (_build_cpu_skl_classifier, _build_fil_classifier, _build_fil_skl_classifier, _build_gtil_classifier, _build_mnmg_umap, _build_optimized_fil_classifier, _treelite_fil_accuracy_score) remain imported and referenced by the same names in this module.
🧹 Nitpick comments (2)
python/cuml/cuml/benchmark/runners.py (2)
111-111: Rename unused loop variablerepto_Ruff B007: the loop control variable
repis never referenced in the body.♻️ Proposed fix
- for rep in cuml_timer.benchmark_runs(): + for _ in cuml_timer.benchmark_runs():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` at line 111, Rename the unused loop variable rep to _ in the for loop that iterates over cuml_timer.benchmark_runs() to address Ruff B007; update the loop header from "for rep in cuml_timer.benchmark_runs():" to "for _ in cuml_timer.benchmark_runs():" so the unused value is explicitly ignored while leaving the loop body and cuml_timer/benchmark_runs() usage unchanged.
119-123: Addstacklevel=2to allwarnings.warncallsAll four
warnings.warninvocations (lines 121, 141, 148, 327) are missing thestacklevelargument, so the warning will point intorunners.pyinstead of the caller's frame — making the stack location useless in logs.♻️ Proposed fixes
- warnings.warn( - f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)" - ) + warnings.warn( + f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)", + stacklevel=2, + )- warnings.warn( - "run_cpu argument is set to True but no CPU " - ... - ) - warnings.warn( - f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)" - ) + warnings.warn( + "run_cpu argument is set to True but no CPU " + ..., + stacklevel=2, + ) + warnings.warn( + f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)", + stacklevel=2, + )Apply the same
stacklevel=2fix at line 327 insideAccuracyComparisonRunner._run_one_size.Also applies to: 141-150, 327-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 119 - 123, Add stacklevel=2 to every warnings.warn call in this file so the reported stack frame points at the caller; specifically update the warnings.warn in the run_cuml branch that uses f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)" as well as the other warnings.warn sites (including the one inside AccuracyComparisonRunner._run_one_size) to call warnings.warn(<message>, stacklevel=2). Ensure each existing warnings.warn call keeps its current message and any other kwargs but adds stacklevel=2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/benchmark/algorithms.py`:
- Line 2: Update the SPDX header year range in the file to include 2026 by
modifying the copyright line at the top of
python/cuml/cuml/benchmark/algorithms.py so the year range reads 2019-2026;
locate the existing SPDX header comment and change the end year to 2026 to keep
the header current.
- Around line 760-763: The warnings.warn call emitting "Not all dependencies
required for `cuml.dask` are installed, the dask algorithms will be skipped"
should include a stacklevel so the warning points at the caller; update the
warnings.warn(...) invocation in python/cuml/cuml/benchmark/algorithms.py (the
warnings.warn call shown) to pass stacklevel=2 (e.g., warnings.warn(<message>,
stacklevel=2)) so attribution is correct for end users.
- Around line 298-355: The CPU fallback sets trustworthiness_fn to None, causing
CPU UMAP runs to miss the trustworthiness metric; change the fallback so
trustworthiness_fn points to sklearn.manifold.trustworthiness (import it if
necessary) instead of None. Update the else-branch assignment for
trustworthiness_fn (the variable named trustworthiness_fn in this file) to use
sklearn.manifold.trustworthiness so CPU and GPU UMAP entries compute the same
metric. Ensure you add or verify the import for sklearn.manifold.trustworthiness
at the top of the module if it isn’t already imported.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 140-150: Remove the redundant first warnings.warn in the elif
branch that checks "run_cpu and not algo_pair.has_cpu()": keep only the concise
second warning that uses f"Skipping CPU benchmark for {algo_pair.name} (missing
CPU library)". Ensure you do not change the value of run_cpu (do not reassign
it) and update only the code around the condition that calls algo_pair.has_cpu()
so a single, accurate warning is emitted.
- Around line 106-123: The cuML/CPU benchmark paths silently skip when a run is
requested but the corresponding implementation is missing; update both
SpeedupComparisonRunner._run_one_size and AccuracyComparisonRunner._run_one_size
to mirror the existing warning pattern: after the existing "if run_cuml and
is_gpu_available() and algo_pair.has_cuml()" block add an "elif run_cuml and
is_gpu_available() and not algo_pair.has_cuml()" that issues a
warnings.warn(f"Skipping cuML benchmark for {algo_pair.name} (no cuML
implementation)") when verbose is true, and likewise after the "if run_cpu and
algo_pair.has_cpu()" block add an "elif run_cpu and not algo_pair.has_cpu()"
that issues a warnings.warn(f"Skipping CPU benchmark for {algo_pair.name} (no
CPU implementation)") when verbose is true; reference the methods
algo_pair.has_cuml(), algo_pair.has_cpu(), run_cuml/run_cpu and the classes
SpeedupComparisonRunner._run_one_size and AccuracyComparisonRunner._run_one_size
to locate where to insert these branches.
---
Outside diff comments:
In `@python/cuml/cuml/benchmark/bench_helper_funcs.py`:
- Line 2: Update the SPDX copyright header year range at the top of the file:
replace "2019-2025" with "2019-2026" in the existing SPDX line (the first line
containing "SPDX-FileCopyrightText") so the header reflects the new modification
year.
---
Duplicate comments:
In `@python/cuml/cuml/benchmark/algorithms.py`:
- Around line 94-103: The GPU helper import block using "from bench_helper_funcs
import ..." will raise ModuleNotFoundError when this module is imported as
cuml.benchmark.algorithms; update the import to a package-qualified or
dual-import pattern so resolution works regardless of sys.path (e.g., try an
absolute import from cuml.benchmark.bench_helper_funcs and fall back to
relative/package import), ensuring the same symbols (_build_cpu_skl_classifier,
_build_fil_classifier, _build_fil_skl_classifier, _build_gtil_classifier,
_build_mnmg_umap, _build_optimized_fil_classifier, _treelite_fil_accuracy_score)
remain imported and referenced by the same names in this module.
In `@wiki/README.md`:
- Line 9: The sentence starting with "We have criteria for defining our
[definition of done](DEFINITION_OF_DONE_CRITERIA.md)..." needs compound
adjectives hyphenated: change "high performance" to "high-performance" and "high
quality" to "high-quality" where they modify "implementations" to follow style
rules; update that single sentence in the README to use "high-performance,
maintainable and overall high-quality implementations" (preserving links and
surrounding text).
---
Nitpick comments:
In `@python/cuml/cuml/benchmark/runners.py`:
- Line 111: Rename the unused loop variable rep to _ in the for loop that
iterates over cuml_timer.benchmark_runs() to address Ruff B007; update the loop
header from "for rep in cuml_timer.benchmark_runs():" to "for _ in
cuml_timer.benchmark_runs():" so the unused value is explicitly ignored while
leaving the loop body and cuml_timer/benchmark_runs() usage unchanged.
- Around line 119-123: Add stacklevel=2 to every warnings.warn call in this file
so the reported stack frame points at the caller; specifically update the
warnings.warn in the run_cuml branch that uses f"Skipping cuML benchmark for
{algo_pair.name} (GPU not available)" as well as the other warnings.warn sites
(including the one inside AccuracyComparisonRunner._run_one_size) to call
warnings.warn(<message>, stacklevel=2). Ensure each existing warnings.warn call
keeps its current message and any other kwargs but adds stacklevel=2.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
python/cuml/cuml/benchmark/algorithms.pypython/cuml/cuml/benchmark/bench_helper_funcs.pypython/cuml/cuml/benchmark/runners.pywiki/BENCHMARK.mdwiki/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- wiki/BENCHMARK.md
gforsyth
approved these changes
Feb 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/benchmark/run_benchmarks.py (1)
317-405:⚠️ Potential issue | 🟠 MajorHandle the “no benchmarks enabled” case when GPU is unavailable and
--skip-cpuis set.At Line 317,
run_gpubecomesFalsewhen cuML/GPU is unavailable. If the user passes--skip-cpu, Line 403 also disables CPU execution, sorunners.run_variations(...)is called with both modes off.Proposed fix
- # Determine whether to run GPU benchmarks - run_gpu = is_gpu_available() and not args.skip_gpu + # Determine whether to run GPU and CPU benchmarks + gpu_available = is_gpu_available() + run_gpu = gpu_available and not args.skip_gpu + run_cpu = not args.skip_cpu + + if not run_gpu and not run_cpu: + raise ValueError( + "GPU-only mode requested (--skip-cpu), but cuML/GPU mode is unavailable." + ) @@ - run_cpu=(not args.skip_cpu), + run_cpu=run_cpu, run_cuml=run_gpu,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/run_benchmarks.py` around lines 317 - 405, The code can call runners.run_variations with both GPU and CPU disabled when run_gpu is False and args.skip_cpu is True; add a guard before the runners.run_variations(...) call that checks if not run_gpu and args.skip_cpu and raise a clear ValueError (or argparse-style error) explaining no benchmarks are enabled and instructing the user to enable CPU or GPU; reference run_gpu, args.skip_cpu and the runners.run_variations(...) call so you place the check immediately prior to that invocation.
♻️ Duplicate comments (3)
python/cuml/cuml/benchmark/runners.py (2)
140-150:⚠️ Potential issue | 🟡 MinorDrop the first CPU warning; it is inaccurate and redundant.
The first warning states
run_cpu=Falsebehavior, butrun_cpuis never changed. The second warning already communicates the real behavior.🔧 Proposed fix
elif run_cpu and not algo_pair.has_cpu(): - warnings.warn( - "run_cpu argument is set to True but no CPU " - "implementation was provided. It's possible " - "an additional library is needed but one could " - "not be found. Benchmark will be executed with " - "run_cpu=False" - ) warnings.warn( - f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)" + f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)", + stacklevel=2, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 140 - 150, Remove the redundant/inaccurate first warnings.warn block in the branch guarded by "elif run_cpu and not algo_pair.has_cpu()": delete the multi-line warning that claims "run_cpu argument is set to True but no CPU implementation..." so only the second warning (warnings.warn(f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)")) remains; locate this logic around the use of run_cpu and algo_pair.has_cpu() in runners.py and keep the f-string warning which accurately describes the skipped CPU benchmark.
106-123:⚠️ Potential issue | 🟡 MinorRequested benchmark paths can still be silently skipped in both runners.
There are still missing
elifbranches for “requested but no implementation available” cases, so runs can be silently omitted.🔧 Proposed fix
elif run_cuml and not is_gpu_available(): if verbose: warnings.warn( - f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)" + f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)", + stacklevel=2, ) + elif run_cuml and is_gpu_available() and not algo_pair.has_cuml(): + if verbose: + warnings.warn( + f"Skipping cuML benchmark for {algo_pair.name} (no cuML implementation)", + stacklevel=2, + ) @@ elif run_cuml and not is_gpu_available(): if verbose: warnings.warn( - f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)" + f"Skipping cuML benchmark for {algo_pair.name} (GPU not available)", + stacklevel=2, ) + elif run_cuml and is_gpu_available() and not algo_pair.has_cuml(): + if verbose: + warnings.warn( + f"Skipping cuML benchmark for {algo_pair.name} (no cuML implementation)", + stacklevel=2, + ) @@ if run_cpu and algo_pair.has_cpu(): ... + elif run_cpu and not algo_pair.has_cpu(): + if verbose: + warnings.warn( + f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)", + stacklevel=2, + )Also applies to: 291-330, 332-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 106 - 123, The branch logic in the benchmark runner is missing explicit handling for cases where the user requested a backend (e.g., run_cuml) but that backend has no implementation for the current algo_pair, causing silent skips; update the control flow around the cuml block (using run_cuml, is_gpu_available(), and algo_pair.has_cuml()) to add an additional elif that triggers when run_cuml is true, GPU is available, but algo_pair.has_cuml() is false—emit a warning (or raise) that the requested cuML implementation is not available for algo_pair.name; apply the same pattern for other backend blocks (e.g., scikit/sklearn: run_sklearn and algo_pair.has_sklearn()/algo_pair.run_sklearn) in the other affected regions so requested-but-unimplemented paths are not silently omitted.python/cuml/cuml/benchmark/algorithms.py (1)
352-355:⚠️ Potential issue | 🟠 MajorCPU UMAP currently loses its accuracy metric because
trustworthiness_fnisNone.This makes CPU-only UMAP runs produce no trustworthiness metric even though a CPU implementation exists.
🔧 Proposed fix
- trustworthiness_fn = None + trustworthiness_fn = sklearn.manifold.trustworthiness#!/bin/bash # Verify trustworthiness fallback and UMAP metric wiring rg -n "trustworthiness_fn\s*=" python/cuml/cuml/benchmark/algorithms.py -C2 rg -n "accuracy_function=trustworthiness_fn" python/cuml/cuml/benchmark/algorithms.py -C2Also applies to: 668-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/algorithms.py` around lines 352 - 355, Replace the None placeholder so UMAP CPU runs produce a trustworthiness metric: import and assign the sklearn manifold trustworthiness function to trustworthiness_fn (e.g., use sklearn.manifold.trustworthiness or from sklearn.manifold import trustworthiness) instead of None where accuracy_fn and r2_fn are set; ensure the code paths that later reference trustworthiness_fn (e.g., any usage like accuracy_function=trustworthiness_fn) will pick up this function so CPU-only UMAP reports trustworthiness.
🧹 Nitpick comments (1)
python/cuml/cuml/benchmark/run_benchmarks.py (1)
313-333: Prefer returning fromrun_benchmark()instead of callingsys.exit()inside it.
run_benchmark()is now a reusable function; usingsys.exit()in it makes embedding/tests harder than necessary.Proposed refactor
if args.print_status: - sys.exit() + return @@ if args.print_algorithms: for algo in algorithms.all_algorithms(): print(algo.name) - sys.exit() + return @@ if args.print_datasets: for dataset in datagen.all_datasets().keys(): print(dataset) - sys.exit() + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/run_benchmarks.py` around lines 313 - 333, The function currently calls sys.exit() in several early-exit branches (the args.print_status block, the args.print_algorithms loop, and the args.print_datasets loop) which makes run_benchmark() non-reusable; replace these sys.exit() calls with simple returns (e.g., return None or an explicit status) so the function exits gracefully without terminating the process. Specifically, update the blocks that check args.print_status, args.print_algorithms (the loop over algorithms.all_algorithms() / print(algo.name)), and args.print_datasets (the loop over datagen.all_datasets()) to return after printing, and ensure any callers of run_benchmark() handle the returned value instead of relying on process termination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/benchmark/algorithms.py`:
- Around line 95-103: The bare relative import "from bench_helper_funcs import
..." inside the HAS_CUML branch should use the same dual-import pattern used
earlier: attempt an absolute/package import of the functions from
bench_helper_funcs and fall back to importing from the local module path if that
fails; update the import for _build_cpu_skl_classifier, _build_fil_classifier,
_build_fil_skl_classifier, _build_gtil_classifier, _build_mnmg_umap,
_build_optimized_fil_classifier, and _treelite_fil_accuracy_score to follow that
try/except import pattern and guard it under the existing HAS_CUML check so
package mode and standalone mode both succeed.
In `@python/cuml/cuml/benchmark/datagen.py`:
- Around line 141-149: The current calculation of n_informative and n_redundant
can produce invalid values for small n_features; update the logic before calling
sklearn_make_classification so that n_informative is clamped to at least 1 and
at most n_features (e.g., n_informative = max(1, min(n_features, n_features // 2
or 1))) and compute n_redundant as a non-negative value that does not push the
sum over n_features (e.g., n_redundant = max(0, min(2, n_features -
n_informative)) and if n_informative + n_redundant > n_features set n_redundant
= n_features - n_informative); apply these corrected variables where
sklearn_make_classification is invoked to ensure valid parameter combinations.
- Line 34: datagen.py currently does a bare import of HAS_CUML and
is_gpu_available which breaks package vs standalone imports; update the import
to the dual-import pattern used elsewhere by trying a relative import first
(e.g., try: from .gpu_check import HAS_CUML, is_gpu_available) and falling back
to the top-level import in an except ImportError block (except ImportError: from
gpu_check import HAS_CUML, is_gpu_available) so functions or classes in
datagen.py can be imported in both contexts.
In `@python/cuml/cuml/benchmark/gpu_check.py`:
- Around line 13-35: The current _check_cuml/HAS_CUML only tests cuml
importability, not whether a usable GPU runtime exists; update _check_cuml to
(1) import cuml, then (2) verify a CUDA device is available by trying to import
a GPU runtime (e.g., cupy and checking cupy.cuda.runtime.getDeviceCount() > 0)
and/or falling back to numba.cuda.is_available(), returning True only if both
cuml imports and a device check succeed; keep HAS_CUML assigned from _check_cuml
and leave is_gpu_available() returning HAS_CUML.
In `@python/cuml/cuml/benchmark/run_benchmark.py`:
- Around line 23-25: The module currently calls sys.exit(main()) at import time
which terminates the process; wrap the call in a module-level guard so it only
runs when executed as a script (use the standard if __name__ == "__main__":
followed by calling main() and exiting with its return via sys.exit). Locate the
unconditional sys.exit(main()) call (referencing main from cli) and move it
inside that guard so importing this module no longer triggers process
termination.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 442-447: When GPU is unavailable, the current code unconditionally
sets run_cuml = False which can leave nothing enabled if run_cpu is already
False; modify the logic around is_gpu_available() to first check run_cpu (and
any other runner flags) and if GPU is unavailable AND run_cpu is False raise an
explicit error/exit (e.g., RuntimeError with a clear message) instead of
silently disabling run_cuml; otherwise only set run_cuml = False when GPU is
unavailable and at least one CPU runner is enabled. Reference
is_gpu_available(), run_cuml and run_cpu to locate and update the conditional.
---
Outside diff comments:
In `@python/cuml/cuml/benchmark/run_benchmarks.py`:
- Around line 317-405: The code can call runners.run_variations with both GPU
and CPU disabled when run_gpu is False and args.skip_cpu is True; add a guard
before the runners.run_variations(...) call that checks if not run_gpu and
args.skip_cpu and raise a clear ValueError (or argparse-style error) explaining
no benchmarks are enabled and instructing the user to enable CPU or GPU;
reference run_gpu, args.skip_cpu and the runners.run_variations(...) call so you
place the check immediately prior to that invocation.
---
Duplicate comments:
In `@python/cuml/cuml/benchmark/algorithms.py`:
- Around line 352-355: Replace the None placeholder so UMAP CPU runs produce a
trustworthiness metric: import and assign the sklearn manifold trustworthiness
function to trustworthiness_fn (e.g., use sklearn.manifold.trustworthiness or
from sklearn.manifold import trustworthiness) instead of None where accuracy_fn
and r2_fn are set; ensure the code paths that later reference trustworthiness_fn
(e.g., any usage like accuracy_function=trustworthiness_fn) will pick up this
function so CPU-only UMAP reports trustworthiness.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 140-150: Remove the redundant/inaccurate first warnings.warn block
in the branch guarded by "elif run_cpu and not algo_pair.has_cpu()": delete the
multi-line warning that claims "run_cpu argument is set to True but no CPU
implementation..." so only the second warning (warnings.warn(f"Skipping CPU
benchmark for {algo_pair.name} (missing CPU library)")) remains; locate this
logic around the use of run_cpu and algo_pair.has_cpu() in runners.py and keep
the f-string warning which accurately describes the skipped CPU benchmark.
- Around line 106-123: The branch logic in the benchmark runner is missing
explicit handling for cases where the user requested a backend (e.g., run_cuml)
but that backend has no implementation for the current algo_pair, causing silent
skips; update the control flow around the cuml block (using run_cuml,
is_gpu_available(), and algo_pair.has_cuml()) to add an additional elif that
triggers when run_cuml is true, GPU is available, but algo_pair.has_cuml() is
false—emit a warning (or raise) that the requested cuML implementation is not
available for algo_pair.name; apply the same pattern for other backend blocks
(e.g., scikit/sklearn: run_sklearn and
algo_pair.has_sklearn()/algo_pair.run_sklearn) in the other affected regions so
requested-but-unimplemented paths are not silently omitted.
---
Nitpick comments:
In `@python/cuml/cuml/benchmark/run_benchmarks.py`:
- Around line 313-333: The function currently calls sys.exit() in several
early-exit branches (the args.print_status block, the args.print_algorithms
loop, and the args.print_datasets loop) which makes run_benchmark()
non-reusable; replace these sys.exit() calls with simple returns (e.g., return
None or an explicit status) so the function exits gracefully without terminating
the process. Specifically, update the blocks that check args.print_status,
args.print_algorithms (the loop over algorithms.all_algorithms() /
print(algo.name)), and args.print_datasets (the loop over
datagen.all_datasets()) to return after printing, and ensure any callers of
run_benchmark() handle the returned value instead of relying on process
termination.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pyproject.tomlpython/cuml/cuml/benchmark/__init__.pypython/cuml/cuml/benchmark/__main__.pypython/cuml/cuml/benchmark/algorithms.pypython/cuml/cuml/benchmark/bench_helper_funcs.pypython/cuml/cuml/benchmark/cli.pypython/cuml/cuml/benchmark/core.pypython/cuml/cuml/benchmark/datagen.pypython/cuml/cuml/benchmark/gpu_check.pypython/cuml/cuml/benchmark/run_benchmark.pypython/cuml/cuml/benchmark/run_benchmarks.pypython/cuml/cuml/benchmark/runners.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/benchmark/main.py
- python/cuml/cuml/benchmark/init.py
csadorf
added
improvement
csadorf
requested changes
Mar 2, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
python/cuml/cuml/benchmark/runners.py (1)
440-445:⚠️ Potential issue | 🟠 MajorFail fast when GPU fallback leaves no enabled benchmark path.
At Line 444,
run_cumlis forced toFalsewhen GPU is unavailable. Ifrun_cpuis alreadyFalse, the run becomes a silent no-op.🔧 Proposed fix
gpu_available = is_gpu_available() if not gpu_available: print("Note: Running in CPU-only mode (GPU not available)") run_cuml = False + if not run_cpu and not run_cuml: + raise ValueError( + "No benchmarks to run: GPU unavailable and CPU benchmarking disabled." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 440 - 445, The code currently forces run_cuml = False when is_gpu_available() is False which can leave both run_cuml and run_cpu disabled and produce a silent no-op; after the GPU check (where you set run_cuml = False) add a guard that checks the final flags (run_cuml and run_cpu) and fail fast (raise a RuntimeError or exit with a clear message) if both are False so the runner reports no enabled benchmark path instead of doing nothing.python/cuml/cuml/benchmark/run_benchmarks.py (1)
318-323:⚠️ Potential issue | 🟠 MajorGate GPU benchmark mode on
HAS_CUMLas well as GPU visibility.At Line 319,
run_gpucan becomeTrueeven when cuML is not installed, which conflicts with the reported status and can route into misleading “cuml” runs.🔧 Proposed fix
- run_gpu = is_gpu_available() and not args.skip_gpu + run_gpu = HAS_CUML and is_gpu_available() and not args.skip_gpu + + if args.skip_cpu and not run_gpu: + raise ValueError( + "GPU-only mode requested, but cuML/GPU runtime is unavailable." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/run_benchmarks.py` around lines 318 - 323, The GPU-run gating currently uses run_gpu = is_gpu_available() and not args.skip_gpu which can be True when cuML isn't installed; change the condition to also require HAS_CUML (e.g., run_gpu = is_gpu_available() and HAS_CUML and not args.skip_gpu) and ensure the subsequent setup_rmm_allocator(args.rmm_allocator) call is only executed when run_gpu is True so GPU benchmarks and allocator setup never proceed if HAS_CUML is False.python/cuml/cuml/benchmark/datagen.py (1)
149-157:⚠️ Potential issue | 🟠 MajorClamp classification feature parameters for low-dimensional inputs.
Lines 149-150 can yield invalid combinations (e.g.,
n_features=1⇒n_redundant < 0), which can break data generation.🔧 Proposed fix
- n_informative = max(2, n_features // 2) - n_redundant = min(2, n_features - n_informative) + n_informative = min(max(1, n_features // 2 or 1), n_features) + n_redundant = max(0, min(2, n_features - n_informative))#!/bin/bash # Verify current formulas produce invalid values for small n_features. python - <<'PY' for n_features in [1, 2, 3]: n_informative = max(2, n_features // 2) n_redundant = min(2, n_features - n_informative) print( f"n_features={n_features}: " f"n_informative={n_informative}, n_redundant={n_redundant}, " f"sum={n_informative + n_redundant}" ) PYAs per coding guidelines: "Missing input dimension checks (n_samples, n_features) and not handling edge cases (empty datasets, single sample) must be addressed in Python estimators."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/datagen.py` around lines 149 - 157, The n_informative/n_redundant calculation can produce negative or invalid values for small n_features; before calling sklearn_make_classification, clamp n_informative to be at least 1 and at most n_features (e.g., n_informative = min(n_features, max(1, n_features // 2, 2))) and compute n_redundant as a non-negative value no greater than remaining features (e.g., n_redundant = max(0, min(2, n_features - n_informative))), ensuring n_informative + n_redundant <= n_features so the sklearn_make_classification invocation is always valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/benchmark/gpu_check.py`:
- Around line 13-20: The helper _check_cuml currently only catches ImportError
for the import cuml statement; change the exception handling in _check_cuml to
catch any Exception (e.g., except Exception as e) so import-time errors that are
not ImportError are handled and the function returns False; optionally emit a
warning or logging message with the exception details before returning False to
aid debugging while keeping behavior the same for SystemExit/KeyboardInterrupt
(do not catch BaseException).
---
Duplicate comments:
In `@python/cuml/cuml/benchmark/datagen.py`:
- Around line 149-157: The n_informative/n_redundant calculation can produce
negative or invalid values for small n_features; before calling
sklearn_make_classification, clamp n_informative to be at least 1 and at most
n_features (e.g., n_informative = min(n_features, max(1, n_features // 2, 2)))
and compute n_redundant as a non-negative value no greater than remaining
features (e.g., n_redundant = max(0, min(2, n_features - n_informative))),
ensuring n_informative + n_redundant <= n_features so the
sklearn_make_classification invocation is always valid.
In `@python/cuml/cuml/benchmark/run_benchmarks.py`:
- Around line 318-323: The GPU-run gating currently uses run_gpu =
is_gpu_available() and not args.skip_gpu which can be True when cuML isn't
installed; change the condition to also require HAS_CUML (e.g., run_gpu =
is_gpu_available() and HAS_CUML and not args.skip_gpu) and ensure the subsequent
setup_rmm_allocator(args.rmm_allocator) call is only executed when run_gpu is
True so GPU benchmarks and allocator setup never proceed if HAS_CUML is False.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 440-445: The code currently forces run_cuml = False when
is_gpu_available() is False which can leave both run_cuml and run_cpu disabled
and produce a silent no-op; after the GPU check (where you set run_cuml = False)
add a guard that checks the final flags (run_cuml and run_cpu) and fail fast
(raise a RuntimeError or exit with a clear message) if both are False so the
runner reports no enabled benchmark path instead of doing nothing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
python/cuml/cuml/benchmark/__main__.pypython/cuml/cuml/benchmark/algorithms.pypython/cuml/cuml/benchmark/bench_helper_funcs.pypython/cuml/cuml/benchmark/datagen.pypython/cuml/cuml/benchmark/gpu_check.pypython/cuml/cuml/benchmark/run_benchmarks.pypython/cuml/cuml/benchmark/runners.pywiki/BENCHMARK.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/benchmark/main.py
csadorf
force-pushed
the
enable-nocuml-bench
branch
from
5ca211d to
91aa6e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/benchmark/run_benchmarks.py (1)
236-257:⚠️ Potential issue | 🟠 MajorGate
run_cumlon cuML availability, not GPU visibility alone.
run_gpucurrently depends only on GPU availability. On hosts with CUDA/CuPy but no cuML installation, this still enables the cuML execution path.Suggested fix
- run_gpu = is_gpu_available() and not args.skip_gpu + run_gpu = is_cuml_available() and is_gpu_available() and not args.skip_gpu + if not is_cuml_available() and not args.skip_gpu: + print("Note: cuML not installed; running in CPU-only mode")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/run_benchmarks.py` around lines 236 - 257, The code currently sets run_gpu = is_gpu_available() and not args.skip_gpu and then passes run_cuml=run_gpu to runners.run_variations, which enables cuML runs on GPU visibility alone; change this to explicitly detect cuML availability (e.g., try importing cuml or call a helper like is_cuml_available()) and set run_cuml = run_gpu and cuml_available. Ensure setup_rmm_allocator(args.rmm_allocator) remains conditioned on run_gpu, replace the run_variations argument to use the new run_cuml variable (run_cuml=run_cuml), and update any validation (_validate_args) or upstream logic that assumes run_cuml was tied to run_gpu.
♻️ Duplicate comments (3)
python/cuml/cuml/benchmark/runners.py (2)
138-148:⚠️ Potential issue | 🟡 MinorEmit a single CPU-skip warning in this branch.
This branch currently emits two warnings for one condition, and the first message is misleading because
run_cpuis not actually reassigned.Suggested fix
elif run_cpu and not algo_pair.has_cpu(): - warnings.warn( - "run_cpu argument is set to True but no CPU " - "implementation was provided. It's possible " - "an additional library is needed but one could " - "not be found. Benchmark will be executed with " - "run_cpu=False" - ) warnings.warn( - f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)" + f"Skipping CPU benchmark for {algo_pair.name} (missing CPU library)", + stacklevel=2, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 138 - 148, The branch that checks "elif run_cpu and not algo_pair.has_cpu()" currently emits two warnings and uses a misleading message claiming "run_cpu=False"; update this to emit a single clear warning stating the CPU benchmark will be skipped for the specific algorithm due to a missing CPU implementation or library. Locate the branch tied to algo_pair.has_cpu(), remove the duplicate warnings, and replace them with one warnings.warn that references algo_pair.name and explains the skip (e.g., "Skipping CPU benchmark for {algo_pair.name}: missing CPU implementation/library").
442-447:⚠️ Potential issue | 🟠 MajorFail fast when GPU fallback leaves no benchmark path enabled.
If GPU is unavailable and
run_cpuis alreadyFalse, the function silently proceeds with nothing to run.Suggested fix
gpu_available = is_gpu_available() if not gpu_available: print("Note: Running in CPU-only mode (GPU not available)") run_cuml = False + if not run_cpu and not run_cuml: + raise ValueError( + "No benchmarks to run: GPU is unavailable and CPU benchmarking is disabled." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/runners.py` around lines 442 - 447, When GPU is unavailable the current block sets run_cuml = False but doesn’t guard against the case where run_cpu is also False; add a fail-fast check right after the is_gpu_available() block (near the call to is_gpu_available() and where run_cuml is set) that detects if not gpu_available and not run_cpu and then raise a clear RuntimeError (or exit) indicating "GPU unavailable and CPU benchmarks disabled — nothing to run" so the function fails fast instead of proceeding silently.python/cuml/cuml/benchmark/datagen.py (1)
149-156:⚠️ Potential issue | 🟠 MajorClamp classification defaults for low feature counts.
For small
n_features, current math can produce invalid combinations (e.g., negativen_redundant), causing runtime failures in classification data generation.Suggested fix
- n_informative = max(2, n_features // 2) - n_redundant = min(2, n_features - n_informative) + n_informative = min(max(1, n_features // 2 or 1), n_features) + n_redundant = max(0, min(2, n_features - n_informative))As per coding guidelines:
Missing input dimension checks (n_samples, n_features) and not handling edge cases (empty datasets, single sample) must be addressed in Python estimators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/benchmark/datagen.py` around lines 149 - 156, Validate input dimensions before calling sklearn_make_classification: ensure n_samples >= 1 and n_features >= 1 (raise ValueError with a clear message if not), and guard against edge cases like single-sample or empty datasets; then compute defaults for n_informative and n_redundant safely by clamping them to valid ranges (e.g., set n_informative = max(1, min(n_features, n_features // 2)) and n_redundant = max(0, min(2, n_features - n_informative))) so n_redundant is never negative and n_informative never exceeds n_features before passing them into sklearn_make_classification in datagen.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/benchmark/cli.py`:
- Around line 53-57: The CLI help string for the argparse option defined with
"--max-rows" (see the parser.add_argument call that sets type=int and
default=100000) wrongly references "max_row"; update the help text to match the
flag name (e.g., "Evaluate at most max-rows samples" or more naturally "Evaluate
at most --max-rows samples" / "Maximum number of rows to evaluate") so the help
output and flag are consistent and spell-checked.
In `@python/cuml/cuml/benchmark/gpu_check.py`:
- Around line 42-67: get_available_input_types and get_status_string currently
use different checks (is_gpu_available vs is_cuml_available) causing
inconsistent outputs; unify them by checking both capabilities: update
get_available_input_types() to include GPU-backed types only when
is_gpu_available() is True and is_cuml_available() is True (or when you prefer,
check GPU visibility separately and document), and update get_status_string() to
return one of three clear states using both helpers—e.g., "GPU mode (cuML
available)" when both is_gpu_available() and is_cuml_available() are True, "GPU
visible (cuML not installed)" when is_gpu_available() is True but
is_cuml_available() is False, and "CPU-only mode (cuML not installed)"
otherwise; modify the functions get_available_input_types and get_status_string
accordingly so both use the same combined capability logic.
---
Outside diff comments:
In `@python/cuml/cuml/benchmark/run_benchmarks.py`:
- Around line 236-257: The code currently sets run_gpu = is_gpu_available() and
not args.skip_gpu and then passes run_cuml=run_gpu to runners.run_variations,
which enables cuML runs on GPU visibility alone; change this to explicitly
detect cuML availability (e.g., try importing cuml or call a helper like
is_cuml_available()) and set run_cuml = run_gpu and cuml_available. Ensure
setup_rmm_allocator(args.rmm_allocator) remains conditioned on run_gpu, replace
the run_variations argument to use the new run_cuml variable
(run_cuml=run_cuml), and update any validation (_validate_args) or upstream
logic that assumes run_cuml was tied to run_gpu.
---
Duplicate comments:
In `@python/cuml/cuml/benchmark/datagen.py`:
- Around line 149-156: Validate input dimensions before calling
sklearn_make_classification: ensure n_samples >= 1 and n_features >= 1 (raise
ValueError with a clear message if not), and guard against edge cases like
single-sample or empty datasets; then compute defaults for n_informative and
n_redundant safely by clamping them to valid ranges (e.g., set n_informative =
max(1, min(n_features, n_features // 2)) and n_redundant = max(0, min(2,
n_features - n_informative))) so n_redundant is never negative and n_informative
never exceeds n_features before passing them into sklearn_make_classification in
datagen.py.
In `@python/cuml/cuml/benchmark/runners.py`:
- Around line 138-148: The branch that checks "elif run_cpu and not
algo_pair.has_cpu()" currently emits two warnings and uses a misleading message
claiming "run_cpu=False"; update this to emit a single clear warning stating the
CPU benchmark will be skipped for the specific algorithm due to a missing CPU
implementation or library. Locate the branch tied to algo_pair.has_cpu(), remove
the duplicate warnings, and replace them with one warnings.warn that references
algo_pair.name and explains the skip (e.g., "Skipping CPU benchmark for
{algo_pair.name}: missing CPU implementation/library").
- Around line 442-447: When GPU is unavailable the current block sets run_cuml =
False but doesn’t guard against the case where run_cpu is also False; add a
fail-fast check right after the is_gpu_available() block (near the call to
is_gpu_available() and where run_cuml is set) that detects if not gpu_available
and not run_cpu and then raise a clear RuntimeError (or exit) indicating "GPU
unavailable and CPU benchmarks disabled — nothing to run" so the function fails
fast instead of proceeding silently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
python/cuml/cuml/benchmark/__init__.pypython/cuml/cuml/benchmark/__main__.pypython/cuml/cuml/benchmark/algorithms.pypython/cuml/cuml/benchmark/bench_helper_funcs.pypython/cuml/cuml/benchmark/cli.pypython/cuml/cuml/benchmark/datagen.pypython/cuml/cuml/benchmark/gpu_check.pypython/cuml/cuml/benchmark/run_benchmarks.pypython/cuml/cuml/benchmark/runners.pywiki/BENCHMARK.mdwiki/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- wiki/BENCHMARK.md
- wiki/README.md
csadorf
approved these changes
Mar 3, 2026
Contributor
|
/merge |
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
5 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.
No description provided.