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

2023.12.12: An extension displays a blocking popup related to CUDA installation #1991

Open
jcfr opened this issue Dec 12, 2023 · 17 comments
Open
Labels
Type: Bug Something's not working correctly.

Comments

@jcfr
Copy link
Member

jcfr commented Dec 12, 2023

image

@lassoan
Copy link
Contributor

lassoan commented Dec 12, 2023

This is so wrong in several aspects:

  • slicer.util functions should be used for displaying messages (they don't block execution in testing mode)
  • no need to exclude Windows or macOS
  • no need for low-level GPU access (should be done via SlicerPyTorch)

We can temporarily remove the extension while the maintainers fix the issues.

@jcfr
Copy link
Member Author

jcfr commented Dec 12, 2023

I will follow-up here once I identified the culprit.

@jcfr
Copy link
Member Author

jcfr commented Dec 13, 2023

Here is the code leading the popup:

https://github.com/DCBIA-OrthoLab/SlicerDentalModelSeg/blob/00816c31c570e41a5d580e291519dbf8b541d95e/CrownSegmentation/CrownSegmentation.py#L216-L226

The extension was originally introduced through this pull request:

The following comment originally posted by @pieper in #1867 (comment) is still relevant:

Please in the readme be sure to point out that this code depends on CUDA. It looks like there's already a fallback to CPU mode so describing the tradeoffs in the readme would be helpful.

Longer term, you should see if you can rework the installation code to use SlicerPyTorch to get the right code installed.

Also it would help if you provide sample data. I see some in the repository so those should be referenced in the readme or even better registered with the SampleData module so it's easy for users to test the module.

In the same vein, it looks like you don't have any tests.

The code was originally contributed by @MathieuLeclercq and seems to be maintained by @GaelleLeroux with some input from @allemangD

@jcfr
Copy link
Member Author

jcfr commented Dec 13, 2023

@GaelleLeroux With some guidance from us, would you have the bandwidth to help improve this extension ?

@jcfr jcfr added the Type: Bug Something's not working correctly. label Dec 13, 2023
@GaelleLeroux
Copy link
Contributor

@jcfr I'm currently trying to update this module. @juanprietob and @FlorianDAVAUX have released shapeaxi, a new library that includes teeth segmentation. I'm working on changing the code to use it instead of the previous code. @juanprietob, @FlorianDAVAUX do you know if it's still necessary to have a CUDA capable GPU to download and use dentalmodelseg in shapeaxi? If so, how can I improve the code to avoid it ?

@jcfr
Copy link
Member Author

jcfr commented Dec 13, 2023

I'm working on changing the code to use it instead of the previous code

This is great. Thanks for working on this 🙏

released shapeaxi

See https://pypi.org/project/shapeaxi/

do you know if it's still necessary to have a CUDA capable GPU

Short answer is yes.

It looks like it depends on pytorch3d, this means that this extension could probably updated to depend on SlicerPyTorch1

See also:

Footnotes

  1. https://github.com/fepegar/SlicerPyTorch

@lassoan
Copy link
Contributor

lassoan commented Dec 13, 2023

It looks like it depends on pytorch3d, this means that this extension could probably updated to depend on SlicerPyTorch

pytorch3d is a one-man project. The developer uses linux and cuda and the package practically does not work in any other environment. pytorch3d uses pytorch and SlicerPyTorch would take care of the installation of pytorch, but that's not sufficient to make pytorch3d work.

It would be great if you could get rid of pytorch3d, because the dependency on pytorch3d makes the extension inaccessible for 99% of Slicer users (12% of Slicer users are on Linux, probably less than 10% have a CUDA-capable GPU).

@GaelleLeroux
Copy link
Contributor

I've managed to use pytorch3d on Windows, so it's more accessible, but it still requires a CUDA-compatible GPU...

@lassoan
Copy link
Contributor

lassoan commented Dec 13, 2023

It probably only worked because you are a developer and had a C++ compiler, CUDA SDK, etc. all set up on your computer.

@GaelleLeroux
Copy link
Contributor

Yes, you're right, but we can't get rid of pytorch3d and I haven't found a way to install pytorch3d without CUDA. I think first of all this module needs a powerful computer to run properly, so maybe we can say it's a requirement to have a CUDA compatible GPU. I don't have any idea how to solve this problem... if you have one I would be interested to work on it.

@lassoan
Copy link
Contributor

lassoan commented Dec 14, 2023

Making your software work on an average computer (Windows, macOS, Linux / CPU, CUDA, or Apple MPS backend) is essential for your software to make an impact beyond publishing papers. If users can easily try your software on their current system and see that it works (just at reduced quality or longer computation time) then they will use it and can decide to invest into getting a new computer with a strong discrete GPU to get results faster or higher quality.

I understand that it can be hard to change a dependency once you have built a whole lot of things on it and there is no high-quality drop-in replacement. Maybe you could reach out to pytorch3d developers to clarify their available resources, commitment, and long-term plans for the package.

  • If they do not plan to provide wheels for common platforms then it means that they do not intend this package for end users, only for researchers/developers. Your goals and motivations may be different, but for me, this would disqualify this pytorch3d as a dependency, as I would not want to invest my time into something that I won't be able to get to the hands of users.
  • If they would like to make pytorch3d available for users they just don't have the expertise or resources then there is a bit more hope for this package. If you strongly believe that pytorch3d is the future for mesh processing on modern computing hardware then you may consider contributing to pytorch3d - with improving their build system, create wheels for many platforms (Windows, macOS, common Linux variants), implement alternatives for components that currently require CUDA, apply for grants that funds their and your time to do this work, etc.

@GaelleLeroux
Copy link
Contributor

Thanks so much @lassoan for the response and for raising such relevant points.
I acknowledge that the dependency on Pytorch3D and the requirement for high-performance graphics cards with CUDA capabilities does pose a significant challenge in terms of broadening our user base. At present, we do not have an immediate solution to modify this dependency, which is indeed a critical aspect of our consideration for future developments.
When considering the computing infrastructure required for running AI tools, it's crucial to recognize that even robust modules like TotalSegmentator demand substantial computing power. For users interested in effectively utilizing AI tools, having a computer without CUDA capabilities significantly limits efficiency and performance. Consequently, a CUDA-enabled system is not just advantageous but increasingly necessary for engaging with advanced AI applications.
I have brought @juanprietob, @bpaniagua, and @allemangD into this conversation, as they can provide more insights and future plans regarding the development of our surface mesh Shape Analysis modules, including DentalModelSeg, ALIIOS, ARegIOS, FlexReg, and ShapeAxi. Their perspectives will be invaluable in addressing the high-level planning and strategic directions we might take.

@jamesobutler
Copy link
Contributor

In the context of the Slicer extensions index, an extension published through this method should probably at minimum have support for macOS or Windows (not windows subsystem for Linux (WSL)) as that will capture a significant user base. This would be for the sake of the Slicer community that extension functionality is cross platform and can be run by a reasonable end-user that doesn’t have to be a developer. There is often frustrations within the Slicer community when new AI tools don’t “just work”.

If an extension is unable to reach those expectations, I think it would be best for the extension to be self-published with distribution handled by those developers as they will likely be providing support anyways for specific dependencies and other setup. It appears SlicerDentalModelSeg is still in the research and development stage which hasn’t yet made things generally easy-to-use for the Slicer community although it may be fine for the relevant research lab.

@lassoan
Copy link
Contributor

lassoan commented Dec 14, 2023

@GaelleLeroux Thanks for the consideration. Just a note on TotalSegmentator: one reason it is very widely used because it does not require a GPU, but it can run on CPU as quickly as on GPU but at half resolution; or it can provide full-resolution results in 30-40 minutes (about 10x longer time than computation takes on GPU). This works amazingly well, because users can verify in a few minutes that TotalSegmentator works well on their data and they can decide to either stop there (if they don't have more time or don't need better-quality results) or wait for 40 minutes for full-quality results. Therefore, lack of GPU becomes a small inconvenience in most use cases.

@jamesobutler I agree that it can be frustrating for users when they don't know that the extension is not at that maturity level that they expect. Requiring extensions to fulfill some requirements before allowing them to show up in the Extensions Manager would be a simple solution, but perhaps a better one would be to show the maturity level of the extension clearly in the Extensions Manager, maybe set default filter settings to only show well-tested, widely usable ones. I've added an issue to track this: Slicer/Slicer#7474 (I thought we had an issue for this already but I could not find it)

@GaelleLeroux
Copy link
Contributor

@lassoan @bpaniagua @allemangD @juanprietob
Regarding the maturity level indication in the Extensions Manager and your suggestion of setting default filters, it will indeed help in managing user expectations effectively. However, I have a query related to our extension's visibility with these changes. Will users still be able to see and download our extensions, even if they are not categorized under the 'well-tested, widely usable' filter? It's important for us to ensure that our extension remains accessible to those who might benefit from it.
Additionally, regarding system compatibility issues, we can include detailed system requirements in our extension's description and add a pop-up window for users, which would appear when first opening the extension. Would this approach be in line with the proposed modifications to the Extensions Manager?

@lassoan
Copy link
Contributor

lassoan commented Dec 14, 2023

This Extensions Manager feature will probably take several months to implement and there areany higher-priority tasks, so it inot clear when development would start. But it's good that you have raised these questions.

@GaelleLeroux
Copy link
Contributor

Thank you for your response, I understand that there are numerous higher-priority tasks at hand and that the development of this feature may take some time. Let me know if there is any way I can assist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something's not working correctly.
Development

No branches or pull requests

4 participants