dcmqi icon indicating copy to clipboard operation
dcmqi copied to clipboard

Python wrapping

Open fedorov opened this issue 5 years ago • 14 comments

Potentially, it might be interesting to have a python package that wraps the converter interfaces. It is difficult to assess the effort and benefits of such package, since (due to my limited understanding of how things work):

  • potentially, dcmqi would need to be compiled on python packaging platform(s)
  • those python interfaces would still interact with file system for reading/writing input/output objects, as developing in-memory interfaces with pydicom, for example, would be probably difficult
  • not clear what would be the advantages of having such package as opposed to a simple python-only script/package accompanying dcmqi, which would allow to expose interfaces to the dcmqi converter binaries that users could download directly

For the reference, since I spent a little bit of time on this, here are some related pointers:

  • wrapping c++ with cython: https://cython.readthedocs.io/en/latest/src/userguide/wrapping_CPlusPlus.html
  • self-contained example: https://github.com/sturlamolden/cython-cpp-test

If anyone has an opinion on this topic, or is interested to contribute this functionality, please add comments to this issue!

fedorov avatar Mar 22 '19 22:03 fedorov

One option would be to provide an itk remote module. I have been looking into this for other code, but it might also be good for dcmqi.

pieper avatar Mar 22 '19 22:03 pieper

advantages of having such package as opposed to a simple python-only script/package accompanying dcmqi, which would allow to expose interfaces to the dcmqi converter binaries that users could download directly

Probably the ease of installation

has an opinion on this topic

Since the dcmqi CLIs are standalone executables. A first step could be to simply provide the CLIs within a python package like we do for cmake and ninja.

to provide an itk remote module

Doing so would allow to leverage the available ITK build tree when doing CI. Regarding the API, unless that code is refactored to provide ITK interfaces/classes, I am not sure it makes yet.

jcfr avatar Mar 25 '19 13:03 jcfr

Probably the ease of installation ... Since the dcmqi CLIs are standalone executables. A first step could be to simply provide the CLIs within a python package like we do for cmake and ninja.

Interesting, so it's those two:

  • https://pypi.org/project/cmake/
  • https://pypi.org/project/ninja/

But there are already binaries for cmake, for example, for all platforms - what is the real advantage of pip install? I don't quite get the motivation here.

I didn't even realize you have those. Can you tell, how big would be the effort to make something like this for dcmqi, given its organization?

to provide an itk remote module Doing so would allow to leverage the available ITK build tree when doing CI. Regarding the API, unless that code is refactored to provide ITK interfaces/classes, I am not sure it makes yet.

I agree, I had similar concerns.

fedorov avatar Mar 25 '19 16:03 fedorov

@pieper says this could be the cleanest approach: https://cppyy.readthedocs.io/. Related pointers:

  • colab example: https://colab.research.google.com/drive/1DB3XQkuRj5T2OhY95hnvd40R1EzD_lzL?usp=sharing
  • access Slicer mrml and vtk: https://gist.github.com/pieper/f9da3e0a73c70981b48d0747132526d5

fedorov avatar Nov 25 '20 17:11 fedorov

there are already binaries for cmake, for example, for all platforms - what is the real advantage of pip install? I don't quite get the motivation here.

This was an important step to streamline the integration with the python ecosystem in the context of python binary packages.

These are building blocks for scikit-build

Downloading and extracting an archive was tedious for both python developer and CI maintenainers.

jcfr avatar Nov 25 '20 19:11 jcfr

Thinking about this more, I do think the approach used by cmake and ninja makes the most sense for dcmqi. I don't think we would need to expose the full C++ API for most python use cases.

pieper avatar Nov 25 '20 19:11 pieper

We would want a helper script though to expose a pythonic api.

pieper avatar Nov 25 '20 19:11 pieper

@jcfr thanks for the clarification. What would be the best place to start to do something like you did for cmake for dcmqi?

fedorov avatar Nov 25 '20 20:11 fedorov

After speaking with @piiq at the Project Week I will try to take a look at SWIG (or some of the other tools he suggested) to bind the CPP DCQMI implementation to python - and then possibly package everything.

I'll report on the progress as soon as I have something. I have never tried this, but it could be fun. Worst case we could start from a dumb python wrapper using subprocess (and pulling the right binaries upon installation - after packaging).

*edit: just saw @piiq already forked DCMQI and is probably working on this. I'd be glad to tag along, so let's see how this goes.

denbonte avatar Feb 03 '23 16:02 denbonte

There are several mature packages for wrapping C++ code for use in python and it would be good to survey them for use with dcmqi. In my experience cppyy is worth exploring as a modern take on the problem. Here is a colab notebook with some examples.

pieper avatar Feb 03 '23 18:02 pieper

Yes, I was also mentioning cppyy to @denbonte

piiq avatar Feb 04 '23 10:02 piiq

Overall, I am worried that python wrapping will become a rabbit hole, and also I do not know how much expertise would be needed to deal with compilers and CI of those wrapping tools on various platforms. But if someone wants to contribute this functionality - I am definitely very much for it (I would definitely want to fix mac OS CI first though to at least make sure it does not cause issues on mac).

Worst case we could start from a dumb python wrapper using subprocess (and pulling the right binaries upon installation - after packaging).

This is where I would start as well!

fedorov avatar Feb 05 '23 16:02 fedorov

We decided to proceed with the following plan towards simplified access to dcmqi from python, which @LennyN95 will be working on:

  1. establish dcmqi-python-distributions python package that will allow installation of the platform-specific dcmqi binaries via pip install dcmqi. This should follow the approach used in https://github.com/ImagingDataCommons/s5cmd-python-distributions for wrapping s5cmd. Comments in the commit history help understand the process, which started from the template mentioned in the first commit: https://github.com/ImagingDataCommons/s5cmd-python-distributions/commit/33e5e7441da7799d3a0be03ce284dd5e6e0037b1.
  2. once the above is done, establish a separate pydcmqi package that will depend on 1 and will provide API to simplify interaction launching conversion binaries and accessing metadata in JSON.

fedorov avatar Mar 06 '24 14:03 fedorov

establish dcmqi-python-distributions python package

It would go into this repo: https://github.com/ImagingDataCommons/dcmqi-python-distributions

fedorov avatar Apr 09 '24 15:04 fedorov