ENH: add WebServer module #5999
Conversation
Code imported from Steve Pieper's developments. https://github.com/pieper/SlicerWeb
|
Thanks for the contribution. To give the code a chance to mature and be more broadly tested. What do you think of:
Later, we could then add the module to the core as a Slicer remote like we do for the Surface Toolbox (see |
|
@jcfr I went with this option based on a discussion with @lassoan on the Slicer discourse. We felt there was a motivating use case to have this feature in the core and little downside for anyone who's not using the module. Regarding where to keep the code, my experience has been that having many repositories for core modules leads to extra burden for maintainers since we need multiple PRs to fix common issues. |
|
I've marked this as draft for now so we can discuss. There are some TODO items in the code and we should think about which of those are important enough to put in place before merging this. The most important in my mind is putting an explicit start/stop button in the module (currently it starts on |
There was a problem hiding this comment.
Thank you, this is great! I added some comments and suggestions inline.
I think it is good to have it in the Slicer core, as the amount of code is quite small and we don't get much benefit from other half-integrated extensions either (LandmarkRegistration, SurfaceToolbox).
|
|
||
| This code has been developed over a number of years in a separate repository where there are additional experiments demontrating other potential uses. The version in Slicer core has been stripped down to address the most common expected use cases. See [https://github.com/pieper/SlicerWeb](https://github.com/pieper/SlicerWeb). | ||
|
|
||
| Also note that the code should be considered somewhat experimental and a likely security risk. Do not expose web server endpoints on the public internet without careful consideration. |
There was a problem hiding this comment.
Make it clear that it is only a security risk if the endpoints are enabled (and they are disabled by default).
Docs/user_guide/modules/webserver.md
Outdated
| Creates a fairly simple but pretty powerful web server that can respond to http(s) requests with data from the current Slicer or modify the Slicer state. This module is meant to be the basis for implemeting web applications that use Slicer as a remote render / computation engine or for controlling your Slicer instance for interaction with other system code like shell scripts or other applications. | ||
|
|
||
| There are three basic types of endpoints: | ||
| - **Static:** Hosts files out of the module's `docroot` like any standard http server. Currently this is used just for examples. |
There was a problem hiding this comment.
We should separate potentially clinically useful examples from from tests and pure technology demonstrations. Clinically useful stuff should be at the top and the rest (timeimage, timecube, spinning cube) should be in a separate section, but preferably in a separate page (can be linked from the main page).
Docs/user_guide/modules/webserver.md
Outdated
|
|
||
| ## Current Features | ||
|
|
||
| The server launches on port 2016 by default when you enter the module. Access logs are shown in the GUI by default and can be routed to the console if persistence is needed (the GUI logs are cleared periodically). If port 2016is in use, other ports are checked sequentially until an open port is found, allowing more than one Slicer web server to run concurrently. |
There was a problem hiding this comment.
| The server launches on port 2016 by default when you enter the module. Access logs are shown in the GUI by default and can be routed to the console if persistence is needed (the GUI logs are cleared periodically). If port 2016is in use, other ports are checked sequentially until an open port is found, allowing more than one Slicer web server to run concurrently. | |
| The server launches on port 2016 by default when you enter the module. Access logs are shown in the GUI by default and can be routed to the console if persistence is needed (the GUI logs are cleared periodically). If port 2016 is in use, other ports are checked sequentially until an open port is found, allowing more than one Slicer web server to run concurrently. |
Docs/user_guide/modules/webserver.md
Outdated
| For example, these command can be used to download data and change the screen layout: | ||
|
|
||
| ``` | ||
| curl -X POST localhost:2016/slicer/repl --data "import SampleData; volumeNode = SampleData.SampleDataLogic().downloadMRHead(); slicer.app.layoutManager().setLayout(slicer.vtkMRMLLayoutNode.SlicerLayoutOneUpRedSliceView)" |
There was a problem hiding this comment.
repl is a bit too abstract (and it is not really a loop but we really just perform a single command). What are some other endpoint names that are commonly used for this kind of functionality?
Some examples that look more readable to me:
- localhost:2016/slicer/execute
- localhost:2016/slicer/exec
- localhost:2016/slicer/cmd
There was a problem hiding this comment.
Good point. Internally it uses python exec so that would be the best choice in terms of clarity and documenting the feature. I'll add it and deprecate repl.
Docs/user_guide/modules/webserver.md
Outdated
|
|
||
| ## Related modules | ||
|
|
||
| - The [OpenIGTLink](https://github.com/openigtlink/SlicerOpenIGTLink) Extension has some similar functionality cusomized for image guided therapy applications. It should be preferred for integration with imaging devices and use in a clinical setting. |
There was a problem hiding this comment.
| - The [OpenIGTLink](https://github.com/openigtlink/SlicerOpenIGTLink) Extension has some similar functionality cusomized for image guided therapy applications. It should be preferred for integration with imaging devices and use in a clinical setting. | |
| - The [OpenIGTLink](https://github.com/openigtlink/SlicerOpenIGTLink) Extension has some similar functionality customized for image guided therapy applications. It should be preferred for integration with imaging devices and use in a clinical setting or setting up continuous high-throughput image and transform streams. |
|
|
||
| # python imports | ||
| try: | ||
| from BaseHTTPServer import HTTPServer |
There was a problem hiding this comment.
Is this Python 2/3 compatibility? We can drop Python2.
| import string | ||
| import time | ||
| try: | ||
| import urlparse |
| # SlicerRequestHandler | ||
| # | ||
|
|
||
| class SlicerRequestHandler(object): |
There was a problem hiding this comment.
Could we move out each request handler to a separate file?
It would make this file shorter and simpler and it could server as example for extensions defining and registering their own custom request handlers.
|
Some more findings:
|
|
I've tried this WebServer module to open a file in a running Slicer instance by running a .bat file and it works very nicely using this command ( The only thing that is not nice that there is a few-second delay before Slicer starts to process the command. It might be an issue with the QSocketNotifier. We tried to use it in SlicerJupyter but it had issues (100% CPU use for sure, but maybe performance problems, too). First we had to switch to simple polling in Windows, but then problems were found on Linux, too; so we ended up using polling on all platforms. |
|
Thanks for the review - good feedback 👍 I'll do some more tweaks and test on windows. Good topic to discuss in tomorrow's call. |
|
Few high level comments There are few aspects:
This is why I suggest to split
|
I will not be available tomorrow. That said, I will look forward the meeting notes. |
| Resources/docroot/index.html | ||
| Resources/docroot/favicon.ico | ||
| Resources/docroot/images/3DSlicer-DesktopIcon.png | ||
| Resources/docroot/ServerTests/glMatrix-0.9.5.min.js |
There was a problem hiding this comment.
I suggest to update the example to directly download the script instead of packaging it.
|
I agree that the module should be split up to a module that has the infrastructure to register/unregister handlers (plugins). I suggested the same in my comments - have handlers in separate files; and have API that allows other modules to register their handlers, too. I like the WebServer module name because it describes the scope of the module nicely and does not limit itself to a particular style of web API (although right now all handlers use REST API). Distributing components via npm is a great idea (once we have some usable components and not just technology demos). |
| self.port = SlicerHTTPServer.findFreePort(self.port) | ||
| self.logMessage("Starting server on port %d" % self.port) | ||
| self.logMessage('docroot: %s' % self.docroot) | ||
| # example: certfile = '/Users/pieper/slicer/latest/SlicerWeb/localhost.pem' |
There was a problem hiding this comment.
Hi Steve. Here is some feedback:
The default pem file path and an "autostart" flag (telling if the WebServer should start automatically at application launch) could be read from application settings. Or autostart could be turned off by default and you could launch Slicer with specifying a small script that starts the WebServer with the parameters you like (--python-code "import WebServer; server=WebServer.... ; server.start(...)“.
|
It might be worth looking at FastAPI, which provides a really easy way to create an API from python code, and includes a high performance web server. Self-documenting and uses typing as well. |
This could be good. The problem I found with pretty much all python web servers is that they want to "own" the event loop where here event loop is owned by the Qt application. @haehn is trying to recruit some students to help with this as part of another project so we can hope to get some more experiments going soon. |
It looks like there are ways to integrate the Qt event loop with asyncio, which in turn runs FastAPI through another layer (uvicorn or similar). On the other hand, I don't know if async event would be what you want. |
- Moved out request handlers to separate files. - Allow registration of custom request handlers. - Iterate through registered request handlers and let that one process the request that returned the highest confidence. - Save default handler and logging options in application settings. - Start/stop the server when a start/stop button is pressed (not start when opening the WebServer module GUI). - Updated Slicer icon. - Updated module icon (API icon from Google Material Icon Set) - Added `sampleData` command to allow easy testing and demoing of the API - Added `gui` command to switch between full application/viewers only, switch viewer layout - Added `screenshot` command to get screenshot of the application main window
|
I've fixed most of the review comments and TODOs. I haven't tested the DICOM handler. |
* add entry on demo page to reset view to four up with controls * change "got it" messages to more meaningful feedback * add extra tooltip hints on usage * fix logic of local connection button
This puts the stack trace in the log and prevents any bad request handlers from leaving the network connection dangling.
A newer version of pydicom is required to fix a bug in json serialization of a pydicom multivalue class. Also fix a warning from bad uri syntax.
* Implements study level query and metadata. * Adds offset and limits to study listing * Adds documentation on using with OHIF
|
@pieper Is there anything else you want to add/change in this module? If not, then I think we should rebase and merge it, as remote control of the application is quite important to be able to better compete with other viewers that start up faster and/or are more integrated into the Python environment. I'm thinking about adding an ITKSphinxExample for using Slicer as a viewer at the upcoming May 20 ITK event and this feature would be a prerequisite. It may take a few days to get all the issues ironed out on all platforms, so there is not too much time left. |
|
@lassoan yes, I am at a good spot with it. The only outstanding conflice is that I needed to upgrade pydicom. I don't think that will be an issue so let's merge it and we if anything pops up we can deal with it independently. |
|
The |
|
Thanks for the heads up @jamesobutler - looks like some simple format issues. It would be nice if the lint just created a PR automatically. I guess I can fix these up in the browser (I'm away from my regular setup). |
This should be possible - I am taking note of this and will evaluate our options. |
Cherry picked from: * bdc1ee0 - ENH: add WebServer module (Slicer#5999) * d00bc5b - STYLE: Fix python lint issues in WebServer module * fdcdbe5 - DOC: Improve Webserver module documentation * fba5df4 - DOC: Improve Webserver module documentation (part 2) * 9c7f205 - DOC: extra details and warnings about WebServer (Slicer#6370) * c9a95b1 - ENH: Add DICOMweb example to webserver examples page * 0f29c7c - BUG: Fix WebServer failing to load when Slicer built without DICOM support (Slicer#6407)
Cherry picked from: * bdc1ee0 - ENH: add WebServer module (Slicer#5999) * d00bc5b - STYLE: Fix python lint issues in WebServer module * fdcdbe5 - DOC: Improve Webserver module documentation * fba5df4 - DOC: Improve Webserver module documentation (part 2) * 9c7f205 - DOC: extra details and warnings about WebServer (Slicer#6370) * c9a95b1 - ENH: Add DICOMweb example to webserver examples page * 0f29c7c - BUG: Fix WebServer failing to load when Slicer built without DICOM support (Slicer#6407)
Code imported from Steve Pieper's development repository.
https://github.com/pieper/SlicerWeb