cjolowicz / cookiecutter-hypermodern-python

Hypermodern Python Cookiecutter
http://cookiecutter-hypermodern-python.readthedocs.io/
MIT License
1.8k stars 233 forks source link

Add Nox session running the tests without pinned dependencies #817

Open Spectre5 opened 3 years ago

Spectre5 commented 3 years ago

This is a really impressive cookiecutter template, very nice work. I like the solution you've used for pre-commit as I've been considering doing the same thing for my projects --- editing the hook file to use the virtual environment. Currently I do the poetry run X trick instead.

My question though is regarding Nox. You've setup an extremely repeatable build process by making use of the poetry lock file and enforcing those versions in the various Nox sessions with the constraints. There are obvious benefits to this, but I also understand that a benefit of tox (and Nox, I believe) is that it builds and tests not just with different python versions but also different dependencies (and possibly sub dependencies). Thus it normally installs from the sdist, since that is what would normally be used from PyPI. When your project is uploaded and installed from PyPI, there will be no lock file and the exact state of the installed virtual environment is unknown. There are countless possibilities, of course, since the project could be installed along with others and you can't test every possible combination. But installing the sdist without constraints would at least be akin to installing into a fresh virtual environment. What are your thoughts on this?

cjolowicz commented 3 years ago

Hey @Spectre5 thanks for the feedback!

Like you say, it's not feasible to test against all dependency versions that satisfy the constraints of your package. Installing without a lock file means that checks are no longer repeatable, which is not desirable IMO. However, you can explicitly test your package against different versions of specific dependencies. I would recommend using the nox.parametrize decorator to specify dependency versions, and installing the specified version after you have installed your package witth the pinned dependencies. I don't think the project template should include boilerplate for this, as it depends on the need of a specific project. But there is an example here: https://github.com/cjolowicz/nox-poetry/issues/268#issuecomment-774549874

As for installing from sdist, I'm not sure I'm following your premise. Both wheel and sdist specify dependencies the same, namely the way they're declared in pyproject.toml. The lock file doesn't enter the picture here. Also, there is no preference for sdist when installing from PyPI.

Spectre5 commented 3 years ago

Thanks for that example, I'll check it out.

Regarding sdist vs wheel, let me clarify: I didn't really mean to differentiate between an sdist and wheel. What I really meant to differentiate was installing with and without the constraints. The way this project is setup, it changes the python version but all libraries are identical due to the constraints file being used, which is intentional here. But this doesn't test the most basic case of a user installing from PyPI into a new virtual environment since newer dependencies may get used in that case.

Let me provide a simple example (xyz is a dummy library for example purposes):

Say you have a requirement for library xyz ^2.0 and you target Python 3.6+. The latest version of xyz is 2.5.0, but it is only compatible with Python 3.7+, thus Poetry pins the library to 2.4.0 in the lock file. All CI checks are good and you release to PyPI. Someone installs your library into a new virtual environment with Python 3.8 and you get xyz 2.5.0 installed since the wheel has the requirement as Requires-Dist: xyz (>=2.0,<3.0) based on the pyproject.toml file. But there were breaking changes made in 2.5.0 of library xyz, so your library doesn't work. Arguably, the xyz library shouldn't be making breaking changes in a minor release, but it can and does happen. Or perhaps you used xyz >= 2.0 and the breaking changes come in a 3.0 version of the library. Using >= 2.0 is dangerous for that reason, but a library is often trying to keep requirements as "open as possible" so that it can maintain compatibility with as many other libraries as possible.

The point is that this project doesn't test against the most likely installation, i.e. using the sdist or wheel from PyPI without lock file constraints into a clean environment.

I'm not specifically trying to argue for any changes here, I'm mainly trying to make sure I understand all of the aspects here and that I'm not missing something. If I'm right, then it seems reasonable to test with all of the constraints as you have here, and then to also test without them. If one fails and not the other, then you know exactly where the issue is and can make the appropriate updates, changes, or library version limitations. This would double the number of test environments, but if that is too onerous then you could only test "unconstrained" in the latest Python version (e.g. Python 3.9).

cjolowicz commented 3 years ago

Thanks for elaborating, you're raising some interesting points!

Here's another scenario where end users might not get the dependencies that are used in CI: This project template has an indirect dependency on requests via several development dependencies. As of this writing, requests depends on idna < 3, while idna's latest version is 3.1. So if your project depends on idna, your tests run against idna 2.10, while users typically end up with idna 3.1. This happens because the lock file includes both core and development dependencies, while the package metadata only includes core dependencies.

Let me provide a simple example (xyz is a dummy library for example purposes):

Say you have a requirement for library xyz ^2.0 and you target Python 3.6+. The latest version of xyz is 2.5.0, but it is only compatible with Python 3.7+, thus Poetry pins the library to 2.4.0 in the lock file. All CI checks are good and you release to PyPI. Someone installs your library into a new virtual environment with Python 3.8 and you get xyz 2.5.0 installed since the wheel has the requirement as Requires-Dist: xyz (>=2.0,<3.0) based on the pyproject.toml file. But there were breaking changes made in 2.5.0 of library xyz, so your library doesn't work. Arguably, the xyz library shouldn't be making breaking changes in a minor release, but it can and does happen.

Just some comments on the specific scenario you're presenting here. This is more of a side note.

My first thought was that this might work just fine using environment markers for the Python version.

I attempted to reproduce this using Python 3.5 and the regex library; regex 2020.10.28 was the last version to support Python 3.5. Running poetry add regex, Poetry pinned regex to the latest version anyway, which makes the package uninstallable under Python 3.5.

Next I tried to configure multiple constraints depending on the Python version:

[tool.poetry.dependencies]
python = "^3.5"
regex = [
    {version = "==2020.10.28", python = "3.5"},
    {version = "^2020.11.13", python = "^3.6"},
]

Poetry exports the following requirements file:

regex==2020.10.28; python_version == "3.5"
regex==2020.11.13; python_version >= "3.6" and python_version < "4.0"

Passing this as a constraints file to pip reports Collecting regex==2020.10.28 in a Python 3.5 environment, and Collecting regex<2021.0.0,>=2020.11.13 in a Python 3.9 environment. The console message under Python 3.9 looks a little like pip ignored the constraints file. But playing around with this some further, I think it does take the constraints file into account, even though the console message only mentions the constraint from the package metadata.

To conclude, in this scenario, and given an up-to-date lock file, the CI should run with the same dependencies that a user gets when installing from PyPI.

Or perhaps you used xyz >= 2.0 and the breaking changes come in a 3.0 version of the library. Using >= 2.0 is dangerous for that reason, but a library is often trying to keep requirements as "open as possible" so that it can maintain compatibility with as many other libraries as possible.

Shouldn't this be caught once you (or Dependabot) update your lock file?

The point is that this project doesn't test against the most likely installation, i.e. using the sdist or wheel from PyPI without lock file constraints into a clean environment.

Yes, I think you're right about that.

I'm not specifically trying to argue for any changes here, I'm mainly trying to make sure I understand all of the aspects here and that I'm not missing something. If I'm right, then it seems reasonable to test with all of the constraints as you have here, and then to also test without them. If one fails and not the other, then you know exactly where the issue is and can make the appropriate updates, changes, or library version limitations. This would double the number of test environments, but if that is too onerous then you could only test "unconstrained" in the latest Python version (e.g. Python 3.9).

I'm -1 on running a mandatory check without pins in CI, because in the event of a breaking change, this will break the entire CI pipeline. Preventing this from happening is one of the very reasons to pin dependencies. Breaking changes should only affect the PR that updates the dependency.

But I think we may be able to do better here. To start with, we could have an optional Nox session unpinned-tests (feel free to bikeshed on the name) that is identical to tests but uses the plain nox.session decorator instead of nox_poetry.session. To avoid code duplication, both sessions could invoke an undecorated session function containing the current implementation.

As for CI checks, here are some rough ideas with options:

Spectre5 commented 3 years ago

Sounds like a bug in poetry if in pinned in to a version that Python 3.6 isn't compatible with. I'll look into that later myself too for my own interest and report anything I find.

I like your ideas on when and how to run the unpinned tests.

Spectre5 commented 3 years ago

For regex, it looks like it doesn't fill in the python_requires: https://pypi.org/pypi/regex/2021.3.17/json

And if you look at the sdist, there is no minimum python requirement there either. So I think this is why Poetry still pins to the latest version even though th pyproject.toml file includes Python 3.5. I don't believe that Poetry uses the classifiers. It first tries the JSON data from PyPI and then tries downloading the sdist if it doesn't find a requirement in the JSON data.

Spectre5 commented 3 years ago

Here is a follow up on this issue.

Let's say you have a project that needs to support Python 3.6 and up and uses the library matplotlib. Here is a quick dummy pyproject.toml for it. Note that I've used version ^3.3 for matplotlib here since that is the last version compatible with Python 3.6. The recently released 3.4.0 is only compatible with Python 3.7 and up.

[build-system]
requires = ['poetry-core>=1.0.0']
build-backend = 'poetry.core.masonry.api'

[tool.poetry]
name = 'dummy'
version = '0.0.1'
description = 'dummy'
authors = ['dummy <dummy>']

[tool.poetry.dependencies]
python = '>=3.6'
matplotlib = '^3.3'

Poetry then locks the library to 3.3.4, which looks great.

matplotlib = [
    {file = "matplotlib-3.3.4-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:672960dd114e342b7c610bf32fb99d14227f29919894388b41553217457ba7ef"},
    {file = "matplotlib-3.3.4-cp36-cp36m-manylinux1_i686.whl", hash = "sha256:7c155437ae4fd366e2700e2716564d1787700687443de46bcb895fe0f84b761d"},
    {file = "matplotlib-3.3.4-cp36-cp36m-manylinux1_x86_64.whl", hash = "sha256:a17f0a10604fac7627ec82820439e7db611722e80c408a726cd00d8c974c2fb3"},
    {file = "matplotlib-3.3.4-cp36-cp36m-win32.whl", hash = "sha256:215e2a30a2090221a9481db58b770ce56b8ef46f13224ae33afe221b14b24dc1"},
    {file = "matplotlib-3.3.4-cp36-cp36m-win_amd64.whl", hash = "sha256:348e6032f666ffd151b323342f9278b16b95d4a75dfacae84a11d2829a7816ae"},
    {file = "matplotlib-3.3.4-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:94bdd1d55c20e764d8aea9d471d2ae7a7b2c84445e0fa463f02e20f9730783e1"},
    {file = "matplotlib-3.3.4-cp37-cp37m-manylinux1_i686.whl", hash = "sha256:a1acb72f095f1d58ecc2538ed1b8bca0b57df313b13db36ed34b8cdf1868e674"},
    {file = "matplotlib-3.3.4-cp37-cp37m-manylinux1_x86_64.whl", hash = "sha256:46b1a60a04e6d884f0250d5cc8dc7bd21a9a96c584a7acdaab44698a44710bab"},
    {file = "matplotlib-3.3.4-cp37-cp37m-win32.whl", hash = "sha256:ed4a9e6dcacba56b17a0a9ac22ae2c72a35b7f0ef0693aa68574f0b2df607a89"},
    {file = "matplotlib-3.3.4-cp37-cp37m-win_amd64.whl", hash = "sha256:c24c05f645aef776e8b8931cb81e0f1632d229b42b6d216e30836e2e145a2b40"},
    {file = "matplotlib-3.3.4-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:7310e353a4a35477c7f032409966920197d7df3e757c7624fd842f3eeb307d3d"},
    {file = "matplotlib-3.3.4-cp38-cp38-manylinux1_i686.whl", hash = "sha256:451cc89cb33d6652c509fc6b588dc51c41d7246afdcc29b8624e256b7663ed1f"},
    {file = "matplotlib-3.3.4-cp38-cp38-manylinux1_x86_64.whl", hash = "sha256:3d2eb9c1cc254d0ffa90bc96fde4b6005d09c2228f99dfd493a4219c1af99644"},
    {file = "matplotlib-3.3.4-cp38-cp38-win32.whl", hash = "sha256:e15fa23d844d54e7b3b7243afd53b7567ee71c721f592deb0727ee85e668f96a"},
    {file = "matplotlib-3.3.4-cp38-cp38-win_amd64.whl", hash = "sha256:1de0bb6cbfe460725f0e97b88daa8643bcf9571c18ba90bb8e41432aaeca91d6"},
    {file = "matplotlib-3.3.4-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:f44149a0ef5b4991aaef12a93b8e8d66d6412e762745fea1faa61d98524e0ba9"},
    {file = "matplotlib-3.3.4-cp39-cp39-manylinux1_i686.whl", hash = "sha256:746a1df55749629e26af7f977ea426817ca9370ad1569436608dc48d1069b87c"},
    {file = "matplotlib-3.3.4-cp39-cp39-manylinux1_x86_64.whl", hash = "sha256:5f571b92a536206f7958f7cb2d367ff6c9a1fa8229dc35020006e4cdd1ca0acd"},
    {file = "matplotlib-3.3.4-cp39-cp39-win32.whl", hash = "sha256:9265ae0fb35e29f9b8cc86c2ab0a2e3dcddc4dd9de4b85bf26c0f63fe5c1c2ca"},
    {file = "matplotlib-3.3.4-cp39-cp39-win_amd64.whl", hash = "sha256:9a79e5dd7bb797aa611048f5b70588b23c5be05b63eefd8a0d152ac77c4243db"},
    {file = "matplotlib-3.3.4-pp36-pypy36_pp73-macosx_10_9_x86_64.whl", hash = "sha256:1e850163579a8936eede29fad41e202b25923a0a8d5ffd08ce50fc0a97dcdc93"},
    {file = "matplotlib-3.3.4-pp36-pypy36_pp73-manylinux2010_x86_64.whl", hash = "sha256:d738acfdfb65da34c91acbdb56abed46803db39af259b7f194dc96920360dbe4"},
    {file = "matplotlib-3.3.4-pp37-pypy37_pp73-macosx_10_9_x86_64.whl", hash = "sha256:aa49571d8030ad0b9ac39708ee77bd2a22f87815e12bdee52ecaffece9313ed8"},
    {file = "matplotlib-3.3.4-pp37-pypy37_pp73-manylinux2010_x86_64.whl", hash = "sha256:cf3a7e54eff792f0815dbbe9b85df2f13d739289c93d346925554f71d484be78"},
    {file = "matplotlib-3.3.4.tar.gz", hash = "sha256:3e477db76c22929e4c6876c44f88d790aacdf3c3f8f3a90cb1975c0bf37825b0"},
]

However, when you build it, you get this requirement in the wheel (as expected):

Requires-Dist: matplotlib (>=3.3,<4.0)

Now, installing this wheel into a Python 3.6 environments ends up with matplotlib 3.3.4. If a 3.3.5 was released in the future, then of course it would have that instead (which is newer than our locked version used in testing). But installing this wheel into a Python 3.7+ environment ends up with matplotlib 3.4.0. This potential issue was exactly what my original issue was trying to express. So since all testing using this cookiecutter project would be using matplotlib 3.3.4, we would not be testing with what users would actually get installed via PyPI using the wheel. And if 3.4.0 has breaking changes, then our dummy library would potentially be broken. So this is where an unpinned test would potentially help, which could be part of releasing as you've suggested.

cjolowicz commented 3 years ago

Thanks for the example and the analysis, that's helpful!

So I think the next step would be to add an unpinned-tests session to the Noxfile. Would you be interested in working on a PR?

Spectre5 commented 3 years ago

Sure, as for how to integrate the test, you listed 3 good options. I was thinking that I'd go with adding it to the Tests workflow but allowing it to failure (let me know if you prefer one of the other options). However, unless I'm mistake, it doesn't appear that github actions allows failures based on this issue. It is a long issue and there is a work around or two listed there, so I could try one of those although it surprises me that there is not a native allow failure CI check in github actions?

cjolowicz commented 3 years ago

Hm I wasn't aware of this GA limitation. We could:

I'd also be happy if we left CI out of the picture for now.

I was also planning to add a CI job for Python 3.10 using continue-on-error, so that's affected too.