pydicom / deid

best effort anonymization for medical images using python

https://pydicom.github.io/deid/

MIT License

140 stars 43 forks source link

updated pydicom dependency from 2.1.1 to 2.2.2 #194

Closed glebsts closed 2 years ago

glebsts commented 2 years ago

193 'bump pydicom' - bumped to 2.2.2, local tests green, added PyCharm to gitignore

Description

deid was locked to 2.1.1, but latest is 2.2.2 with some features and bugfixes is available. Lets bump! Also minor addition to gitignore. Related issues: # (issue)

193 Checklist

[ x] I have commented my code, particularly in hard-to-understand areas
[x ] My changes generate no new warnings
[x ] My code follows the style guidelines of this project

Open questions

How could we test changes? I.e. update to DICOM dictionary 2021d

vsoch commented 2 years ago

@wetzelj do you see any issues with bumping pydicom?

wetzelj commented 2 years ago

On the surface, no, I don't see a problem. There are a couple items in the release notes for Version 2.2.0 that may be worth a review for potential impacts:

read_file() is deprecated and will be removed in v3.0, use dcmread() instead.
write_file() is deprecated and will be removed in v3.0, use dcmwrite() instead.
Dataset no longer inherits from dict

I won't be able to immediately pull this into our application and start testing, but will plan for it with our next release (development starting soon).

vsoch commented 2 years ago

Sounds good! I can make some time this weekend to help @glebsts with some of those updates.

glebsts commented 2 years ago

@vsoch what can I do? Until 3.0 no rush with dropping read_file/write_file, could be done in slower manner imo

vsoch commented 2 years ago

My thinking is that we should start to respond to some of these deprecations - eg grep for read and write file and make the changes. I suppose we could also be conservative and just wait for the release of 3.x and then do a major bump in version ourselves - that might be less buggy for died users if someone is using an older version that doesn’t have these new functions. We could also just remove the exact version requirement and allow newer versions probably without issue until 3.x forces change. What do y’all think?

glebsts commented 2 years ago

My approach is to keep scope clear and narrow. Purpose of this PR is to bump pydicom (now it forces me to install not-the-latest pydicom which I also use in my project). As it doesn't seem to introduce breaking changes, please take your time to run some tests if needed, and merge it. Then we can start another PR with getting rid of read_file/write_file. Removing version lock is poentially dangerous if nobody is following pydicom changelog proactively, but making it major-locked on 2.x.x. should be ok. Again, as separate PR. IMHO :)

vsoch commented 2 years ago

That sounds reasonable - we could also have a development branch with these changes and you could work from that for a few weeks to test things out, and then have a more official release after that.

vsoch commented 2 years ago

If you like this approach, here is a branch that you can PR to - https://github.com/pydicom/deid/tree/0.2.29-rc. We could either keep the PR here as is (and indeed we should to prepare to merge to master) and if it makes sense, we can also do a range of versions > 2 and < 3 (and this should be more flexible until the breaking changes for 3.x).

glebsts commented 2 years ago

I am confused. Not sure about your git flow. This specific PR - bump to 2.2.2 locked, how can I get it merged and released? So that I could actually start using it. No point to keep PR hanging.

vsoch commented 2 years ago

@glebsts the idea is that we'd want to have it tested a bit in the wild before just releasing. You are saying you would have issue installing from a branch? It can be done!

pip install git+https://github.com/pydicom/deid.git@0.2.29-rc

It's just a slightly more conservative approach to blindly merging without having a testing period. I think it's likely there won't be any issues and I'd be happy to merge into the main branch and release after a bit of testing. I'm not actively using deid so I am hoping you or others in the community might be able to help with that!

glebsts commented 2 years ago

Thank you for clarification! Yes, it is ok for us to checkout package from branch. I will update PR

vsoch commented 2 years ago

Thank you @glebsts ! Let's get this merged. I'm going to mark my calendar for a month to check in with how the branch is working - @wetzelj I can ping you as well if you plan to test it.

wetzelj commented 2 years ago

I'm targeting to do some testing in the next couple weeks (not guaranteed, but I'm hopeful). Please keep me in the loop.