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

Make pillow and numpy optional #27

Closed
dimrozakis opened this issue Feb 18, 2020 · 5 comments
Closed

Make pillow and numpy optional #27

dimrozakis opened this issue Feb 18, 2020 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@dimrozakis
Copy link
Contributor

Looking at setup.py I see that numpy & pillow are required dependencies. After a quick search of the codebase, I see that both of them are only used in the retrieve_instance_frames subparser of cli.py. Furthermore, in the case of numpy, only its (rather simple) fromstring function is used and only in the case where the the retrieved frame is compressed with JPEG LS.

Since these are two significant dependencies that may need to be built, and are only useful for a very specific feature, would you be open to making them optional dependencies (for example using setuptools extras)?

In my case, I'm considering using this project (the library, not the CLI) as part of an application that will be running under pypy instead of cpython, possibly in an alpine container. I'd have to build both pillow & numpy for a feature I don't intend on using.

It's totally understandable if you wish to keep these dependencies as required and I could always just build them or use a slightly modified fork that removes these dependencies.

@hackermd hackermd self-assigned this Feb 18, 2020
@hackermd hackermd added the enhancement New feature or request label Feb 18, 2020
@hackermd
Copy link
Collaborator

Good point. I like the idea of declaring them "extra" dependencies.

@hackermd
Copy link
Collaborator

I have thought about this a bit more and am considering deprecating the functionality for decoding images client side altogether. This is beyond the scope of this library and was probably not a good idea to take this on in the first place. The client could instead retrieve an image rendered from the server using the WADO-RS Retrieve Rendered Transaction.

@hackermd
Copy link
Collaborator

@dimrozakis sorry for the slow turnaround. I have looked into the dependencies and think we could remove all dependencies but requests and pydicom (see e236023).

We would loose the ability to decompress and view images, but I think this should probably be handled outside of this library anyways. With hindsight, I should not have added this functionality to begin with.

What do you think?

I would like to add a few unit tests before releasing this, but would appreciate if you could already test it in your constrained environment.

@dimrozakis
Copy link
Contributor Author

Hi @hackermd

We would loose the ability to decompress and view images, but I think this should probably be handled outside of this library anyways.

I agree. And these features are technically only available in the CLI, any users of the library would have to handle them on their own even with the current implementation. Just as displaying DICOMs is presumably out of scope for this client, it makes sense that displaying JPEGs would be too. These features are also very easy to implement on the client side (of a library user) with just a few lines of code, so no great benefit by including it in. Overall I'd vote to deprecate these features & their dependencies.

I would like to add a few unit tests before releasing this, but would appreciate if you could already test it in your constrained environment.

I'm busy working on some other projects at the moment, but towards the end of the week I should be returning on this subject. I should also note that my usage of this project, as mentioned in the original post, is limited on the library (api.py::DICOMwebClient) and I haven't ever really used cli.py where all of the relevant changes are made in e236023, but I'll be pulling in your commit to try it out.

@hackermd
Copy link
Collaborator

Dependencies have been removed since version 0.30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants