Open fedorov opened 5 years ago
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.
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.
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:
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.
@pieper says this could be the cleanest approach: https://cppyy.readthedocs.io/. Related pointers:
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.
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.
We would want a helper script though to expose a pythonic api.
@jcfr thanks for the clarification. What would be the best place to start to do something like you did for cmake for dcmqi?
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.
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.
Yes, I was also mentioning cppyy to @denbonte
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!
We decided to proceed with the following plan towards simplified access to dcmqi from python, which @LennyN95 will be working on:
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.pydcmqi
package that will depend on 1 and will provide API to simplify interaction launching conversion binaries and accessing metadata in JSON.establish
dcmqi-python-distributions
python package
It would go into this repo: https://github.com/ImagingDataCommons/dcmqi-python-distributions
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):
For the reference, since I spent a little bit of time on this, here are some related pointers:
If anyone has an opinion on this topic, or is interested to contribute this functionality, please add comments to this issue!