-
Notifications
You must be signed in to change notification settings - Fork 245
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
ENH: Add MOOSE3.0 Segmentation Extension #2127
base: main
Are you sure you want to change the base?
Conversation
Add SlicerMOOSE.json
Thanks a lot for your contribution, very nice work! Installation of dependencies worked flawlessly (not an easy thing to do) and I could get some segmentation results. A couple of things to address (many things but mostly small ones that should be easy to address):
|
Hi @lassoan, we are looking into this, we think it's a bug, but we will get back to you in a bit. Many thanks for reviewing the PR, we appreciate your time! |
Just two more things:
Please replace this code:
by something like this:
Models are big and users may have trouble finding and removing model files they no longer need. At least describe in the module documentation where models are installed, but it would be even better to add buttons to the "Advanced" section to manage the models (open the models folder, remove all models), as it is done in MONAIAuto3DSeg: |
@lassoan I also see that the |
added "PyTorch" to build_dependencies for Windows and to be sync with Extension repo
@LalithShiyam @Keyn34 would you be able to confirm if there are any redistribution restrictions applied to the segmentation results produced by your model? Does any license apply to the outputs of this module? |
Hi @fedorov, would GPLV3 the original license of moosev3 be a problem? |
GPL is a source code license, not a license that is designed to handle data. Typically, a license from the family of Creative Commons licenses is selected (Wikipedia provides a nice graph summarizing the gradation from permissive to restrictive https://en.wikipedia.org/wiki/Creative_Commons_license). I do not think we are in a position to enforce any specific license on the output. Obviously, the more permissive license is, the more reusable the segmentations generated would be, and the more use of your software could be anticipated, but it is your decision which license you choose. The reason I asked the question is to document the license so that the users of your extension are fully aware of what they can and cannot do with the segmentation results. (image credit: https://en.wikipedia.org/wiki/Creative_Commons_license#/media/File:Creative_commons_license_spectrum.svg) |
Hi @fedorov many thanks for the detailed explanation, much appreciated. As you can tell. We are newbies. I think CC BY-NC should be ideal for us. But we are open to hear your thoughts! Cheers, |
Of course, I would love to see CC-BY, but it is your choice, and it is consistent with GPL you selected for the source code. Thank you for the clarification! |
In this case, I briefly share my point of view on this. Anything that is released with a restrictive license (GPL, CC BY NC, ...) is mostly ignored by clinicians, researchers, and companies, due to usage restrictions of any "derived work". For most groups, it is safer to just redo the work from scratch than worry about legal risks. If your goal is to make money by selling your models with a commercial license to companies then a restrictive license may be appropriate. However, achieving success with such license is really difficult, because you are on your own (you project will not have contributors or research collaborators, because contributors would not be able to freely use the result of their work), you have to be really good to do better than all the open-source models that are distributed with permissive license; and you have to compete with all the AI companies on cost and quality of the models. If you release your results with a permissive license (MIT, Apache, BSD, ...) then you give up on the possibility to sell a commercial license to companies. However, there are many other ways you get rewarded for your efforts. Since your work can be used clinically, commercially, or as basis of additional research work, suddenly many people become interested in making you successful: they want to work with you, help you, fund you, pay for your expertise and insights. You can achieve much more, you can have much bigger impact, work with many smart people, which all may make life more meaningful, interesting, and enjoyable. Your supervisor, your institution, your funding source, etc. may have certain default preferences, but if you have a specific idea of how you want to do things, then they are usually flexible and supportive. For example, if they initially suggested releasing your work with a non-commercial license, but you are considering opening up, then you may be able to convince them that everyone is better off with using a permissive license. |
I completely agree with the above by @lassoan. I heard about your package a while ago (before this PR), but once I saw that it is GPL, I didn't want to spend any further time looking into it, and I communicated my concerns to my collaborators who considered it. To follow up on the topic of data shared under CC-BY-NC, following Andras response, I can share my perspective. I am a co-PI of the team building Imaging Data Commons (IDC). IDC is more than just a data repository - one of our goals is to work on enrichment of images with analysis results and annotations. Along those lines, we analyzed NLST - one of the largest collections in IDC, >125,000 CT series - using TotalSegmentator and pyradiomics and included analysis results in IDC (you can read more here). It is highly unlikely we would consider doing what we did with TotalSegmentator for your model - even if it is times better than TotalSegmentator - for several reasons. First, under the terms of our contract, all of the software we develop must be released under non-restrictive license (GPL is not one of them), and we would need to develop additional code to apply this model to our data (as we did in https://github.com/ImagingDataCommons/CloudSegmentator). Second, as much as possible we strive to share data under CC-BY license. The few CC-BY-NC collections we have in IDC right now are legacy, and up to now we were able to get all of the new datasets shared under CC-BY. I would like to second Andras: if you can, I strongly encourage you to revisit the license for your code to use a non-viral license, and also revisit the license for the segmentations produced by your code to be CC-BY. There are indeed some precedents where developers reconsidered their license from the original choice of GPL - you can see a related discussion and arguments similar to what Andras said above here: bhklab/med-imagetools#117 (comment). |
Many thanks for taking the time to explain all these facets - we truly appreciate it. We understand things better now and we are happy to change the license to be permissive: Apache and CC-BY. We are sorting out things based on your review, we will send the cleaned up version today. Cheers, |
Hi @lassoan @fedorov MOOSE is now Apache 2.0 and CC-BY: https://github.com/ENHANCE-PET/MOOSE! Cheers, |
Hello everyone, I have a few questions:
I implemented all the suggestions as best as possible. Thanks again!
|
@Keyn34 You will want to make the same change in your fork’s default branch (main) https://github.com/Keyn34/MOOSE as GitHub uses that to display the license in the summary section. Rebasing your main branch against the upstream https://github.com/ENHANCE-PET/MOOSE should fix that. That will help clarify things even though I know you already changed the license file in the SlicerMoose branch. Also do you plan on committing the changes in your fork’s SlicerMoose branch to the upstream? I know Slicer is still hanging onto Python 3.9 currently which was why you had to do those additional changes. I know some Slicer users incorrectly will pip install a package directly to use it in scripting rather than letting the Slicer extension install it. A pip install of moosez would pull from PyPI where a version < 3 of moosez would get installed as that would be the latest to support Python 3.9. You don’t have to commit your changes to the upstream, but it is more of a warning about what the users may do in the meantime haha. Lookout for any of these reports on the Slicer discourse. Slicer aims to update its Python version which will probably happen early next year in which case this extension could go back to using the upstream moosez Python package. |
This becomes a bit of a digression, and I am happy to take this discussion offline or meet separately, but ... Considering the above, you will find a lot of publicly available cancer imaging data in IDC, and furthermore you can relatively easily access any of those images via the SlicerIDCBrowser extension (see this post for details/demo: https://discourse.canceridc.dev/t/sliceridcbrowser-extension-released/515). You could use this data to do more testing, and you could also consider applying your model to a subset of data from IDC and (now that the outputs are covered by CC-BY!) submitting it to IDC - this will demonstrate capabilities of your model, and also enrich IDC data for other users' benefit. If you like, I am happy to meet to discuss this separately. |
@jamesobutler taken care of. The main branch is now up-to-date with the licenses. And thank you for the warning! Right now, we will keep the versions separated, but we want to provide one single version at a later stage.
@fedorov This is very interesting and a great idea. Thanks a lot for the suggestion! We will definitely discuss this internally with the remaining team. |
Hi @fedorov, happy to do so. How can we connect? There is a discord channel in moose which you joined earlier; we can talk there if it's easier. |
@LalithShiyam happy to chat on discord, or you can contact me via gmail andrey dot fedorov! |
Since acvl_utils requires pytorch, we need to install acvl_utils after pytorch. Thanks to @Keyn34 for the tip (Slicer/ExtensionsIndex#2127 (comment)).
Awesome!
Good catch, fixed it! |
Hi everyone, Big thanks to @fedorov for introducing us to IDC! As suggested, we did some random testing on 10 datasets from NLST, where the field-of-view is limited to the thoracic region. Unlike the slicer sample thoracic datasets, Out of the 10 random datasets, The bad news: we still don’t know why it failed on the slicer thoracic datasets. It might be the gadgets attached to the surface of the subject, but I am not sure (even this subject had some clips attached on the chest) - we still need to look into it. P.S: Is there any thing else that we need to do in regards to this extension? |
@Keyn34, what do you mean by this? Why do you say Ubuntu doesn’t require the Slicer PyTorch extension? That would appropriately install the necessary CUDA version for that platform. Can you describe further the issue you had with the install of the CUDA 11.8 version of PyTorch? |
@jamesobutler We noticed when using the CLI or package of MOOSE, Ubuntu users are usually OK with simply running the setup process where the latest version of I use 3DSlicer 5.6.2 on Ubuntu 22.04.5 with an RTX 3090, driver Version 535.216.03, and CUDA Version 12.2. When I install the PyTorch Extension via the Extension Manager and use it to install torch with the default settings, the extension will install When I install the MOOSE extension on top of it, I get the warning: Whenever I need to import MOOSE as a package into the extensions (for fetching the segmentation names and IDs), I get the error:
This does not happen when I install it, as I am doing right now in the extension, where Then, everything works perfectly. On Windows, everything works fine when installing I understand this is not an issue with the PyTorch/MOOSE extension or |
You can ignore this warning. It will not cause any problem for us in Python-3.9 and will go away when Slicer will switch to Python 3.11 soon.
Light-the-torch package is used for automating the installation instructions provided in https://pytorch.org/get-started/locally/ (detect operating system, hardware, and CUDA version). Interestingly in CUDA 12 in linux, the logic of generating the installation command got reversed: to install CUDA-accelerated version you use the default URL, while for CPU version you use a special URL. This logic may not have been implemented in light-the-torch yet, but @jamesobutler have already pushed a fix that should address this (pmeier/light-the-torch#151). @jamesobutler What would you recommend? Should we wait for the light-the-torch update and test again? Or we should implement some quick workaround in this extension until light-the-torch gets fixed? |
I didn't notice this before, but yes it does appear torch has a special condition for linux to by default install the latest CUDA 12.4 computation backend whl when no computation is specified. Philip of Table 2 of https://docs.nvidia.com/cuda/cuda-toolkit-release-notes/index.html states the Minimum Required Driver Version on Linux for CUDA 11.x toolkit is
However, maybe Ubuntu's 535.216.3 broke some compatibility things with CUDA 11.8. |
@lassoan and @jamesobutler, just letting you know, and as I was curious: I updated my driver to version 560.35.05 with CUDA version 12.6. I then installed Then, I installed the MOOSE extension again and tried to run it, which unfortunately produced the same error (and warning) as above. When I installed I checked out the CLI implementation of MOOSE as well: I created a virtual Python 3.10 environment and installed So, I am not sure if it's the driver version. I think it's mainly a problem with the compatibility of the computational backend/ |
New extension
MOOSE (Multi-organ objective segmentation) is a data-centric AI solution that generates multilabel organ segmentations to facilitate systemic TB whole-person research.
Tier 1
Any extension that is listed in the Extensions Catalog must fulfill these requirements.
3d-slicer-extension
GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter3d-slicer-extension
in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topicsSettings
and in repository settings uncheckWiki
,Projects
, andDiscussions
(if they are currently not used).About
in the top-right corner of the repository main page and uncheckReleases
andPackages
(if they are currently not used)Notes: