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

Add Python 3.12 CI tests #267

Merged
merged 1 commit into from
May 27, 2024

Conversation

DimitriPapadopoulos
Copy link
Contributor

No description provided.

CPBridge
CPBridge previously approved these changes Oct 11, 2023
Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to wait until jpegls is working on 3.12 before we can add this I think

@CPBridge CPBridge dismissed their stale review October 11, 2023 20:58

Waiting until dependencies work with 3.12

@DimitriPapadopoulos
Copy link
Contributor Author

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

I have opened issue planetmarshall/pillow-jpls#20.

@CPBridge
Copy link
Collaborator

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

I have opened issue planetmarshall/pillow-jpls#20.

Yes that's right, thanks for notifying upstream

@erikogabrielsson
Copy link
Contributor

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

@DimitriPapadopoulos
Copy link
Contributor Author

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

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

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

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

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?

elif transfer_syntax_uid == JPEGLSLossless:
import pillow_jpls # noqa

@erikogabrielsson
Copy link
Contributor

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

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

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

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

Merging this now that the dependency issue is fixed

@CPBridge CPBridge merged commit 46d3c81 into ImagingDataCommons:master May 27, 2024
1 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants