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

setup.py should read from a minimal-requirements list by default #197

Closed
JelleAalbers opened this issue Jul 6, 2019 · 4 comments
Closed

Comments

@JelleAalbers
Copy link
Member

We pin dependencies to ensure reproducibility of analysis results. However, strax has now become a library used by the actual analysis projects, such as https://github.com/XENONnT/straxen. For straxen specifically, it will be used in a common environment, https://github.com/XENONnT/base_environment. Pinning is best done at the end of a toolchain, where you can have all other requirements (to ensure there is no clash or reversal of a pin) and all the tests you need to do to validate an updated dependency version.

We should switch the current list of pinned dependencies and the pyup subscription @tunnell set up to either straxen or base_environment. See also #5, #6, #7. This may also be of interest to @XeBoris and @rynge.

@rynge
Copy link

rynge commented Jul 8, 2019

Yes, I would be very interested in getting this cleaned up. In base_environment, we are using ripa on the strax repo to get the requirements satisfied:

https://github.com/XENONnT/base_environment/blob/master/create-env#L306

However, my concern is that even though one package pins a particular version, it can later be reversed by a different package which a different pin. I think the reason things have worked fine so far is that strax/straxen is installed at the end so their pins are the ones put in place.

One example of another package we use which also tries to use very specific versions is Rucio:

https://github.com/rucio/rucio/blob/master/tools/pip-requires-client

@tunnell
Copy link
Member

tunnell commented Sep 11, 2019

Don't we strip the pins in PyPI so this shouldn't be an issue?

https://github.com/AxFoundation/strax/blob/master/setup.py#L4

But it allows us during testing to install pinned versions. Or have pinned versions for each release that we know work?

@JelleAalbers
Copy link
Member Author

Good point, the pins do no harm as long as we're clear they are optional. It is also useful to get a warning from the pyup/travis combination if a new dependency release breaks things.

We still get this benefit once the pins are transferred to the common environment, but there we also get notified if a dependency breaks some xenon-specific software like straxen. But no harm having it both there and in strax -- especially since pyup currently doesn't fully support conda dependencies (see XENONnT/base_environment#4: it reads conda.yml but only processes the pip dependencies), so we don't get the full benefits in the common environment.

Probably strax's setup.py should read from another list with minimal requirements, e.g. numba > = 0.44 (see #167).

@JelleAalbers JelleAalbers changed the title Transfer pins to straxen or environment setup.py should read from a minimal-requirements list by default Dec 18, 2019
@JelleAalbers
Copy link
Member Author

Since the pins are no longer updated I removed them for the moment, along with the version stripping. I'll open a new issue for re-enabling them, ideally with new tools.

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

3 participants