ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language
https://highdicom.readthedocs.io
MIT License
162 stars 34 forks source link

Add Python 3.12 CI tests #267

Closed DimitriPapadopoulos closed 1 month ago

DimitriPapadopoulos commented 9 months ago

I understand this depends on pillow-jpls, doesn't it?

I have opened issue https://github.com/planetmarshall/pillow-jpls/issues/20.

CPBridge commented 9 months ago

I understand this depends on pillow-jpls, doesn't it?

I have opened issue https://github.com/planetmarshall/pillow-jpls/issues/20.

Yes that's right, thanks for notifying upstream

erikogabrielsson commented 7 months ago

Is there a reason for pillow-jpls not being an extra like pylibjpeg et al?

DimitriPapadopoulos commented 7 months ago

I think this is because some kind of JPEG library is needed. From the Installation guide:

The library relies on the underlying pydicom package for decoding of pixel data, which internally delegates the task to either the pillow or the pylibjpeg packages. Since pillow is a dependency of highdicom and will automatically be installed, some transfer syntax can thus be readily decoded and encoded (baseline JPEG, JPEG-2000, JPEG-LS). Support for additional transfer syntaxes (e.g., lossless JPEG) requires installation of the pylibjpeg package as well as the pylibjpeg-libjpeg and pylibjpeg-openjpeg packages. Since pylibjpeg-libjpeg is licensed under a copyleft GPL v3 license, it is not installed by default when you install highdicom.

erikogabrielsson commented 7 months ago

Sure, but if a user need to read Jpeg-LS it would be easy to install the extra. Pillow typically supports Jpeg and Jpeg 2000 without any extra packages.

CPBridge commented 7 months ago

That's correct. The main reason that pylibjpeg is optional is licence considerations. Since there are no such issues with pillow-jpls, we didn't make it optional. However if things like this holdup with 3.12 occur, it may make sense to make it optional too.

Generally we have been a bit wary of this approach of having a confusing mess of optional dependencies

erikogabrielsson commented 7 months ago

Thanks for the clarification @CPBridge. I agree that optional dependencies can be messy and that you should consider making it optional if it looks like its no longer maintained.

DimitriPapadopoulos commented 7 months ago

So the idea would be to add a try: before import pillow_jpls and emit an error message suggesting to install the module if except ImportError? https://github.com/ImagingDataCommons/highdicom/blob/0f43f8e805bfc9be5256976f8eb984c7e45c3c5e/src/highdicom/frame.py#L260-L261

erikogabrielsson commented 7 months ago

I (too) made a PR to pillow-jpls for Python 3.12 and Pillow 10 support.

Regarding reading mpressed transfer syntaxes other than standard jpeg and jpeg 2000, wsidicom uses imagecodecs for decoding and encoding jpeg 12 bit, jpeg lossless, and jpeg ls.

CPBridge commented 7 months ago

Thanks @erikogabrielsson . I had sort of hoped that pillow-jpls would come back to life and save us from having to work around this, but that is looking increasingly less likely. I will look into whether imagecodecs could be a good alternative.

erikogabrielsson commented 7 months ago

From what I understand, highdicom only uses pillow-jpls to enable encoding? Decoding can only be done if having the libjpeg extra with pylibjpeg-libjpeg. I did a quick test with:

return jpegls_encode(array)

in the jpeg ls statement in frame.py and it passed the tests.

erikogabrielsson commented 6 months ago

Thanks @erikogabrielsson . I had sort of hoped that pillow-jpls would come back to life and save us from having to work around this, but that is looking increasingly less likely. I will look into whether imagecodecs could be a good alternative.

Looks like the issue is now addressed. Hopefulle we will se a build for 3.12 soon.

CPBridge commented 1 month ago

Merging this now that the dependency issue is fixed