CI Fix bertopic testing - #8298
Conversation
This resolves the problem of torch pulling in a CUDA 13 stack which results in some packages using CUDA 12 and some 13.
|
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:
📝 WalkthroughWalkthroughThe PR adds BERTopic wheel integration dependency wiring, a new workflow job, and updates the integration script to generate requirements, install them with cuML wheels, and run the smoke test with explicit cuML-backed models. ChangesBERTopic wheel integration test changes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/test_wheel_integrations.sh`:
- Around line 48-49: The BERTopic smoke test currently only checks proxy
wrapping, which does not verify GPU execution. Update the invocation in the
test_wheel_integrations.sh BERTopic block to run with --verbose, then assert the
output includes the execution signal used by the in-repo health checks (ran on
GPU) and does not show any CPU fallback text. Use the existing
cuml.accel/health-check contract as the reference so the test validates actual
GPU use rather than just is_proxy behavior.
- Around line 48-49: Add a runner-side GPU availability preflight before the
BERTopic smoke test starts, so the CI job fails fast with a clear message if no
GPU is visible. Place the check in the same shell flow in
ci/test_wheel_integrations.sh just before the rapids-logger and timeout python
-m cuml.accel invocation, and make sure the failure path prints a meaningful
setup error rather than relying on BERTopic/cuml to fail later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3e5a1648-8804-43fe-b7e0-8e37121ff970
📒 Files selected for processing (1)
ci/test_wheel_integrations.sh
betatim
added
improvement
Cute use of our profiler instead of grepping the output.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/test_wheel_integrations.sh (1)
30-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winForce-reinstall CPU-only PyTorch here.
rapids-pip-retry install --index-url ... torchcan leave an already-satisfiedtorchin place, so a reused CI image may still keep a CUDA-enabled build and defeat the mixed-stack guard. Use--force-reinstall(or uninstall first) and asserttorch.version.cuda is Noneafter install.Suggested change
rapids-logger "Installing CPU-only PyTorch" -rapids-pip-retry install --index-url https://download.pytorch.org/whl/cpu torch +rapids-pip-retry install --force-reinstall --index-url https://download.pytorch.org/whl/cpu torch +python - <<'PY' +import torch +assert torch.version.cuda is None, f"Expected CPU-only torch, got CUDA-enabled torch ({torch.version.cuda})" +PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/test_wheel_integrations.sh` around lines 30 - 33, The CPU-only PyTorch install step in the test_wheel_integrations flow can leave an existing CUDA-enabled torch untouched on reused CI images. Update the installation step around rapids-pip-retry install for torch to force a reinstall (or uninstall torch first), and add a post-install check that verifies torch.version.cuda is None so the mixed-stack guard is actually enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ci/test_wheel_integrations.sh`:
- Around line 30-33: The CPU-only PyTorch install step in the
test_wheel_integrations flow can leave an existing CUDA-enabled torch untouched
on reused CI images. Update the installation step around rapids-pip-retry
install for torch to force a reinstall (or uninstall torch first), and add a
post-install check that verifies torch.version.cuda is None so the mixed-stack
guard is actually enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9eeaeea5-12c9-4ac6-8fd9-d0c47d9c8970
📒 Files selected for processing (1)
ci/test_wheel_integrations.sh
|
I'm going to tag in @jameslamb on this because he has spent way too long figuring out every permutation of pytorch wheel interactions and I don't want to undo his good work. |
| # Step 2: Install CPU-only PyTorch first so BERTopic's transitive torch | ||
| # dependency does not pull a CUDA 13 stack on top of cuML's | ||
| rapids-logger "Installing CPU-only PyTorch" | ||
| rapids-pip-retry install --index-url https://download.pytorch.org/whl/cpu torch | ||
|
|
||
| # Step 3: Install BERTopic (reuses the already-installed CPU torch) | ||
| rapids-logger "Installing BERTopic" | ||
| rapids-pip-retry install --prefer-binary bertopic |
There was a problem hiding this comment.
There's a more reliable way to do this AND to use a CUDA build of torch.
I recommend you follow how cugraph-gnn testing works, where we download (NOT install) a torch CUDA wheel:
Then pass it as a requirement alongside the other things you're installing:
which would end with something roughly like this:
rapids-pip-retry install \
--prefer-binary \
"${LIBCUML_WHEELHOUSE}"/libcuml*.whl \
"${CUML_WHEELHOUSE}"/cuml*.whl \
/tmp/wherever-you-downloaded-torch/torch_*.whl \
bertopicOr if you really must still have separate install commands (if #7374 still applies here in 26.08), then the same thing but split like this:
rapids-pip-retry install \
--no-deps \
/tmp/wherever-you-downloaded-torch/torch_*.whl
rapids-pip-retry-install \
--prefer-binary \
"${LIBCUML_WHEELHOUSE}"/libcuml*.whl \
"${CUML_WHEELHOUSE}"/cuml*.whl \
bertopicIn general fewer separate pip install calls = reduced risk of a broken environment. And the more reliable way to force a particular file into an environment is to download it and pip install --no-deps it.
There was a problem hiding this comment.
I'm fine for alternative approaches, especially if they have precedent somewhere else.
The reason I decided to install CPU only pytorch is that cuml and torch end up installing/depending on CUDA 12 and CUDA 13. There is --index-url https://download.pytorch.org/whl/cu126 so we could use that (via your "download first" approach)?
I could almost be convinced that running with what ever you get "the naive way" (separate pip install commands without the special torch index) is what we should test with. So what we have on main atm. The reason I am half convinced that we should leave it unchanged is that I think that configuration is a likely thing for a real world user to have installed (for better or worse). But it also feels a bit wrong to test that :-/
The "mixed CUDA versions" is a side comment in #8168
There was a problem hiding this comment.
But it also feels a bit wrong to test that :-/
Yeah I'm sympathetic to this (wanting CI to match what users do), but I think in this particular case the job is testing things that are not cuML's responsibility, like the challenges of creating a pip environment with torch wheels and libraries that depend on them, and of incrementally creating a pip environment generally.
From my perspective, especially since this job we're talking about is just a smoke test, it'd be better to make the environment construction more reliable. You could think of that as demonstrating one example of how to get these things working together, which users facing issues in environments constructed other ways could be directed to.
There was a problem hiding this comment.
@betatim @csadorf I pushed changes in 4a8f672
I think we can do something even simpler than the pip download approach I linked above, since this smoke test is the only place that cuml CI currently cares about a torch wheel dependency (as far as I can see).
I tested it locally and saw it work for both CUDA 12 and CUDA 13.
Added this job to PR CI temporarily so we can test here too, but it looks like our environments are still getting Cython 3.2.7 :/
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
BERTopic is directly integrated with cuML and does not require cuml.accel.
I thought so to, but from what I could find in the docs (https://maartengr.github.io/BERTopic/getting_started/dim_reduction/dim_reduction.html#cuml-umap or https://maartengr.github.io/BERTopic/getting_started/clustering/clustering.html#cuml-hdbscan) it does not look like it will use cuml automagically. Which made me think that we can either explicitly pass cuml instances (as shown in the docs) or use |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dependencies.yaml`:
- Around line 563-574: The cuda: "13.*" requirements entry is pinned to a Torch
build that is not available on the cu132 wheel index, causing installs to fail.
Update the packages list under the torch requirements matrix in
dependencies.yaml to use a published cu132 build or relax the torch version pin
to one that actually exists on the cu132 index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e7d5624f-b530-4965-8aee-08ccf61621d7
📒 Files selected for processing (3)
.github/workflows/pr.yamlci/test_wheel_integrations.shdependencies.yaml
| LIBCUML_WHEELHOUSE=$(rapids-download-from-github "$(rapids-artifact-name wheel_cpp libcuml cuml --cuda "$RAPIDS_CUDA_VERSION")") | ||
| CUML_WHEELHOUSE=$(rapids-download-from-github "$(rapids-artifact-name wheel_python cuml cuml --stable --cuda "$RAPIDS_CUDA_VERSION")") | ||
| RAPIDS_TESTS_DIR=${RAPIDS_TESTS_DIR:-"${PWD}/test-results"} | ||
| mkdir -p "${RAPIDS_TESTS_DIR}" |
There was a problem hiding this comment.
RAPIDS_TESTS_DIR isn't used anywhere in this script, it can be removed.
That's right. Our smoke test should implement the recommended path within the BERTopics docs. |
|
merged in |
|
Tests are passing 🎉 I removed the temporary additions that James made, so we can merge this |
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
I think we could add a test with cuml.accel in a follow-up, but right now these smoke tests should test what is actually documented within the docs. LGTM!
Thanks a lot for taking this on.
|
/ok to test 5b56654 |
|
/ok to test 092f91d |
|
Huh, this is a weird one: I'll try updating to latest |
|
/ok to test |
@jameslamb, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test fce383e |
|
/merge |
7 participants
Fixes #8168
This does two things to fix the testing of bertopic. First it runs the integration script with
cuml.accelin order for it to actually use cuml. Right now the test doesn't use cuml.The second thing it does is explicitly install the CPU only version of
torchbefore installingbertopic. This preventssentence-transformers(a dependency ofbertopic) from pulling in atorchversion that brings CUDA 13 packages with it. We don't really need a GPU accelerated version of torch in this test, so this is a simple way to avoid the mixed CUDA environment problem.