Skip to content
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.

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

Cuda morphological hausdorff #4078

Open
wants to merge 57 commits into
base: dev
Choose a base branch
from

Conversation

jakubMitura14
Copy link

about
#4053

@wyli
Copy link
Contributor

wyli commented Apr 6, 2022

Thanks, could you please sign the commits and add the necessary copyright section, see: https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md#preparing-pull-requests

@charliebudd
Copy link
Collaborator

charliebudd commented Apr 6, 2022

Looks like we have some failing tests but no worries cause its just an import error. Looks like you need to import your new metirc in the __init__.py file of the metrics module.

I've added a few targeted comments for now, I would say the cuda file is quite long. Is there a sensible way to split it up? even just seperating some declarations into a well commented .cuh header file would provide a good entry point for people looking at the code.

@jakubMitura14
Copy link
Author

jakubMitura14 commented Apr 6, 2022

Deep thanks for your input @charliebudd, about dividing it into separate files, yes It was like that at the begining
https://github.com/jakubMitura14/HousdorffHDF5cMake

But for some reasons particularly in cmake builds (rdc flag is on) occupancy gets lower when multiple files are used.

I also upload pdf with some description of the algorithm (work in progress), just for some reference. look into Algorithm description

Also can I ask how to run the tests locally, with all MONAI?. I mean I have locally the fork with CUDA Hausdorff Distance, and I want to check is it compiles, with the rest of MONAI.

For reference - working python pytorch extension - with benchmarks

Hausdorff.pdf

@wyli
Copy link
Contributor

wyli commented Apr 6, 2022

/black

@jakubMitura14 jakubMitura14 marked this pull request as draft April 6, 2022 16:33
@jakubMitura14 jakubMitura14 marked this pull request as ready for review April 8, 2022 17:55
Signed-off-by: Jakub Mitura <[email protected]>
Signed-off-by: Jakub Mitura <[email protected]>
@jakubMitura14
Copy link
Author

@wyli Hello could you run the workflows?

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 21, 2022

Hi @jakubMitura14 ,

I started the workflows, please sign-off the git commits.

Thanks.

@jakubMitura14
Copy link
Author

jakubMitura14 commented Apr 21, 2022

Hi @jakubMitura14 ,

I started the workflows, please sign-off the git commits.

Thanks.

Thanks @Nic-Ma ! I am signing commits now (I hope that correctly) only some first commits were not - what can I do with this?

Additionally I do not have the access currently to linux machine can I ask You, or somebody else to run

 style fixes: ./runtests.sh --autofix

Thanks!

@wyli
Copy link
Contributor

wyli commented Apr 25, 2022

Hi @jakubMitura14 ,
I started the workflows, please sign-off the git commits.
Thanks.

Thanks @Nic-Ma ! I am signing commits now (I hope that correctly) only some first commits were not - what can I do with this?

Additionally I do not have the access currently to linux machine can I ask You, or somebody else to run

 style fixes: ./runtests.sh --autofix

Thanks!

I can help with this, but to do it more efficiently, would be great if you could review the code again, to remove the syntax errors such as:

<<<<<<< HEAD
=======

>>>>>>> 7fb1821c8c2aa82d8aaa21bfbd1a69237b0ea960

@jakubMitura14
Copy link
Author

jakubMitura14 commented Apr 25, 2022 via email

@wyli
Copy link
Contributor

wyli commented May 2, 2022

there are some test errors, any idea?

======================================================================
ERROR: test_value_12 (tests.test_hausdorff_distance_morphologic.TestHausdorffDistanceMorphological)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/conda/lib/python3.8/site-packages/parameterized/parameterized.py", line 533, in standalone_func
add_video needs package moviepy

    return func(*(a + p.args), **p.kwargs)
  File "/__w/MONAI/MONAI/tests/test_hausdorff_distance_morphologic.py", line 125, in test_value
    hd_metric = MorphologicalHausdorffDistanceMetric(
  File "/__w/MONAI/MONAI/monai/metrics/morphological_hausdorff_distance.py", line 88, in __init__
    self.compiled_extension = load_module("hausdorff")
  File "/__w/MONAI/MONAI/monai/_extensions/loader.py", line 86, in load_module
    module = load(
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1130, in load
    return _jit_compile(
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1368, in _jit_compile
    return _import_module_from_library(name, build_directory, is_python_module)
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1761, in _import_module_from_library
    module = importlib.util.module_from_spec(spec)
  File "<frozen importlib._bootstrap>", line 556, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 1166, in create_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
ImportError: /github/home/.cache/torch_extensions/py38_cu116/hausdorff_Linux_3_8_12_112_11_6/hausdorff_Linux_3_8_12_112_11_6.so: undefined symbol: _Z25getHausdorffDistance_CUDAN2at6TensorES0_iiif

@jakubMitura14
Copy link
Author

jakubMitura14 commented May 2, 2022

there are some test errors, any idea?

======================================================================
ERROR: test_value_12 (tests.test_hausdorff_distance_morphologic.TestHausdorffDistanceMorphological)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/conda/lib/python3.8/site-packages/parameterized/parameterized.py", line 533, in standalone_func
add_video needs package moviepy

    return func(*(a + p.args), **p.kwargs)
  File "/__w/MONAI/MONAI/tests/test_hausdorff_distance_morphologic.py", line 125, in test_value
    hd_metric = MorphologicalHausdorffDistanceMetric(
  File "/__w/MONAI/MONAI/monai/metrics/morphological_hausdorff_distance.py", line 88, in __init__
    self.compiled_extension = load_module("hausdorff")
  File "/__w/MONAI/MONAI/monai/_extensions/loader.py", line 86, in load_module
    module = load(
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1130, in load
    return _jit_compile(
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1368, in _jit_compile
    return _import_module_from_library(name, build_directory, is_python_module)
  File "/opt/conda/lib/python3.8/site-packages/torch/utils/cpp_extension.py", line 1761, in _import_module_from_library
    module = importlib.util.module_from_spec(spec)
  File "<frozen importlib._bootstrap>", line 556, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 1166, in create_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
ImportError: /github/home/.cache/torch_extensions/py38_cu116/hausdorff_Linux_3_8_12_112_11_6/hausdorff_Linux_3_8_12_112_11_6.so: undefined symbol: _Z25getHausdorffDistance_CUDAN2at6TensorES0_iiif

Frankly i do not know how the symbol _Z25getHausdorffDistance_CUDAN2at6TensorES0_iiif appears i suppose it is sth related to pybind but i do not figured it out yet what is the source of the problem

@wyli
Copy link
Contributor

wyli commented May 3, 2022

these commands would give more detailed error messages in colab GPU runtime: https://colab.research.google.com/drive/1szt-G4srXSRyJKpVuKiKhKYH9Mkbk5dO#scrollTo=q1YHjwlQqHi1 in case it's useful for debugging

@jakubMitura14
Copy link
Author

these commands would give more detailed error messages in colab GPU runtime: https://colab.research.google.com/drive/1szt-G4srXSRyJKpVuKiKhKYH9Mkbk5dO#scrollTo=q1YHjwlQqHi1 in case it's useful for debugging

Yes thanks ! it helps - but frankly i do not get why I have
identifier "cudaMallocAsync" is undefined

this is perfectly valid cuda function locally it gave no such errors

hmm

@johnzielke
Copy link
Contributor

Not sure if it is relevant, but the next cupy version will include a CUDA implementation for distance_transform_edt (so only for the euclidean distance case).

@jakubMitura14
Copy link
Author

Intresting - thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants