Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python wrapping #377

Open
fedorov opened this issue Mar 22, 2019 · 14 comments
Open

Python wrapping #377

fedorov opened this issue Mar 22, 2019 · 14 comments
Assignees

Comments

@fedorov
Copy link
Member

fedorov commented Mar 22, 2019

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:

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

@pieper
Copy link
Member

pieper commented Mar 22, 2019

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.

@jcfr
Copy link
Contributor

jcfr commented Mar 25, 2019

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.

@fedorov
Copy link
Member Author

fedorov commented Mar 25, 2019

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.

@fedorov
Copy link
Member Author

fedorov commented Nov 25, 2020

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

@jcfr
Copy link
Contributor

jcfr commented Nov 25, 2020

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.

@pieper
Copy link
Member

pieper commented Nov 25, 2020

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
Copy link
Member

pieper commented Nov 25, 2020

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

@fedorov
Copy link
Member Author

fedorov commented Nov 25, 2020

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

@denbonte
Copy link

denbonte commented Feb 3, 2023

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.

@pieper
Copy link
Member

pieper commented Feb 3, 2023

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.

@piiq
Copy link

piiq commented Feb 4, 2023

Yes, I was also mentioning cppyy to @denbonte

@fedorov
Copy link
Member Author

fedorov commented Feb 5, 2023

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
Copy link
Member Author

fedorov commented Mar 6, 2024

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: ImagingDataCommons/s5cmd-python-distributions@33e5e74.
  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
Copy link
Member Author

fedorov commented Apr 9, 2024

establish dcmqi-python-distributions python package

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants