Performance problems with custom REST callback that uses studies/{id}/media (zip archive)

[a.sallai]

We are trying to use a custom REST callback for the studies/{id}/media endpoint to extend the resulting zip file with a dicom viewer (based on this post by sdscotti). We are trying to use python subprocesses in order to not block the GIL while loading large study data (sizes close to a Gb).

We are doing something very similar to this example in the python plugin docs:

import json
import multiprocessing
import orthanc
import requests
import signal

TOKEN = orthanc.GenerateRestApiAuthorizationToken()

def SlaveProcess():
    r = requests.get('http://localhost:8042/instances',
                     headers = { 'Authorization' : TOKEN })
    return json.dumps(r.json())

def Initializer():
    signal.signal(signal.SIGINT, signal.SIG_IGN)

POOL = multiprocessing.Pool(4, initializer = Initializer)

def OnRest(output, uri, **request):
    answer = POOL.apply(SlaveProcess)
    output.AnswerBuffer(answer, 'text/plain')

orthanc.RegisterRestCallback('/computation', OnRest)

Our problem is that the output.AnswerBuffer call must run in the “master” python process according to the docs.

This means that there is no way to send answers to multiple clients in parallel,
so if one client requests a ~1Gb study then even though its processing can run in a subprocess, the part that sends it back to the client (output.AnswerBuffer) must run in the “master” process and all other clients will have to wait quite a while because of the GIL.

Is there a workaround for this?

[benjamin.golinvaux]

Hello

I don’t think you will block the GIL when retrieving large studies because it’s released during blocking IO transfers

Have you tried the same with a regular call to the orthanc module directly in the Python plugin? The advantage being that you’re using a direct route to the REST API that bypasses the local loopback.

Another option, in your case, would be to add a very simple http server running on the side, for instance with FastAPI, and put a reverse proxy in front of both Orthanc and your server. Your server could then download the study and serve the modified zip archive.

[a.sallai]

Originally, we tried it just the way you suggest: use a single python process and use functions provided by the orthanc module to fetch the study, like this:

study_data = io.BytesIO(orthanc.RestApiGet(uri))

where uri is /studies/d4e5dd11-3f327529-c939bfae-5eeaa188-f30d027d/media for example.

This definitely does block the GIL. We use python callbacks for all HTTP endpoint accesses (custom auth) and when we had the above in our code, all UI and REST API access was blocked while the orthanc.RestApiGet() call was executing.

Here is a dumbed down, but complete and working version to reproduce it:

import orthanc
import io

def Filter(uri, **request):
    print('dummy filter, URI: %s' % uri)
    return True

def OnMedia(output, uri, **request):
    study_data = io.BytesIO(orthanc.RestApiGet(uri))

    output.SetHttpHeader("Content-Disposition", f'attachment; filename="archive.zip"')
    output.AnswerBuffer(study_data.getvalue(), "application/zip")


orthanc.RegisterRestCallback(f"/studies/(.*)/media", OnMedia)

orthanc.RegisterIncomingHttpRequestFilter(Filter)

If we issue a call to /studies/{id}/media, python execution is blocked until the zip archive is received.

Our next iteration was to call the Orthanc REST API in a subprocess (now we can’t use the orthanc module but using the http module still works) but then we saw that output.AnswerBuffer() also seems to be blocking the GIL.

Our only option seems to be what you also suggested: do the zip processing outside of Orthanc.

[benjamin.golinvaux]

Hi,

Can you maybe try to replace your call to orthanc.RestApiGet(uri) to a call to requests.get on localhost?

Since requests definitely unlocks the GIL during IO (actually, Python releases the GIL during IO) , that should allow you to check if the problem really comes from the GIL being held when calling the orthanc module functions.

I do not know how the Python plugin internally works, but if you notice an improvement there, maybe it’d be worth discussing whether GIL unlocks can be put around internal functions to the Orthanc sdk (same goes for AnswerBuffer where, unfortunately, you cannot really emulate GIL unlocks)

Does this make sense?

[a.sallai]

Thanks, Benjamin, I appreciate your suggestions.

I tried using the requests library instead of orthanc.RestApiGet but Orthanc wouldn’t start with the following error:

E0621 12:05:59.713996             MAIN PluginsManager.cpp:153] Error during the installation of the Python script, traceback:
<class 'ModuleNotFoundError'>
No module named 'requests'

  File "/python-scripts/main.py", line 3, in <module>
    import requests

Why could this be? Not really relevant to the main question but I’d like to know.

Anyways, I used the http module, for our purposes it should be the same I think.
You were right, execution doesn’t seem to get blocked in this case.

So I’m wondering how orthanc.RestApiGet and AnswerBuffer work. Why is the GIL not unlocked when these functions do I/O?

This is some progress, nontheless. Now it looks like I can solve the fetching of the study with some non-orthanc-module HTTP call that doesn’t block. I still have the AnswerBuffer call however that blocks and that would cause problems for clients trying to download studies concurrently (this is a large Orthanc instance with many possible clients).

[benjamin.golinvaux]

Hello

Regarding your ModuleNotFoundError , please note that requests is not a built-in module and the package needs to be installed separately (python -m pip install requests)

Regarding AnswerBuffer and held GIL, I need to discuss this with the core devs and I might be wrong, but it looks like a Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS block around the internal Orthanc call might help.

How familiar are you with building the Orthanc plugin? What OS / distro / Python version are you using the plugin with?

[benjamin.golinvaux]

blanket-GIL-release.diff.txt (624 Bytes)

Hello,

This is a very crude patch that should allow to check how AnswerBuffer behaves when releasing the GIL during Orthanc I/O.

This is not tested and could introduce data integrity issues among the various threads, although I don’t think it will.

Very short guide, assuming patch file copied in ~:

# make work dir
mkdir ~/orpy-sandbox
cd ~/orpy-sandbox

# clone orthanc-python
hg clone https://hg.orthanc-server.com/orthanc-python

# clone orthanc
hg clone https://hg.orthanc-server.com/orthanc

# apply patch
cd orthanc-python
hg import --no-commit ~/blanket-GIL-release.diff.txt

mkdir -p ~/orpy-sandbox/bin/build
mkdir -p ~/orpy-sandbox/bin/install

cd ~/orpy-sandbox/bin/build
cmake \
   -DPYTHON_VERSION="3.10" \
   -DCMAKE_BUILD_TYPE=Debug \
   -DCMAKE_INSTALL_PREFIX="~/orpy-sandbox/bin/install" \
   -DSTATIC_BUILD=ON \
   -DORTHANC_FRAMEWORK_SOURCE=path \
   -DORTHANC_FRAMEWORK_ROOT="/home/$USER/orpy-sandbox/orthanc/OrthancFramework/Sources" \
   -DALLOW_DOWNLOADS=ON \
   ../../orthanc-python

make -j20 install

Once it’s done, you can temporarily rename the regular libOrthancPython.so file and copy the one in /home/bgo/orpy-sandbox/bin/install/share/orthanc/plugins/libOrthancPython.so file at a location where Orthanc can find it.

If this helps, we might want to check if it’s possible to carefully review the impact and where the GIL can actually be released (this must be done with the utmost care)

Please make sure to change the cmake invocation to pick the correct Python version.

The procedure is very similar if you are running Windows, provided you stick to the static build.

HTH and let me know how it goes!

[jodogne]

Hello,

I would recommend caution with this patch:

+  Py_BEGIN_ALLOW_THREADS
   OrthancPluginAnswerBuffer(OrthancPlugins::GetGlobalContext(), self->object_, arg0.buf, arg0.len, arg2);
+  Py_END_ALLOW_THREADS

Indeed, the arg0 and arg2 arguments might change if other threads access the same Python object. The proper solution would probably consist in making a copy of the content of the Python objects in the C++ code before calling Py_BEGIN_ALLOW_THREADS, which would come at an additional cost.

Regards,
Sébastien-

[jodogne]

Python plugin 4.3

Dear all, I am pleased to announce the release of version 4.3 of the Python plugin for Orthanc. This new version brings three important changes: A Python interface is now available to provide code completion and documentation about the orthanc module in your IDE. The related file is known as orthanc.pyi and is known to work with both Visual Studio Code and PyCharm. A new configuration option named AllowThreads has been introduced to allow multiple Python threads to simultaneously access the O…

[benjamin.golinvaux]

Hi András,

Please let us know if the new configuration option that Sébastien has added solves the contention issues that you were experiencing. I am very interested in your results.

HTH

[a.sallai]

Hi Benjamin,

I did a couple tests and can confirm that setting the new Python.AllowThreads configuration option to true has an unlocking effect on the output.AnswerBuffer calls, meaning that our HTTP threads (each running auth-related python code using orthanc.RegisterIncomingHttpRequestFilter) no longer seem to be blocked even while output.AnswerBuffer is executing.

The same seems to be true for orthanc.RestApiGet, that doesn’t block either with Python.AllowThreads set to true.

For completeness’ sake, here is the piece of code we originally created for extending our DICOMDIR downloads with the microdicom viewer:

def OnMedia(output, uri, **request):
    viewer_path = "/microdicom-viewer"
    study_id = uri.split("/")[2]
    study_data = io.BytesIO(orthanc.RestApiGet(uri))

    with zipfile.ZipFile(study_data, "a") as study_zip:
        for root, _, files in os.walk(viewer_path):
            prefix = pathlib.Path(root)
            for viewer_file in files:
                name = str(prefix / viewer_file)
                study_zip.write(name, arcname=name.replace(viewer_path, ""),
                                  compress_type=zipfile.ZIP_DEFLATED)
        study_zip.write("/microdicom-license/license.lic", "Win32/license.lic")
        study_zip.write("/microdicom-license/license.lic", "x64/license.lic")

    output.SetHttpHeader("Content-Disposition", f'attachment; filename="{study_id}.zip"')
    output.AnswerBuffer(study_data.getvalue(), "application/zip")

if os.path.isfile("/microdicom-license/license.lic"):
    orthanc.LogInfo("Micordicom license was found under /microdicom-license/license.lic, registering extension for /studies/(.*)/media...")
    orthanc.RegisterRestCallback(f"/studies/(.*)/media", OnMedia)

There was also a more complex implementation that used multiple python processes but after a while we realized that output.AnswerBuffer really must be running in the “master process” (as the docs state) so that was still a bottleneck.

We identified 4 major problems with our implementation:

• (solved) orthanc.RestApiGet blocks, but we can use the http module and call /media ourselves (need to add a new URL path instead of the original /media endpoint to avoid recursion)
• (solved) output.AnswerBuffer blocks and it must run in the “master process” according to the docs. This means that even with multiple python subprocesses, clients will block each other
  while any one of the is running output.AnswerBuffer
• connection timeouts - clients don’t get a response for a very long time, there is no way to send content-length (or any) HTTP header upfront from python code
• does in-memory zipping - will quickly result in oom killer with just a few concurrent downloads (Gb size studies)

This new Python.AllowThreads seems to be solving the first 2 of those problems. The warnings about thread-safety in the docs are a bit alarming thought, I have little knowledge of multi-threaded programs. Even if that isn’t an issue for us, the last 2 of our problems still remain. As a result we decided to implement the custom zipping in an outside service.

Nonetheless, thanks a lot for your input, this thread might serve others well in the future.

[benjamin.golinvaux]

Hi András,

Thank you very much for taking the time to provide this very useful report.

I have to agree with your conclusion. As you can see, the Python plugin is not, at the moment, designed for ultimate control over the new routes that it exposes. Solving these niche use cases would probably eat a lot of development time, for the more control you provide, the more tests, fixes and docs need to be written.

Lifting the GIL (contention) in the Python plugin has been done thanks to your input, and I believe it can already unlock several interesting use cases.

A compound solution where a plugin and an external service collaborate, for instance (with a nginx reverse proxy on top, to provide a unified facade), could be a nice way of lifting those restrictions arising from the lack of streaming support in the http API.

On a final note regarding the warning that Sébastien has added in the docs: this is meant for programmers who might not be aware of the issues that may arise when using several threads and that would perhaps want to modify or free the underlying buffer that is currently serving the network transfer. As long as the data (study_data.getvalue() in your example, for instance) is left untouched during the transfer, no issue will arise. This remark is purely out of an abundance of caution for people who might, for instance, use global variables shared between http handlers (which is a bad pattern in the first place)

I am sorry that your experiment fell short of your expectations, but your input was very valuable. Thanks for this fruitful interaction !

[benjamin.golinvaux]

I forgot to mention that the multipart-aware plugin API functions could perhaps be leveraged for the streaming uses cases that you have in mind. I do not know if they are exposed in the Python plugin for now (they are in the C++ SDK upon which the Python plugin is built) and I do not know whether it could solve your particular use case, but it could be an avenue of research, provided the consuming client can accept it.

Please grep for “multipart” in the following doc: Orthanc Plugin SDK: REST (uclouvain.be) if you want to know more about it.

Cheers