Skip to content

Move to nanobind #455

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

Move to nanobind #455

wants to merge 54 commits into from

Conversation

Intron7
Copy link
Member

@Intron7 Intron7 commented Sep 11, 2025

This for now is a test with precompiled cuda extensions.

@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 11, 2025
@Intron7 Intron7 marked this pull request as draft September 11, 2025 19:02
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 11, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 11, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 11, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 11, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 11, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 12, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 12, 2025
flying-sheep
flying-sheep reviewed Sep 12, 2025
View reviewed changes
Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

Oh wow, that looks super cool! Addresses all my previous concerns about manual specification of magic numbers for chunking, repetitive kernel wrapper code, and so on!

src/rapids_singlecell/preprocessing/_qc.py Outdated
from rapids_singlecell._cuda import _qc_cuda as _qc

if sparse.isspmatrix_csr(X):
_qc.sparse_qc_csr(
Copy link
Member

Choose a reason for hiding this comment

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

ditto, for all of these

@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 16, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 16, 2025
flying-sheep
flying-sheep approved these changes Sep 16, 2025
View reviewed changes
Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

Looks awesome!

I didn’t go through the C++ code assuming it’s a straight port.

docker/manylinux_2_28_aarch64_cuda12.9.Dockerfile
Copy link
Member

Choose a reason for hiding this comment

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

would be good to check if these two are the same except from the base image. there’s already some drift with the comments and indentation!

@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 16, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 16, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 17, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 17, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 17, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 17, 2025
@Intron7
Copy link
Member Author

Intron7 commented Sep 18, 2025 •
edited
Loading

Todo:

  • Protect Kernels against NullPointers
  • Figure out if pointers or nb:arrays are the better option for passing arg
  • add keywords
  • Figure out cuda-13 support

flying-sheep
flying-sheep reviewed Sep 18, 2025
View reviewed changes
Copy link
Member

@flying-sheep flying-sheep left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add positional-only arguments.

also please answer #455 (comment)

Figure out if pointers or nb:arrays are the better option for passing arg

probably arrays, why would you do pointers if nanobind has first-class numba support?

most importantly, I see some int(something.data.ptr), you should definitely never do that.

@flying-sheep flying-sheep added the run-gpu-ci runs GPU CI label Sep 18, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 18, 2025
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 18, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 18, 2025
flying-sheep
flying-sheep approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

Looks good!

@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Sep 22, 2025
@github-actions github-actions bot removed the run-gpu-ci runs GPU CI label Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flying-sheep flying-sheep flying-sheep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Intron7 @flying-sheep