search

pydicom / deid

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

remove restriction on matplotlib (results in error) #111

Closed vsoch closed 4 years ago

vsoch commented 4 years ago

This pull request is opened to provide a base for testing / debugging the current issue with updating matplotlib. Specifically, the problem is issue #109 - by setting a max (working) version you have version conflicts when installing with conda. The branch here removes the dependency, and I welcome testers to take it for a spin before we decide to merge proper.

Signed-off-by: vsoch vsochat@stanford.edu

vsoch commented 4 years ago

This has been tested in #109 and I'm approving merge into a release.

davidparsson commented 4 years ago

I'm noting that this PR does not completely remove the version restriction, but rather reverses it. Is that intended to be merged as is?

vsoch commented 4 years ago

I'm confused - you reported that you tested this. What exactly did you test?

vsoch commented 4 years ago

The pull requests sets a minimum version of the known supported version, which would allow for installing anything above that. This is a change from the previous == requirement.

INSTALL_REQUIRES = (
    ("matplotlib", {"min_version": "2.1.2"}),
    ("pydicom", {"exact_version": "1.2.1"}),
    ("python-dateutil", {"min_version": None}),
)

Could you please provide detail on 1) what you tested if it wasn't this, and 2) what solution you expected here.

davidparsson commented 4 years ago

This PR is what I tested, and indeed it sets a minimum version. I think it made sense to set the minimum version to allow testing it properly, but what I expected to be released was a change that completely remove the version restriction.

I believe that libraries like this one should try to be as permissive as possible when it comes to versions of dependencies, to make life easier for projects relying on this one, and this change removes support for a lot of versions that was allowed before without any obvious reason.

That being said, I am personally all right with this being released since I want the newer version.

vsoch commented 4 years ago

I respect that you have personal beliefs, but I need more than that to completely lift a minimum version requirement. A version less than 2.1.2 would be very old, and I don't have any evidence that this would work. It's already a little shaky lifting the exact requirement, and I am almost sure we will hit some point in the future when an update to matplotlib breaks something here (and I'll need to address that then). This is just the burden of maintaining software - it's a living thing that is constantly changing, and undoubtedly there will be another user in time that reports that lifting the requirement has broken his/her build.

vsoch commented 4 years ago

I see your point though - it's completely reversed. I'm also not comfortable with this now that you've pointed it out - even if it would work for new installs, it might break older ones that already have an older matploblib. This is very messy, but I'll do another small point release that lifts it entirely.

Next time, could you please properly review the PR, or report that you did not? The miscommunication has led to this.

davidparsson commented 4 years ago

Regarding the miscommunication. I really think you'd have to be very clear if you expect a code review from anyone that's not a maintainer as that's not common practice in my experience. But I'm sorry if I've caused any trouble.

Thanks again for being helpful, and for timely responses and releases!

vsoch commented 4 years ago

I think I was pretty clear, I explicitly said "test the PR."

hey @davidparsson any updates on being able to test the PR? I also wanted to get your feedback on using matplotlib-base see the conda suggestion here! Would you have time in the coming future to test either of these for your use case?

What do you need me to say next time?

vsoch commented 4 years ago

And there is no golden rule that contributors, however they might contribute (a question / request via issue or PR) should not also be helpful. In this case (and actually many cases) I am the only maintainer and I ask for help because I need it. I don't have a team of folks I can turn to (the "other maintainers") that have taken on the responsibility for testing. It's just me.

davidparsson commented 4 years ago

And there is no golden rule that contributors, however they might contribute (a question / request via issue or PR) should not also be helpful.

Ok, that's not fair. Is that your response to constructive criticism? I'm the sole maintainer of several projects so I'm aware of the burden, so I've done my best to be both helpful and polite. I've spent a few hours on verifying the functionality of the PR (according to my definition of "testing"), but if you don't think that's helpful then I'm sorry.

vsoch commented 4 years ago

That was very helpful, thank you. Have a nice day!

davidparsson commented 4 years ago

You too! I really appreciate the quick resolution and release!