Upgrade to nanobind 3 by Kayvan-Zahiri · Pull Request #74 · Forest-Neurotech/mach

Kayvan-Zahiri · 2026-09-10T22:25:16Z

Introduction

Upgrades to nanobind 3 in place of the earlier cap below 3.

Changes

Require nanobind>=3.0.1,<4 in build-system.requires, the build group, the Makefile and the CMakeLists setup comment. uv.lock changes only nanobind.
Quote the cibuildwheel before-build specifiers, since the shell read an unquoted >= as a redirect.
kernel.cu: on a CPU-only runner, cudaDriverGetVersion succeeds and reports 0, so the import check warned that driver 0.0 is too old. The pytest "error" filter made PyErr_WarnEx return -1, nothing checked it, and nanobind 3 aborted on the pending exception in enum_create("InterpolationType"). Now a failed version query or a version of 0 warns that no NVIDIA driver was found, and the import-time PyErr_WarnEx calls throw nb::python_error on failure.
pyproject.toml: filterwarnings ignores the no-driver warning, so the CPU-only tests still run with warnings as errors.

Behavior

Importing mach without a driver now warns [mach] No NVIDIA driver was found. CUDA beamforming is unavailable. Locally, uv lock --locked (uv 0.7.3) and pre-commit pass. _cuda_impl.pyi is not regenerated, since that needs a CUDA build.

Fork CI on this branch, with only CI-only steps added on top (push triggers and two import checks): Test-CPU passed (47 tests, nanobind 3.0.1), and its import step printed the warning. With -W error::RuntimeWarning, the import failed with a Python error, not an abort. Check and Build wheels passed.

The GPU tests and the docs build have not run against nanobind 3.

qodo-code-review · 2026-09-10T22:26:44Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~Local builds still accept version 3~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

[dependency-groups].build and CMake's documented no-isolation setup still request unbounded
nanobind>=2.7 or nanobind, while make compile also begins with an unbounded installation.
Developers following the CMake instructions can receive version 3 immediately, and after a lock
refresh the Makefile compilation path can resolve it as well.

Code

pyproject.toml[2]

+requires = ["scikit-build-core>=0.10", "nanobind>=2.7,<3"]

Evidence

The changed isolated-build requirement excludes nanobind 3, but the build dependency group remains
>=2.7, and that unconstrained specifier is preserved in the lock metadata. Both the Makefile and
CMake developer instructions perform no-isolation builds using dependencies installed through paths
that remain unbounded.

pyproject.toml[1-3]
pyproject.toml[90-94]
Makefile[43-49]
CMakeLists.txt[19-25]
uv.lock[1807-1811]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new nanobind upper bound is absent from the developer build dependency group and no-isolation setup commands, so those paths can still install the incompatible major version.

## Fix Focus Areas
- pyproject.toml[90-93]
- Makefile[43-49]
- CMakeLists.txt[19-25]
- uv.lock[1807-1811]

## Recommended Fix
Change every developer-facing nanobind requirement to `nanobind>=2.7,<3`, including the build dependency group and installation commands, then regenerate `uv.lock` so its build-group metadata records the upper bound.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

Review mode: ⚖️ Balanced: This changes build dependency constraints and CI wheel-build shell commands, with runtime/build compatibility impact and a previously identified inconsistency in local build paths.

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

qodo-code-review · 2026-09-10T22:27:09Z

Code review by qodo was updated up to the latest commit 09b60d3

charlesbmi · 2026-09-11T00:18:54Z

Thanks for the contribution. Did you happen to take a look at what the nanobind 3 upgrade would require? I believe it's supposed to not make many breaking changess

Kayvan-Zahiri · 2026-09-11T03:22:34Z

I looked, and the upgrade itself looks small. nanobind 3 isn't really the problem.

On a CPU-only runner, cudaDriverGetVersion succeeds and reports 0, so checkCudaDriverCompatibility() warns that driver "0.0" is too old. pytest's filterwarnings = ["error", ...] turns that warning into an exception, and PyErr_WarnEx returns -1, but the return value isn't checked, so the exception stays pending through module init. nanobind 3 aborts on it inside enum_create. 2.12 happened to get past it.

On my fork, building against nanobind>=3.0.1 with two changes in kernel.cu passes Test-CPU, 47 passed: https://github.com/Kayvan-Zahiri/mach/actions/runs/34557776115

return early when the driver version is 0, since no driver means there is nothing too old to warn about
throw nb::python_error() when PyErr_WarnEx returns -1, so warnings-as-errors surfaces as a real import error

A run that printed the pending error showed the warning text: https://github.com/Kayvan-Zahiri/mach/actions/runs/34557575757

I haven't run the GPU tests or the wheel build against nanobind 3, since my fork has no GPU runner.

Would you rather I turn this PR into the nanobind 3 upgrade, or keep the cap for now and open the upgrade separately?

charlesbmi · 2026-09-11T03:37:20Z

Thanks @Kayvan-Zahiri ! Yeah, since it's pretty well-scoped, let's just turn this into a nanobind 3 upgrade. I'll separately look into avoiding these issues in the future by making a uv.lock file for the build tools, which don't require all of the package dependencies

Require nanobind>=3.0.1,<4 in build-system.requires, the build dependency group, uv.lock, the cibuildwheel before-build step, the Makefile compile target and the CMakeLists setup comment. The before-build specifiers are now quoted so the shell no longer reads >= as a redirect. On a CPU-only runner cudaDriverGetVersion succeeds and reports 0, so checkCudaDriverCompatibility warned that driver 0.0 is too old. Under the pytest "error" warning filter PyErr_WarnEx returned -1, the exception stayed pending through module init, and nanobind 3.0.1 aborted in enum_create. Skip the version comparison when the driver version is 0, and throw nb::python_error when any of the three import-time warnings fails.

Kayvan-Zahiri · 2026-09-11T18:25:42Z

Done, this is now the nanobind 3 upgrade, with the two kernel.cu changes from my last comment. On my fork the new commit passes Test-CPU (47 tests), Check, and Build wheels with the import test on 3.11 to 3.14.

Test-GPU and the docs build haven't run on this PR. Pushes don't trigger them (their pull_request types are opened, reopened, ready_for_review), their only runs here are from the first commit and await approval, and a manual dispatch can only target a branch or tag in this repo. Closing and reopening the PR should start them, though I haven't tried that.

A uv.lock for the build tools sounds good.

charlesbmi · 2026-09-11T19:37:06Z

    if (cudaDriverGetVersion(&driverVersion) != cudaSuccess) {
-        PyErr_WarnEx(PyExc_RuntimeWarning, "Could not get CUDA driver version", 1);
+        if (PyErr_WarnEx(PyExc_RuntimeWarning, "Could not get CUDA driver version", 1) < 0) {
+            throw nb::python_error();
+        }
+        return;
+    }
+
+    // The runtime reports 0 when no driver is installed
+    if (driverVersion == 0) {
        return;
    }


Suggest warning on missing driver - so it's clear at import time that beamforming won't work (although users can still use geometry, etc, and functionality that we run through Test-CPU job

So you could consolidate these 2 checks into:

if (cudaDriverGetVersion(&driverVersion) != cudaSuccess || driverVersion == 0) { // cudaDriverGetVersion reports 0 when no driver is installed if (PyErr_WarnEx( PyExc_RuntimeWarning, "[mach] No NVIDIA driver was found. CUDA beamforming is unavailable. ", 1) < 0) { throw nb::python_error(); } return; }

assuming you take this suggestion to also warn when the driver is missing (CPU-only node), you'll need to update the pytest filterwarnings as well:

filterwarnings = [ "error", "ignore:\\[mach\\] No NVIDIA driver was found.*:RuntimeWarning", "ignore:.*This will add latency due to CPU<->GPU memory transfers.*:UserWarning", # vbeam warnings ... ]

edit: probably an easier to read filterwarnings match would be:

"ignore:.*No NVIDIA driver was found.*:RuntimeWarning",

charlesbmi · 2026-09-11T21:56:37Z

Thanks for the update & fix! I think it would still be helpful to have a warning if a driver was absent, as almost all users of this library are interested in the CUDA kernel, even though there is some functionality that does not require a GPU (tested by test_cpu.yml), only remaining suggestion would be to still warn on missing driver and update filterwarnings correspondingly (see inline comments). Will approve & run workflows manually & merge after that, thanks!

cudaDriverGetVersion reports 0 when no driver is installed. Warn at import in that case, and when the version query fails, so it is clear that CUDA beamforming is unavailable. pytest's filterwarnings ignores this warning, so the CPU-only test job still runs with warnings as errors.

Kayvan-Zahiri · 2026-09-11T23:03:12Z

Done. It now warns [mach] No NVIDIA driver was found. CUDA beamforming is unavailable. when the version query fails or reports 0, and filterwarnings ignores that warning with your simpler match.

On my fork, Test-CPU passes with 47 tests, and a CI-only step that imports mach on the CPU runner prints the warning: https://github.com/Kayvan-Zahiri/mach/actions/runs/34655201037. Check and Build wheels pass too. The GPU tests haven't run.

charlesbmi · 2026-09-11T23:41:26Z

tests passed, merging. thanks!

https://github.com/Forest-Neurotech/mach/actions/runs/34657566517

https://github.com/Forest-Neurotech/mach/actions/runs/34657568484

qodo-code-review Bot reviewed Sep 10, 2026

View reviewed changes

Comment thread pyproject.toml Outdated

Kayvan-Zahiri force-pushed the fix/pin-nanobind-below-3 branch from 20e2e6c to e82f865 Compare September 11, 2026 18:24

Kayvan-Zahiri changed the title ~~Build against nanobind below 3, and quote the wheel build requirements~~ Upgrade to nanobind 3 Sep 11, 2026

charlesbmi reviewed Sep 11, 2026

View reviewed changes

Kayvan-Zahiri mentioned this pull request Sep 11, 2026

Fail the benchmark job when performance regresses #73

Open

3 tasks

charlesbmi merged commit c1a81d3 into Forest-Neurotech:main Sep 11, 2026
14 checks passed

Navigation Menu

Upgrade to nanobind 3 - #74

Upgrade to nanobind 3#74
charlesbmi merged 2 commits into
Forest-Neurotech:mainfrom
Kayvan-Zahiri:fix/pin-nanobind-below-3

Kayvan-Zahiri commented Sep 10, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review Bot commented Sep 10, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi Sep 11, 2026

Uh oh!

charlesbmi Sep 11, 2026

Uh oh!

charlesbmi Sep 11, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Kayvan-Zahiri commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

Changes

Behavior

Uh oh!

qodo-code-review Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

qodo-code-review Bot commented Sep 10, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

charlesbmi Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

charlesbmi Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Kayvan-Zahiri commented Sep 11, 2026

Uh oh!

charlesbmi commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kayvan-Zahiri commented Sep 10, 2026 •

edited

Loading

qodo-code-review Bot commented Sep 10, 2026 •

edited

Loading