pydicom / deid

best effort anonymization for medical images using python
https://pydicom.github.io/deid/
MIT License
142 stars 44 forks source link

Allow pydicom >=2.1.1 and <3.0 #216

Closed jstorrs closed 2 years ago

jstorrs commented 2 years ago

Description

Bump to latest pydicom release

Checklist

vsoch commented 2 years ago

Should we change "exact_version" to "min_version" instead? :thinking:

jstorrs commented 2 years ago

I don't have strong preference, I only noticed because I wanted to use some of the newer pydicom validation in a different project and deid was holding pydicom back.

vsoch commented 2 years ago

I ask because it's been an issue over the years - a few years back there was a huge change to pydicom so the exact version pin was warranted, but now the changes are incremental and the pin is causing more headaches than it's alleviating. So I would say to try for min_version, and if someone opens a bug with respect to an update we can address it.

jstorrs commented 2 years ago

I edited to allow 2.1.1 or newer but not 3.0 or later. Then I created a dummy pydicom 3.0.0 release by messing with pydicom/_version.py and tested in a few venvs. With these changes:

jstorrs commented 2 years ago

Any thoughts on this one? Should I chop off the <3.0 part?

vsoch commented 2 years ago

I think we would be OK just setting a min_version, at least until we know there is a reason for an upper pin as well.

vsoch commented 2 years ago

For this PR, I would just change exact_version to min_version, and if tests pass we are good. No need for before_version (which would be max_version).

vsoch commented 2 years ago

And once it's tested here and merged, I'll do a new deid release.

jstorrs commented 2 years ago

I only added the between_version because I think the logic in setup.py causes max_version to overwrite min_version so it looked like one or the other. I had been reading some pages in pydicom's docs where there are warnings about some breaking changes planned in 3.0 and how enable warnings to start testing against those changes. That said I have no idea when pydicom 3.0 is anticipated.

vsoch commented 2 years ago

Ah I think you are right, and that wouldn't be what we wanted!

The updates should be fun, whenever it happens! And we can either set a double sided pin (as we almost did here) or can plan for support of 2.x up until some date (and some time when 3.x is well established) and give folks time to update. I'm a fairly aggressive maintainer and I tend to lean on the side of preferring to use more recent versions, so I'd learn towards updating to support the more recent Pydicom and having people use previous versions of deid (with the pin) if they are stuck. Anyway, we can go down that road and have more discussion when it's actually happening.