Skip to content

Conversation

aymuos15
Copy link
Collaborator

Benchmarking cupy based component labelling

In benchmark/benchmark.py, I have now added cupy option to the mix. I think this is the first step to understand why this would be useful. I think the results are very interesting and the overall repo would benefit just shifting this step to the gpu. The initial discussion was made in #209

Quick tally:

Volume Size: (500, 500)
Scipy Time: 0.0326 seconds
CC3D Time: 0.0568 seconds
CuPy Time: 0.0046 seconds

Volume Size: (1000, 1000)
Scipy Time: 0.1239 seconds
CC3D Time: 0.2285 seconds
CuPy Time: 0.0111 seconds

Volume Size: (2000, 2000)
Scipy Time: 0.5010 seconds
CC3D Time: 0.9412 seconds
CuPy Time: 0.0377 seconds

Volume Size: (50, 50, 50)
Scipy Time: 0.0207 seconds
CC3D Time: 0.0149 seconds
CuPy Time: 0.0037 seconds

Volume Size: (100, 100, 100)
Scipy Time: 0.1556 seconds
CC3D Time: 0.1170 seconds
CuPy Time: 0.0129 seconds

Volume Size: (200, 200, 200)
Scipy Time: 1.2197 seconds
CC3D Time: 0.9419 seconds
CuPy Time: 0.0853 seconds

Volume Size: (512, 512, 512)
Scipy Time: 21.7650 seconds
CC3D Time: 16.8670 seconds
CuPy Time: 1.2582 seconds

If this is agreed upon, I can try having a stab at the best way to integrate this all over the repo.

The only downside is, in my own experience, sometimes cupy can throw install errors. Again, it is not mandatory to have this, it can automatically fall back to the cpu.

Bumping numpy to v2

This PR also bumps numpy to the latest version. It is based on the discussion in #216 and solves #211

Running the entire test suite throws no errors what so ever.

I think this is important because people are unable to use panoptica on colab at the moment. I think that is very rate-limiting for adoption.

@Copilot Copilot AI review requested due to automatic review settings June 17, 2025 14:41
Copilot

This comment was marked as outdated.

@aymuos15
Copy link
Collaborator Author

aymuos15 commented Jun 17, 2025

Interestingly, just found out that cupy does not "yet" support numpy v2 numpy/numpy#26191

For now, numpy is downgraded if user requests gpu.

@Hendrik-code
Copy link
Collaborator

Hey
Sorry for the late answer. The results are impressive. I would love for panoptica to support cupy at some point. So if you wanna have a stab at it, please be my guest. :)

@neuronflow
Copy link
Contributor

neuronflow commented Jul 4, 2025

I agree very cool stuff.

The only downside is, in my own experience, sometimes cupy can throw install errors. Again, it is not mandatory to have this, it can automatically fall back to the CPU.

This can be solved like here:
https://github.com/BrainLesion/preprocessing/blob/1413eb9b83083ff7abb943e29930014499ef4abe/pyproject.toml#L73C1-L81C32

@neuronflow
Copy link
Contributor

this closes #211 correct @aymuos15 ?

@neuronflow neuronflow added the enhancement New feature or request label Jul 4, 2025
@aymuos15
Copy link
Collaborator Author

aymuos15 commented Jul 4, 2025

This can be solved like here:
https://github.com/BrainLesion/preprocessing/blob/1413eb9b83083ff7abb943e29930014499ef4abe/pyproject.toml#L73C1-L81C32

Did add that to the toml in this commit. But I will keep that in mind before the final push.

this closes #211 correct @aymuos15 ?

It does!

Thanks @Hendrik-code and @neuronflow . Ill work on this and make a commit soon.

@neuronflow
Copy link
Contributor

should it also close #216 ?

@aymuos15
Copy link
Collaborator Author

aymuos15 commented Jul 4, 2025

It should!

If, by the time this is integrated and cupy still does not support numpyv2, I think a warning/notice should be done somewhere which says that using "gpu acceleration" will bump you down to numpyv1. What do you think?

@neuronflow neuronflow linked an issue Jul 4, 2025 that may be closed by this pull request
@neuronflow
Copy link
Contributor

this is still a draft correct @aymuos15 ?

@aymuos15 aymuos15 marked this pull request as draft July 27, 2025 15:49
@aymuos15
Copy link
Collaborator Author

Yes. Will work on this once the part stuff is merged.

@aymuos15
Copy link
Collaborator Author

@Hendrik-code @neuronflow

This is my first pass for the cupy shift. Instead of changing the entire repo, I think starting with just connected component step should be sufficient boost.

In terms of the ci errors, may I suggest using:

python -m poetry install --all-extras

Instead of:

python -m poetry install

in the tests.yml for the github workflows

@Hendrik-code
Copy link
Collaborator

@Hendrik-code @neuronflow

This is my first pass for the cupy shift. Instead of changing the entire repo, I think starting with just connected component step should be sufficient boost.

In terms of the ci errors, may I suggest using:

python -m poetry install --all-extras

Instead of:

python -m poetry install

in the tests.yml for the github workflows

yes, sounds good. Go for it! :)

@aymuos15
Copy link
Collaborator Author

Awesome! Thanks a lot :D

Since this is a dotfile change, do you mind pushing a PR for it? I think not altering the .gitignore or dotfile in this PR would be better?

@neuronflow
Copy link
Contributor

@aymuos15 yes, please go for the dotfile PR

@aymuos15 aymuos15 marked this pull request as ready for review August 30, 2025 11:51
@aymuos15 aymuos15 requested a review from Copilot August 30, 2025 11:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds CuPy GPU-accelerated support for connected component labeling and upgrades numpy to version 2. The changes aim to improve performance for large-scale connected component operations by leveraging GPU computation.

  • Adds CuPy backend for GPU-accelerated connected component labeling with significant performance improvements
  • Upgrades numpy to version 2 to resolve compatibility issues and enable usage in Google Colab
  • Implements comprehensive test coverage for the new CuPy functionality with proper fallback handling

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
unit_tests/test_cupy_connected_components.py Complete test suite for CuPy backend functionality with proper error handling and fallbacks
unit_tests/test_config.py Updates config tests to include CuPy backend validation
pyproject.toml Adds optional CuPy dependencies with CUDA version variants
panoptica/utils/constants.py Extends CCABackend enum to include cupy option
panoptica/_functionals.py Implements CuPy backend in connected components function
benchmark/benchmark.py Adds CuPy benchmarking capabilities with performance comparisons
.github/workflows/tests.yml Adds CI workflow for testing CUDA functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +68 to +69
import cupy as cp
from cupyx.scipy.ndimage import label as cp_label
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

These imports should be moved outside the try block to avoid repeated import overhead. Consider moving these imports to the module level with a try-except block to check CuPy availability once.

Copilot uses AI. Check for mistakes.

Comment on lines +85 to +91
float: Time taken to label the mask in seconds, or None if CuPy is not available.
"""
if not CUPY_AVAILABLE:
return None

# Transfer data to GPU
mask_gpu = cp.asarray(mask)
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

GPU memory allocation should include error handling for out-of-memory conditions, especially for large arrays. Consider wrapping this in a try-except block to catch CUDA memory errors.

Suggested change
float: Time taken to label the mask in seconds, or None if CuPy is not available.
"""
if not CUPY_AVAILABLE:
return None
# Transfer data to GPU
mask_gpu = cp.asarray(mask)
float: Time taken to label the mask in seconds, or None if CuPy is not available or out of memory.
"""
if not CUPY_AVAILABLE:
return None
# Transfer data to GPU
try:
mask_gpu = cp.asarray(mask)
except cp.cuda.memory.OutOfMemoryError as e:
print("CuPy OutOfMemoryError: Unable to allocate GPU memory for mask. Skipping GPU benchmark.")
return None
except cp.cuda.memory.MemoryError as e:
print("CuPy MemoryError: Unable to allocate GPU memory for mask. Skipping GPU benchmark.")
return None

Copilot uses AI. Check for mistakes.

Comment on lines +105 to +106
del mask_gpu
cp.get_default_memory_pool().free_all_blocks()
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

Calling free_all_blocks() after every benchmark run is overly aggressive and may impact performance measurements. Consider using cp.get_default_memory_pool().free_all_blocks() only at the end of all benchmarks or using a context manager for better memory management.

Suggested change
del mask_gpu
cp.get_default_memory_pool().free_all_blocks()

Copilot uses AI. Check for mistakes.

@neuronflow
Copy link
Contributor

hmm what's going on with our tests here? :)

strategy:
fail-fast: false
matrix:
python-version: ["3.10", "3.11", "3.12"]
Copy link
Contributor

Choose a reason for hiding this comment

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

should this include 3.13 @aymuos15 ?

@Hendrik-code
Copy link
Collaborator

@aymuos15 my plan would be to merge #223 first, then this.
Is that fine with you? If so, it only makes sense for me to make my final review here once the other is merged and integrated

@aymuos15
Copy link
Collaborator Author

aymuos15 commented Sep 1, 2025

@Hendrik-code that totally works! I will look into the part breaking aspect of that today.

@neuronflow I will that change post hendrik's merge then. Thank you for pointing that out!

@neuronflow
Copy link
Contributor

@aymuos15 probably the same poetry issue here.

@neuronflow neuronflow linked an issue Sep 25, 2025 that may be closed by this pull request
@Hendrik-code
Copy link
Collaborator

Now that #228 is merged, we merge this, right? (sorry, this got confusing for me) @aymuos15 @neuronflow

@aymuos15
Copy link
Collaborator Author

I would suggest just keep this at the end. Best not to mix the cupy stuff within the actual repo now before the voronoi stuff is done?

@neuronflow
Copy link
Contributor

Now that #228 is merged, we merge this, right? (sorry, this got confusing for me) @aymuos15 @neuronflow

I also got confused. Probably we should work with proper tickets/issues and relationships in the future to avoid such a situation? :)
Screenshot 2025-09-26 at 21 57 03

@aymuos15
Copy link
Collaborator Author

Agreed. I think there were too many things happening at once, which were dependent on each other unfortunately and my main branch on my fork was polluted from previous work which kinda messed things up. Will definitely keep this in mind going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

3 participants