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

Loosen version constraint on core dependencies and re-pin lock file #409

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

jherland
Copy link
Member

@jherland jherland commented Jan 4, 2024

Fixes #408.

Use >= instead of ^ in pyproject.toml to not be too restrictive as
to which version of our dependencies is acceptable when a user installs
FawltyDeps into their project's venv.

The ^ specifier allows for "SemVer compatible" updates to the specified
version, but the exact rules around these are complex, and - it seems -
more restrictive than what we want (at least for most of our deps.

The new >= will allow for any newer version of a dependency to be
used. We will need to be somewhat careful (and frequent) in checking that
a new version a dependency does not break the APIs we are using.

See https://python-poetry.org/docs/dependency-specification/ for more
details about the different available specifiers.

To verify that upgraded dependencies do not break FawltyDeps, we upgrade
all our pinned versions to the latest available and modulo two small
nitpicks reported by the latest Mypy version, it seems we're all good to
go! :-)

Commits:

  • Apply suggested pyproject.toml changes in issue Stop pinning core dependencies #408
  • Re-pin dependencies in lock file
  • Appease Mypy after updating pinned version from 1.0.1 to 1.4.1

@0x26res
Copy link

0x26res commented Jan 5, 2024

Re-pin dependencies in lock file

Did you run poetry update? I would have thought this would update setuptools and importlib_metadata in the lock file, but (as far as I can tell) they don't look updated.

@jherland
Copy link
Member Author

jherland commented Jan 5, 2024

Re-pin dependencies in lock file

Did you run poetry update? I would have thought this would update setuptools and importlib_metadata in the lock file, but (as far as I can tell) they don't look updated.

I ran poetry lock (which AFAICS also updates, unless --no-update is passed). setuptools does not seem to have been updated, I agree, but importlib_metadata was upgraded from 6.6.0 to 6.7.0.

When I run poetry update in my local tree now, it makes no changes to the poetry.lock file at all.

I don't know yet why this does not update to the latest versions available on PyPI...

@0x26res
Copy link

0x26res commented Jan 5, 2024

setuptools does not seem to have been updated

It could be because another dependency, most probably in the extra deps (lint, format, dev, nox, test) is pinned to an older version of setuptools. I had a look (with pipdeptree), and couldn't find the offender.

In a way it doesn't matter, when resolving dependencies, pip will use the information in the main dependencies section of the pyproject, it won't look at the loc file. The only reason we'd want setuptools to be updated the lock file is if we want to run the test against a newer version of setup tools.

@dorranh
Copy link

dorranh commented Jan 5, 2024

I ran poetry lock (which AFAICS also updates, unless --no-update is passed). setuptools does not seem to have been updated, I agree, but importlib_metadata was upgraded from 6.6.0 to 6.7.0.

When I run poetry update in my local tree now, it makes no changes to the poetry.lock file at all.

I don't know yet why this does not update to the latest versions available on PyPI...

@jherland might this be due to how the version bound for types-setuptools is currently defined?

types-setuptools = "^65.6.0.2"

@jherland
Copy link
Member Author

jherland commented Jan 5, 2024

@dorranh: changing our types-setuptools dependency from ^65.6.0.2 to >=65.6.0.2 happily upgrades types-setuptools to 69.0.0.0, but does not affect setuptools at all.

I think the reason setuptools is stuck at 68.0.0 is actually quite a bit simpler: setuptools v68.1.0 removed support for Python v3.7, which means that for as long as FawltyDeps keeps supporting v3.7 Poetry will keep setuptools at <v68.1.0 (aka. ==v68.0.0 as of now).

This fact was not obvious from our poetry.lock as it only contains information about the versions of dependencies that Poetry has found to be compatible, and says nothing about why a version of a dependency was found incompatible.


FWIW, I think this might point to a bug in Poetry... If I change our pyproject.toml like this:

-setuptools = ">=68.0.0"
+setuptools = [
+    # setuptools 68.1.0 drops support for Python v3.7:
+    {version = ">=68.0.0", python = ">=3.8"},
+    {version = ">=68.0.0,<68.1.0", python = "<3.8"},
+]

then I shouldn't really have changed anything in Poetry's dependency calculation; in both cases:

  • on Python 3.7 setuptools is restricted to >=68.0.0 (from our dependency specification), and <68.1.0 due to setuptool's own python requirement.
  • on Python >=3.8 setuptools is only restricted to >=68.0.0 (from our dependency specification).

However, with the above diff applied I do get newer setuptools installed in all venvs that have Python >=3.8.

$ for venv in .nox/tests-3-*; do echo "--- $venv:"; "$venv/bin/pip" list | grep setuptools; done
--- .nox/tests-3-10:
setuptools              69.0.3
--- .nox/tests-3-11:
setuptools              69.0.3
--- .nox/tests-3-12:
setuptools              69.0.3
--- .nox/tests-3-7:
setuptools              68.0.0
--- .nox/tests-3-8:
setuptools              69.0.3
--- .nox/tests-3-9:
setuptools              69.0.3

FTR, the same argument applied to importlib_metadata (which dropped support for Python 3.7 in v6.8.0). Applying the equivalent "over-specification" convinces Poetry to install the latest version on Python >=3.8:

$ for venv in .nox/tests-3-*; do echo "--- $venv:"; "$venv/bin/pip" list | grep importlib-metadata; done
--- .nox/tests-3-10:
importlib-metadata      7.0.1
--- .nox/tests-3-11:
importlib-metadata      7.0.1
--- .nox/tests-3-12:
importlib-metadata      7.0.1
--- .nox/tests-3-7:
importlib-metadata      6.7.0
--- .nox/tests-3-8:
importlib-metadata      7.0.1
--- .nox/tests-3-9:
importlib-metadata      7.0.1

So it seems to me that Poetry mistakenly applies its Python 3.7 calculations to all other Python versions unless we jump through hoops to "over-specify" our dependencies. I'm not sure though, am I missing something here?

@jherland
Copy link
Member Author

jherland commented Jan 5, 2024

I've created a minimal demo at https://github.com/jherland/poetry_overspecify_deps_demo. AFAICS this exact issue does not exist on the Poetry issue tracker yet?

@jherland
Copy link
Member Author

jherland commented Jan 8, 2024

I opened a discussion at https://github.com/orgs/python-poetry/discussions/8857 for now. Let's see if it is promoted into a bug. In the meantime, I guess maybe the best approach for this PR is to "over-specify" our dependencies, to improve test coverage across appplicable dependency versions?

@jherland
Copy link
Member Author

jherland commented Jan 9, 2024

I opened a discussion at https://github.com/orgs/python-poetry/discussions/8857 for now.

Discussion concluded: This is a known shortcoming, unlikely to change in the near future. I guess we need to keep over-specifying our (main) dependencies for now.

This is _not_ our usual `poetry lock --no-update`, instead we run
`poetry lock` to re-pin all our dependencies to the latest available
version that matches our version constraints in pyproject.toml.
Over-specify our main dependencies to get the latest versions of
dependencies installed on more recent Python versions. Without this,
the versions of these dependencies chosen by Poetry will be the ones
that are compatible with Python 3.7, even if the current Python version
is much newer.

We do this to improve our test coverage for dependency versions that are
more likely to be used when FawltyDeps are installed with pip (in which
case Poetry is not involved, and the looser version constraints in
pyproject.toml are followed).

See https://github.com/orgs/python-poetry/discussions/8857 and
https://github.com/jherland/poetry_overspecify_deps_demo for more
details.
@jherland jherland force-pushed the jherland/stop-pinning-core-deps branch from bc02226 to 577fdd2 Compare January 9, 2024 08:25
@0x26res
Copy link

0x26res commented Jan 9, 2024

I apologise for sending you in this rabbit hole. I guess it's not a huge deal to over specify the deps for now, given you don't have many of them.

Copy link
Collaborator

@mknorps mknorps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Johan for that fix! This is not an obvious thing that Poetry does, and it is good we can at leas work our way around that complication.

My comments are in the spirit of "specify narrow if you can". You did two changes in some places - split requirement version dependening on the Python version and changed caret requirement to ">=". I think the latter change is not necessary. Following poetry documentation on the version specification:

For instance, if we previously ran poetry add requests@^2.13.0 and wanted to update the library and ran poetry update requests, poetry would update us to version 2.14.0 if it was available, but would not update us to 3.0.0. If instead we had specified the version string as ^0.1.13, poetry would update to 0.1.14 but not 0.2.0. 0.0.x is not considered compatible with any other version.

Looking also at the examples they gave, caret requirement is allowing a change in the second meaningful digit in the version number.

Copy link
Collaborator

@mknorps mknorps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @jherland I agree that being more restrictive about the major version may not help us ensure that FawltyDeps will not fail.

We better create tests that check for the drift of requirements version bounds and a check if FawltyDEps is working on the updated versions. Described in #411.

@jherland jherland merged commit 7f80e4a into main Jan 11, 2024
@jherland jherland deleted the jherland/stop-pinning-core-deps branch January 11, 2024 12:29
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

Successfully merging this pull request may close these issues.

Stop pinning core dependencies
4 participants