ENH: expose module load/unload API

[pieper]

Exposing these methods allows programmatic control over
module loading and unloading, which can be useful for giving
users access to experimental modules, like with the following
script:

import os
import shutil

archiveFilePath = os.path.join(slicer.app.temporaryPath, "master.zip")
outputDir = os.path.join(slicer.app.temporaryPath, "SlicerAnimator")

try:
    os.remove(archiveFilePath)
except FileNotFoundError:
    pass

try:
    shutil.rmtree(outputDir)
except FileNotFoundError:
    pass

os.mkdir(outputDir)

slicer.util.downloadAndExtractArchive(
    url = "https://github.com/pieper/SlicerAnimator/archive/master.zip",
    archiveFilePath = archiveFilePath,
    outputDir = outputDir)

modulePath = os.path.join(outputDir, "SlicerAnimator-master", "Animator", "Animator.py")
factoryManager = slicer.app.moduleManager().factoryManager()
factoryManager.registerModule(qt.QFileInfo(modulePath))

factoryManager.instantiateModule("Animator")
factoryManager.loadModule("Animator")

slicer.util.selectModule("Animator")
slicer.util.delayDisplay("Waiting")

factoryManager.unloadModule("Animator")

[pieper]

@jcfr - I didn't find any issues that would make me think these methods need to be protected. Do you agree? No odd behavior or crashes I could see.

The motivation for this is being able to easily share "Lab" modules directly out of github repositories. This is for SlicerMorph, where Murat wants Sara to be able to iterate quickly with users. Once this works I plan to write a LabManager module that allows users to easily install and update scripted modules on the fly.

[lassoan]

Note that this is already implemented in extension wizard:

https://github.com/Slicer/Slicer/blob/master/Modules/Scripted/ExtensionWizard/ExtensionWizard.py#L320-L391

Loading individual modules manually is not ideal:

• you would need to resolve dependencies between modules manually to load them in the correct order
• normally you can only load all modules from a folder; it is not nice to temporarily change it

So, I would keep using the method implemented in ExtensionWizard, but probably move it to slicer.util so that it can be found more easily.

What I really miss is that I cannot drag-and-drop a module folder or module .py file to the main window to load it/add it to additional module paths (I have to open the Application settings dialog and choose the Modules section). It would be just a few ten lines of code to add a qSlicerFileDialog (similar to DICOMFileDialog) that would offer to add the folder to the additional module paths / load the modules instantly - if the user drag-and-drops .py files or folder containing .py files to the main window. It could be even smarter and detect if a .py file is a Slicer module or a script and offer to run the script instead of loading it as a module.

[pieper]

Yes, I looked at the ExtensionWizard for ideas, but it's mixed with the gui there and not reusable. Moving the logic of it to slicer.util would make sense.

The drag and drop idea is fine too, but also not related to this PR specifically (except I don't think you could implement it cleanly without these changes to the API). I can implement dynamic scripted module loading from a repository with the existing API without and satisfy my use case, but without unloading it's a little constrained.

The point of this PR is that we already have a public method for loadModules(QStringList), so adding loadModule(QString) just cleans up the API. We can use loadModule(name) rather than loadModules([name,]).

We also have unloadModules() which unloads all modules completely (really only useful at shutdown). unloadModule(QString) just lets you be specific and get rid of one module.

There are always ways developers can misuse any of these APIs, but this PR enables very useful functionality and doesn't make things more dangerous. If you have dependencies, they still need to be managed and that may not fit this use case.

@lassoan I don't understand your point "normally you can only load all modules from a folder; it is not nice to temporarily change it" - can you clarify?

[lassoan]

Yes, I looked at the ExtensionWizard for ideas, but it's mixed with the gui there and not reusable. Moving the logic of it to slicer.util would make sense.

Fully agree. It should be no problem to move that function over to slicer.util.

The drag and drop idea is fine too, but also not related to this PR specifically

I agree, it just came to my mind, as I always miss this but not that much so that I would take the time, but the work you do now is very close to this, so it could make sense to implement in one step.

The point of this PR is that we already have a public method for loadModules(QStringList), so adding loadModule(QString) just cleans up the API.

I think the methods are protected to discourage loading modules one by one (because developers may forget about dependency between modules) and to simplify the API (we have to worry about breaking backward compatibility if there are less public methods). You can just call LoadModules with a single module in the list if you want to load a specific module.

unloadModule(QString) just lets you be specific and get rid of one module

Unloading a module is tough. In general, it does not work, as modules do not have API to release dependencies and reconnect to them; and it may also be problematic due to circular dependencies between modules. "Reload" button in scripted modules widget header often does not work correctly which is OK for developers (they can click "Restart" next to it when things get really out of control). But I would not recommend users to reload modules without restarting Slicer (loading a module dynamically is fine, just not unloading).

@lassoan I don't understand your point "normally you can only load all modules from a folder; it is not nice to temporarily change it" - can you clarify?

"Loading individual .py module files" vs. "adding a folder to additional module paths and loading all .py module files in it" gives slightly different results (e.g., due to adding of folder to Python paths, potential presence of extra .py files in the folder, module dependency resolution, etc.). Module developers might not fully understand these subtle differences and why their module works in one way and not the other.

I would recommend to restrict the API usage to:

• only allow adding an entire folder (you can still list modules to ignore), not individual .py files
• always restart Slicer when unloading modules

These APIs are already publicly exposed and available in Python.

[jcfr]

I would keep using the method implemented in ExtensionWizard, but probably move it to slicer.util so that it can be found more easily.

👍

[pieper]

Thanks for your comments @lassoan but I will take issue with your conclusions.

Restarting after unloading a module may be required, but I don't see any reason to assume that as a general rule. Certainly for most scripted modules that would not be the case. Restarting Slicer is a major waste of time for many use cases. For example if you want to try various versions of a scripted module I think you should be allowed to swap them in and out without restarting slicer and reloading your data.

I suppose a workaround is to give each version a unique module name and never unload. That sounds like a hacky workaround.

Another reason I don't like protected methods like unloadModule is that they introduce an asymmetry in the API. It basically says if you subclass this in C++ then you can do what you want, but you are not able to it from python. That's just artificially limiting the utility of the application.

Looking at the implementation here I don't see anything likely to break in this class if we allow unloadModule to be called, but that's why I asked for @jcfr 's review.

Regarding your comment about loading directories vs individual modules, the registerModule method already works that way and this doesn't change anything.

[lassoan]

LoadModule left as public by accident (or as a temporary workaround) see comments in the source files for details.

I'm perfectly fine to expose features that don't always work to developers. But this unload feature would be used by users, who would just have the impression that Slicer is unstable.

By the way, how this unload/load differs from reload? It would be nice if we would only need to maintain one mechanism for live module updates.

[lassoan]

Just to clarify, I'm fine if this change is merged as is. It is just an experimental feature anyway. Just brought up the points above to give a chance to find simpler and more robust solution.