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

Use compatible release syntax to set bounds on required dependencies #453

Closed
matthewfeickert opened this issue Feb 17, 2021 · 5 comments
Closed

Comments

@matthewfeickert
Copy link
Member

In the same vein as Issue #452, almost none of the required dependencies have upper bounds

https://github.com/diana-hep/madminer/blob/7fc053b23beca88429c0120b2b7d7b6c049944a9/setup.py#L36-L46
https://github.com/diana-hep/madminer/blob/7fc053b23beca88429c0120b2b7d7b6c049944a9/setup.py#L117

This can very easily cause old releases to break in the future, and in the case of uproot I would be shocked if there aren't broken madminer releases out there now given the uproot3 / uproot4 transition. In my experience, using compatible release syntax is the easiest way to both support a range of releases while still setting lower and uppper bounds (c.f. this PR for an example).

This also ensures that when there are API breaking changes in future major releases you have the ability to select them to come in instead of having all of your CI break.

@matthewfeickert
Copy link
Member Author

If it would be helpful I'm happy to contribute a PR either to master or to part of PR #442.

@Sinclert
Copy link
Member

Hi @matthewfeickert ,

Yes, I agree with you that this would be extraordinary very helpful.

I lack the context to know which versions of those packages MadMiner was designed to interact with. Due the small test-suite, I can think of the following options:

  • Option A) Define the compatible release syntax to the current versions of those packages and hope for the best 🤞🏻
  • Option B) Define the compatible release syntax to the latest versions of those packages when those dependencies were added (using GIT commit history to discover this).

Opinions on the approach?

@matthewfeickert
Copy link
Member Author

Yes, I agree with you that this would be extraordinary very helpful.

👍

I lack the context to know which versions of those packages MadMiner was designed to interact with. Due the small test-suite, I can think of the following options:

  • Option A) Define the compatible release syntax to the current versions of those packages and hope for the best 🤞🏻
  • Option B) Define the compatible release syntax to the latest versions of those packages when those dependencies were added (using GIT commit history to discover this).

Opinions on the approach?

I lack the context as well, so my recommendations should be properly down weighted given that, but I would suggest option A. Here's my thoughts why:

  • This is fundamentally easier for the madminer dev team as you can just start from the versions available now and move forward.
  • madminer is a library that has multiple external requirements, and so already is going to exist in rather bespoke environments. The burden any users would face with updating their Python virtual environments to use newer releases of the Python dependencies seems quite minor. While a library should attempt to allow for a wide range of versions with stable APIs and good performance, the bespoke environments that users need for madminer make supporting as many releases as possible a smaller concern (in my mind at least).
  • Option B will be harder as you will (probably?) need to change some parts of the API that use uproot as the API breaking uproot3 / uproot4 break happened very recently. When pyhf went through this we ended up deciding to move everything we could to the uproot4 API and then only keep uproot3 around for writing of ROOT files (coming to future uproot v4 releases) but this also requires us to install both uproot3 and uproot at the moment. If you have the ability to skip over things like this then seems like an easy choice. :)

@Sinclert
Copy link
Member

Hey @matthewfeickert ,

I just ran into the uproot3 VS uproot4 issue you were anticipating, when using this project with Delphes... (most of the problems are located within the madminer/utils/interfaces/delphes_root.py module) 🙄 .

I currently do not have the bandwidth to go over them in a comprehensive manner, so I may create a small PR implementing some lower-bound and upper-bound constraints on the list of unbounded dependencies.

Do you have any suggestion for the other dependencies? If so, please let me know so I can add those to the PR.

@matthewfeickert
Copy link
Member Author

I just ran into the uproot3 VS uproot4 issue you were anticipating, when using this project with Delphes... (most of the problems are located within the madminer/utils/interfaces/delphes_root.py module) .

Ah bummer. :(

I currently do not have the bandwidth to go over them in a comprehensive manner, so I may create a small PR implementing some lower-bound and upper-bound constraints on the list of unbounded dependencies.

Do you have any suggestion for the other dependencies? If so, please let me know so I can add those to the PR.

Sure. I recently changed my tune a bit on this given some convincing from Henry and Hynek Schlawack (c.f. scikit-hep/pyhf#1382), so I think that I'd suggest just setting lower bounds still, but setting up some tests that are tightly testing the lowest bounds you support and be ready to update them as needed.

I don't have much experience here with predicitng what will and won't play nice with things like Delphes (maybe @kratsg does given https://gitlab.cern.ch/scipp/mario-mapyde), but I'll make some comments on your PR. 👍

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

No branches or pull requests

2 participants