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

updated pydicom dependency from 2.1.1 to 2.2.2 #194

Merged
merged 1 commit into from
Nov 9, 2021
Merged

updated pydicom dependency from 2.1.1 to 2.2.2 #194

merged 1 commit into from
Nov 9, 2021

Conversation

glebsts
Copy link
Contributor

@glebsts glebsts commented Nov 4, 2021

#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
Copy link
Member

vsoch commented Nov 4, 2021

@wetzelj do you see any issues with bumping pydicom?

@wetzelj
Copy link
Contributor

wetzelj commented Nov 4, 2021

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

vsoch commented Nov 4, 2021

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

@glebsts
Copy link
Contributor Author

glebsts commented Nov 4, 2021

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

@vsoch
Copy link
Member

vsoch commented Nov 4, 2021

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

glebsts commented Nov 4, 2021

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

vsoch commented Nov 4, 2021

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

vsoch commented Nov 4, 2021

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

glebsts commented Nov 5, 2021

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

vsoch commented Nov 5, 2021

@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/[email protected]

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

glebsts commented Nov 9, 2021

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

@glebsts glebsts changed the base branch from master to 0.2.29-rc November 9, 2021 14:36
@vsoch
Copy link
Member

vsoch commented Nov 9, 2021

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.

@vsoch vsoch merged commit 53a28f4 into pydicom:0.2.29-rc Nov 9, 2021
@wetzelj
Copy link
Contributor

wetzelj commented Nov 9, 2021

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

@glebsts glebsts deleted the 193-bump-pydicom branch November 13, 2021 14:05
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