Add SlicerNeuropacs Extension #2096
Conversation
neuropacman
changed the title
|
Generally this looks good to me.
Where it is now will slow down startup times, so better to wait until the module is entered. Something like this:
That's usually in a place that will get erased on reboot or cleared if it gets full. If these orders are something you think the user will want to persist over time, better to store in |
|
@pieper Thank you for your fast response! I will make the recommended changes. One more question, when you say "but also give the user a chance to set it someplace of their choice.", how/where can I save this selection within Slicer so this path is restored when the module initializes? Or is this something I would need to store externally? |
Under the hood we use the In python you can use |
|
For semi-persistent storage (content is preserved until a storage space limit is exceeded) can also use the Slicer cache folder: import slicer
# Get the cache folder path
cache_folder = slicer.mrmlScene.GetCacheManager().GetRemoteCacheDirectory()
print("Cache folder:", cache_folder) |
|
@pieper I have made the recommended changes on my main branch. Please let me know if there is anything else I need to do! |
|
I've had a look and found the followings: It would make sense to fix before making the extension available:
Other improvements to consider:
|
|
Major blocking issue found: neuropacs Python package pins exact version of all its dependencies! https://github.com/neuropacs/neuropacs-py-api/blob/main/setup.py#L15C1-L21C23 This makes it impossible to install most other Python packages and therefore it breaks many other Slicer extensions. I've just tried to install MONAIAuto3DSeg and segmentation failed to start (I think due to zipp version pinned). A Python application can pin any dependencies it wants, but a Python library cannot do that. A Python library should specify its actual requirements as permissively as possible. It is of course extra work for the maintainer to test with a range of versions of dependencies but this is required if the package is intended to be used along with other Python packages. |
|
For reference I had already gone ahead and written up neuropacs/neuropacs-py-api#18. |
|
Hi @jamesobutler @lassoan @pieper, Thank you for the extensive feedback. I have made the following improvements:
Please let me know if there is anything else! Quick question: Will the "Reload and Test" widget view be removed in the prod environment? I do not see how/where it is being rendered. |
This is controlled by the Developer Mode setting, so you don't need to worry about it. |
|
Thanks for the update. Relaxing the Python requirements should address my main concerns. I could not actually test the extension, because the instruction for getting an API key just takes me to a "request a demo" form. I would not want to spend time with requesting a demo, waiting for an answer, etc. and I think I'm not alone. There are so many competing solutions, so the if you are confident that your solution is better then I would recommend to set up a demo server with a publicly available API key (or at least an API key that is automatically sent when you register a free account) and let the product speak for itself. |
|
Hi @lassoan, at this point I am waiting on others within my company to create a process for obtaining an API key. I am not involved with this. I was instructed to create this extension to prepare for the future when we are ready to release our product. We do not provide any facility for a demo key or anything like this. Although, we do have a dev environment that we use in our own testing that I can provide an API key for so that you can perform testing on your end. Although this dev environment has a different URL that is hard-coded into the extension so I can perhaps create a new branch that uses this test URL you can use. If this is not feasible, you can test on prod but I will give you a key that only has 5 uses. Let me know what you think. |
|
I can spend up to an hour on testing this and give feedback/recommendations if you send me a URL and API key (you can send me to my email). I can modify a .py file to use a custom URL (it helps if you tell me which one and where), no need to create a new branch for that. |
|
Hi @lassoan. Great. I have just sent you an email. Please let me know if you have any questions/issues during testing. |
|
Is there an update on this? |
|
Hi @lassoan @pieper @jamesobutler, please provide an update on this. |
|
Thank you for your patience. We are very busy with preparing the new Slicer Stable Release (5.8). I'll try to get back to this today. |
|
I've tried to test this. Updated the server URL, entered the API key, clicked I've found where the exception information was discarded and added: After this I retried and got more useful information: This indicated that when I clicked I then created a dummy order file (you provided an example in your email, I don't know where interested users would get that information) and tried to set it in the "Order file path". I struggled a lot with this, as clicking Then I realized the very specific requirements that are needed for the input file that was in the PDF (from where itnerested users would know this?) and at this point I gave up. If there was a link in the tutorial I would have continues, but I would not like to write an email, wait for a response, download the data set, etc. If you want to interested users to have a chance of trying your system then you would need to make at least a single data set publicly avaialable. I don't think I can spend more time with this in the next couple of days and even if I could, I don't think my feedback could significantly improve the usefulness of the extension for the community, because too much insider information would be needed even for a first very basic testing (API key, details about input data format, sample data set). However, broad usability of the extension for the community is not a requirement for accepting the extension into the Extensions Index, so I'm OK with getting this integrated. I @pieper and @jamesobutler don't have any more feedback then we can merge this on Monday. |
|
I suggest that @neuropacman give it another look based on the feedback from @lassoan. I don't think anyone benefits if the extension is confusing and hard to even test. |
|
@pieper I agree. I'd like to get it right before it is merged. Sorry for an inconvenience. I have intentionally built this extension under the assumption that users would be aware our product and it's specific input requirements as this is what I was instructed to do. @lassoan I will take your feedback into consideration and reach out again once I am satisfied with everything. Thanks again. |
|
Hi @lassoan @pieper, I have made the following changes:
As for a our highly specific input requirements, this is again something that we assume the users know before attempting to access the extension. We do not foresee this being an issue. Unfortunately, we are not currently able to provide a sample dataset. I hope this is satisfactory. Please let me know if you plan to re-test everything I will provide another API key for you. |
|
@neuropacman you have to admit what you describe has utility only to a small subset of people (those who are assumed to already know the input requirements, or assumed to already have an API key). Since it involves some work on our part to evaluate and host extensions it is kind of against our purposes to have an extension where the only utility is to provide access to your paid service. Does the module provide any other features to the general Slicer user? |
|
@pieper No, the extension does not immediately provide features to a "general Slicer user", as our service requires an API key to operate. Our goal is to provide our users with different mediums of use through extensions/integrations with various PACS and image viewers. However, it is entirely plausible for a "general Slicer user" to discover our extension within Slicer and request an API key to access its functionality. |
|
@pieper I think this falls under the category of an extension that is a portal to commercially licensed functionality.
From the terminologies of Free, Freemium and Premium the above 2 extensions seem to fall under the "Premium" category. The service provided may be niche or generic, but is limited in value on the Slicer Extensions Index if the spirit of the Slicer organization factory build machine which builds and distributes the Slicer Extensions Index (funding provided by Kitware) is to provide extensions under a free (and possibly freemium) model. Maybe @jcfr can provide guidance whether the Slicer Extensions Index can be used to distribute "Premium" type extensions or if the commercial entity should be self-distributing their extension instead (distributing a .ZIP, as mentioned here, that their customers can install into Slicer). FlywheelConnect might have been the first "Premium" Slicer extension, where this case of additional Premium extensions may warrant the need for the Slicer organization to define a stance on the issue. |
|
@jamesobutler it's true that FlyWheel is also a 'Premium' offering, but it's also a somewhat generic tool, something like google drive or a hosted database. In that sense it could help users of either Slicer or FlyWheel to try out the other and use them together if they want. In contrast, Neuropacs seems a little more one-way, which is why I asked what other functionality it might offer. In any case I don't object to the Premium category, and maybe we should go out of our way to encourage companies to promote their novel analysis tools in this way. Slicer could provide a useful platform to compare and contrast different offerings kind of like how you can install dropbox or google drive on your linux machine and decide which you like better. It would be great if Slicer users could have the option of paying for services they find useful. The freemium model would be good, so people can easily use it up to a limit for experiments. |
|
For me an extension is acceptable if it is:
Thresholds for the first two are quite clear. Setting a minimum for usefulness is not easy, but for me usefulness criteria is met if the extension is important for a company, because a company using the platform has inherent value that can compensate the costs. With the introduction of the tiers system we can introduce further incentives to improve usefulness of an extension after it is submitted to the extensions catalog. Let's discuss all these at the weekly meeting tomorrow. |
|
Based on discussion at the weekly developer meeting this extension is accepted into the Extensions Catalog. |
|
@neuropacman This extension was integrated into the https://github.com/Slicer/ExtensionsIndex/tree/5.6 is reserved for extensions for the latest Slicer 5.6.x stable. Something to note is that a new Slicer stable will be coming out shortly and the Note Currently the SlicerNeuroPacs extension is not present in the Slicer Preview build because there are build errors. See https://slicer.cdash.org/index.php?project=SlicerPreview which can be filtered to view the results of your individual extension as well. Reviewing the build results, it appears to be messing up on something related to the extension description. |
|
Good catch @jamesobutler. The issue is that the extension name in the CMakeLists.txt file is incorrect: It should be: |
|
@lassoan Thank you, I have made this correction. |
New extension
3d-slicer-extensionGitHub 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-extensionin "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topicsSettingsand in repository settings uncheckWiki,Projects, andDiscussions(if they are currently not used)Aboutin the top-right corner of the repository main page and uncheckReleasesandPackages(if they are currently not used)My scripted module has a pip dependency "neuropacs". I install neuropacs in the init method if it does not already exist. Please let me know if this needs to be done another way.
To keep track of existing records, I write to a file {slicer.app.temporaryPath}/neuropacs_order_map.json. Just want to make sure this is okay for production environment.