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

Specify versions in requirements.txt #2177

Closed
lucas-wilkins opened this issue Sep 2, 2022 · 8 comments
Closed

Specify versions in requirements.txt #2177

lucas-wilkins opened this issue Sep 2, 2022 · 8 comments
Labels
Critical High priority Enhancement Feature requests and/or general improvements Requires Community Coding Effort Issues that require a collaborative coding effort to resolve correctly

Comments

@lucas-wilkins
Copy link
Contributor

lucas-wilkins commented Sep 2, 2022

Specify versions in requirements.txt so that new releases of dependencies don't break SasView.

This was mentioned by @pkienzle before as it is an issue with build reproducibility (#1986), but the consequence of not doing this are more severe than this - recently numpy was updated and the main branch stopped working.

We really need to sort this soon.

@lucas-wilkins lucas-wilkins added Enhancement Feature requests and/or general improvements Community Input Required Requires Community Coding Effort Issues that require a collaborative coding effort to resolve correctly labels Sep 2, 2022
@llimeht
Copy link
Contributor

llimeht commented Sep 21, 2022

While acknowledging that version pinning can make some aspects of the build more reproducible and the release process simpler, I also urge some caution here. We've seen in SasView that not keeping pace with code deprecations is building up technical debt, and that technical debt can become difficult to pay down at some future date. I've seen other projects pin themselves into irrelevance.

  • Compatibility with new versions of numpy/scipy/matplotlib etc needs to come at some point
  • Pinning old versions provides a false sense that things are "OK" while there is actually a growing todo list from those new versions
  • Pinning old versions can be pinning security vulnerabilities
  • pyinstaller has a narrow compatibility window for its workarounds on packages; you almost always want the most recent pyinstaller to have the best chance of success in making the installer for the current operating systems, but each pyinstaller release will only work with recent module releases

Pinning versions can work, but only if there is good discipline in ensuring that the version pins are always reasonable and not accidentally stuck on unrealistic old versions. That means an amount of janitorial work in reviewing the pins and either updating them to match current releases, or filing (then fixing!) bugs if there are compatibility problems. The Dependabot service can help with this, but it is only as good as the test suite.

Obviously, the best outcome is to write code that is compatible with broad ranges of versions for each dependency. Many packages like pytest, matplotlib, numpy, scipy, sphinx have quite long and well publicised deprecation schedules meaning that it's possible to write code that works across several years' worth of versions. This is quite important for downstream packagers of SasView — when the SasView package hits Debian (and Ubuntu and other derivatives), it will be using whatever the currently packaged versions of the libraries are, regardless of the pins.

@lucas-wilkins
Copy link
Contributor Author

Yeah, that's why this issue has the needs community coding effort tag. The versions need to be bounded in a sensible way, not just set to a specific one.

@butlerpd
Copy link
Member

I wonder if the real issue is the relatively large number of dependencies and the lack of resources for keeping up with all the breaking changes and/or testing thoroughly enough? Not sure there is a fix for that so maybe not a helpful question?

@lucas-wilkins
Copy link
Contributor Author

In fairness, the tests did catch the problem, it's just that it didn't stop it breaking things.

Having lots of dependencies isn't a bad thing, as long as they're well chosen. In fact its generally better to use an external package that someone else is maintaining and putting effort into, that try to reinvent the wheel.

@pkienzle
Copy link
Contributor

Hybrid solution: peg specific versions for release, but run PR tests on both version=peg (all platforms) and version (one platform). That way we get some warning about future breaking changes. For libraries such as sasmodels and sasdata testing against version=min is also necessary. This strategy works best with good test coverage.

At the start of the release cycle peg all packages to the latest version. That way the manual GUI testing will be using relatively up-to-date packages that won't change between test and release.

Supporting multiple versions can require additional code, but with a little care we can program to the latest interfaces while still supporting the pegged version. Usually this can be done with a shim at the top of the file implementing the new interface when it detects the old version, marking it as CRUFT.

$ grep "CRUFT" sasview/src/**/*.py
sasview/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py:# CRUFT: remove when new release of sasmodels is available
sasview/src/sas/qtgui/Perspectives/Fitting/GPUOptions.py:        # CRUFT: next version of reset_environment() will return env
sasview/src/sas/qtgui/Plotting/Binder.py:                # # CRUFT matplotlib-0.91 support
sasview/src/sas/qtgui/Utilities/GuiUtils.py:    # CRUFT: SasView 4.x uses a callback interface to register bits of state
sasview/src/sas/qtgui/Utilities/PlotView.py:        if hasattr(Gcf, '_activeQue'):  # CRUFT: MPL < 3.3.1
sasview/src/sas/sascalc/corfunc/corfunc_calculator.py:        # CRUFT: numpy>=1.14.0 allows rcond=None for the following default
sasview/src/sas/sascalc/fit/BumpsFitting.py:    # CRUFT: Bumps changed its handling of fit options around 0.7.5.6
sasview/src/sas/sascalc/fit/expression.py:    # CRUFT: python < 3.0;  doc builder isn't allowing the following exec
sasview/src/sas/sascalc/invariant/invariant.py:            # CRUFT: numpy>=1.14.0 allows rcond=None for the following default
sasview/src/sas/sascalc/pr/invertor.py:        # CRUFT: numpy>=1.14.0 allows rcond=None for the following default

@pkienzle
Copy link
Contributor

Implementation detail: The requirements.txt format supports os-dependent installs (stackoverflow link).

For example, some of the conditional installs expressed in ci.yml from PR #2263 could be expressed like:

...
PyQt5; sys_platform != "darwin"
PyQt5=1.13; sys_platform == "darwin"
# pyopencl on windows requires "chaco install opencl-intel-cpu-runtime" before(?) pip install
pyopencl; platform_system != "Windows"
pyopencl --only-binary=pyopencl --find-links http://www.silx.org/pub/wheelhouse/ --trusted-host www.silx.org pyopencl; platform_system == "Windows"
...

@llimeht
Copy link
Contributor

llimeht commented Oct 27, 2022

What is the plan for transitive dependencies? There's over 100 of them and they are just as likely to change and break things but they're not specified in requirements.txt - I don't think we'd want them in files that are designed to be maintained by humans.

The complete output of pip freeze is in the current CI in the Installer jobs under "Python package version list".

One can imagine

  • a human-editable file with no pins except the occasional one added to avoid a known bad version, containing only the main dependencies that are actually imported into the sasview code itself.
  • a code-managed file (basically the output of pip freeze, and could be managed by DependaBot) that pins all the versions.

@lucas-wilkins
Copy link
Contributor Author

Think this has been implicitly decided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical High priority Enhancement Feature requests and/or general improvements Requires Community Coding Effort Issues that require a collaborative coding effort to resolve correctly
Projects
None yet
Development

No branches or pull requests

4 participants