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

Permit urllib3 >=2 for non-PyPy Python >=3.10 in order to help users of Poetry #830

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

pjonsson
Copy link
Contributor

@pjonsson pjonsson commented Mar 9, 2024

Poetry makes platform indepdent lock files,
so the PyPy marker is there even when using
CPython >= 3.10.

Add a third constraint that permits any urllib3
version when using Python >=3.10 and some
other implementation than PyPy.

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 9, 2024

@hartwork this is my attempt to workaround the issue I think #826 tries to solve. Adding the extra marker made my Python 3.10 project get urllib3 2.x:

$ poetry update
Updating dependencies
Resolving dependencies... (8.9s)

Package operations: 0 installs, 2 updates, 0 removals

  - Updating urllib3 (1.26.18 -> 2.0.7)
  - Updating types-requests (2.31.0.6 -> 2.31.0.20240218)

Writing lock file

And the change to pyproject.toml was to depend on my local copy of the vcrpy repository that contains this commit:

--- a/pyproject.toml
+++ b/pyproject.toml
...
 [tool.poetry.group.dev.dependencies]
+vcrpy = {path = "../../vcrpy"}

...
--- a/poetry.lock
+++ b/poetry.lock
...
[[package]]
 name = "vcrpy"
 version = "6.0.1"
 description = "Automatically mock your HTTP interactions to simplify and speed 
up testing"
 optional = false
 python-versions = ">=3.8"
-files = [
-    {file = "vcrpy-6.0.1.tar.gz", hash = "sha256:9e023fee7f892baa0bbda2f7da7c8ac51165c1c6e38ff8688683a12a4bde9278"},
-]
+files = []
+develop = false
 
 [package.dependencies]
 PyYAML = "*"
-urllib3 = {version = "<2", markers = "platform_python_implementation == \"PyPy\""}
+urllib3 = [
+    {version = "<2", markers = "platform_python_implementation == \"PyPy\""},
+    {version = "*", markers = "platform_python_implementation != \"PyPy\" and python_version >= \"3.10\""},
+]
 wrapt = "*"
 yarl = "*"

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

Hi @pjonsson,

given that Poetry lock files need to work for all implementations including PyPy, the lock file should still pin urllib3 1.x even with this change so that PyPy is kept in the boat by the lock file. So I don't see how workaround would have a chance to change the situation. Please help me see it if it indeed does. Thank you.

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 9, 2024

On the logical level, the third condition for urllib3 version is mutually exclusive to the other two conditions that are already there.

I don't really know Python though, but doesn't:

+urllib3 = [
+    {version = "<2", markers = "platform_python_implementation == \"PyPy\""},
+    {version = "*", markers = "platform_python_implementation != \"PyPy\" and python_version >= \"3.10\""},
+]

pin the version to <2 for PyPy and pin the version to * for CPython/Jython/etc @hartwork?

@hartwork
Copy link
Collaborator

hartwork commented Mar 9, 2024

@pjonsson my current understanding is this:

  • Either the lock file is cross-platform — one single set of versions for all users — or it is not. This workaround only has a chance if it is not because otherwise the PyPy marker still has power and keeps the urllib3 version down.
  • The two sets of markers are not mutually exclusive because "not A and B" is not equal "not A", mathematically speaking.
  • Pulling in any version of urllib3 for say CPython 3.11 is requiring more than the current setup.py (which is fine without any version of urllib3 when it comes to VCR.py's own direct dependencies). It would be surprising that requiring more on VCR.py's side would be turned into something requiring less by Poetry.

If this is a magic trick, I don't see yet why it would work. Please help me understand.

@hartwork
Copy link
Collaborator

hartwork commented Mar 9, 2024

@pjonsson PS: Comment python-poetry/poetry#8996 (comment) helped me understand Poetry's behavior better only three days ago. Maybe that thread is of interest to you also.

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 9, 2024

@hartwork I don't know the inner workings of Poetry, but my understanding is that while the lock file is platform independent, Poetry provides a weaker guarantee than what you describe (otherwise the consequence should be that you are guaranteed to get the same package versions when running poetry install on a machine with Python 3.12 and a machine with Python 3.8, and that eliminates the point which I assume markers was introduced for). I think my observation that I got urllib3 2.x when adding the constraint in this PR supports my reasoning, but I fully admit there are a lot of implicit assumptions in my reasoning, would running the CI for this PR fail for the reason you indicate? If that is the case, could you trigger the jobs so I can see the failure?

Your third point about requiring more turning into relaxing requirements is fair, there are other packages in my pyproject.toml that depend on urllib3, so for me, some version of urllib3 will be installed no matter what.

Your third point is interesting in another way though, why are the current urllib3 constraints in the install_requires rather than extras_requires? I'm just using vcrpy, so I'm fairly sure that my problem would disappear if the urllib3 constraints could be moved to extras_requires.

@hartwork
Copy link
Collaborator

hartwork commented Mar 9, 2024

@hartwork I don't know the inner workings of Poetry, but my understanding is that while the lock file is platform independent, Poetry provides a weaker guarantee than what you describe (otherwise the consequence should be that you are guaranteed to get the same package versions when running poetry install on a machine with Python 3.12 and a machine with Python 3.8, and that eliminates the point which I assume markers was introduced for).

@pjonsson based on python-poetry/poetry#8996 (comment) my understanding is that Poetry's lock file needs to satisfy all marker rules at once. I consider that troublesome, to be clear, but that seems to be status quo.

I think my observation that I got urllib3 2.x when adding the constraint in this PR supports my reasoning

I'm not fully sure how you ended up in that state and why Poetry was okay writing a lockfile like that (given that seems to go against the one-lock-for-all principle; we're probably missing something about it). Is there a chance you could provide a shell session like mine at python-poetry/poetry#8996 (comment) to reproduce your idea here in isolation on a local Bash Linux shell? (We can also do voice call on the topic if you'd be up for it but want to save some typing, just an idea.)

, but I fully admit there are a lot of implicit assumptions in my reasoning, would running the CI for this PR fail for the reason you indicate? If that is the case, could you trigger the jobs so I can see the failure?

I can trigger them, but we have zero Poetry here so it would either be all green and not give new insights or be red from unrelated breakage and still not give new insights. Am I missing something?

Your third point is interesting in another way though, why are the current urllib3 constraints in the install_requires rather than extras_requires? I'm just using vcrpy, so I'm fairly sure that my problem would disappear if the urllib3 constraints could be moved to extras_requires.

They are in install_requires because they are runtime dependency blockers (rather than classic runtime dependencies): VCR.py needs to block being used with those versions of urllib3. Putting that in extras_require would defeats this purpose, it would no longer be effective.

@pjonsson
Copy link
Contributor Author

pjonsson commented Mar 9, 2024

@hartwork sorry, was making dinner when your last reply was written. If there aren't any Poetry tests in this CI there's no need to trigger the tests since that won't make us any wiser.

I agree with your perspective that things seem to be stuck at the status quo on the Poetry side, which is why I'm trying to get a workaround in vcrpy instead. My reading of the comments you link to is that the Poetry people are responding to exactly what you are asking ("Why can't I restrict urllib3 to >= 2?"), which is because that conflicts with the <2 requirement for PyPy as they say, but the more interesting question is which urllib3 should be installed for CPython 3.10 when vcrpy is required alongside some other dependency that depends on "urllib3 >1.0". See the second session below for more on that.

Here's a reproduction of your linked session in the Python 3.10 docker image with a non-root user:

$ cd "$(mktemp -d)"
$ poetry --version
Poetry (version 1.8.2)
$ python -c 'import platform; print(platform.python_implementation(), platform.python_version())'
CPython 3.10.13
$ poetry init -n
$ poetry add --lock git+https://github.com/pjonsson/vcrpy.git#master
Creating virtualenv tmp-y4uyqutvd9 in /tmp/tmp.Y4UYquTvd9/.venv

Updating dependencies
Resolving dependencies... (0.5s)

Writing lock file

$ grep -B1 urllib3 poetry.lock
[[package]]
name = "urllib3"
--
files = [
    {file = "urllib3-1.26.18-py2.py3-none-any.whl", hash = "sha256:34b97092d7e0a3a8cf7cd10e386f401b3737364026c45e622aa02903dffe0f07"},
    {file = "urllib3-1.26.18.tar.gz", hash = "sha256:f8ecc1bba5667413457c529ab955bf8c67b45db799d159066261719e328580a0"},
--
brotli = ["brotli (==1.0.9)", "brotli (>=1.0.9)", "brotlicffi (>=0.8.0)", "brotlipy (>=0.6.0)"]
secure = ["certifi", "cryptography (>=1.3.4)", "idna (>=2.0.0)", "ipaddress", "pyOpenSSL (>=0.14)", "urllib3-secure-extra"]
--
PyYAML = "*"
urllib3 = {version = "<2", markers = "platform_python_implementation == \"PyPy\""}
--
[package.extras]
tests = ["Werkzeug (==2.0.3)", "aiohttp", "boto3", "httplib2", "httpx", "pytest", "pytest-aiohttp", "pytest-asyncio", "pytest-cov", "pytest-httpbin", "requests (>=2.22.0)", "tornado", "urllib3"]

so Poetry will force urllib3 <2 since the lock file is written the way it is. I find it silly that my CPython package installation is forced by constraints marked for PyPy, but that is apparently how Poetry is supposed to work.

Here's the same session with this PR:

$ cd "$(mktemp -d)"
$ poetry --version
Poetry (version 1.8.2)
$ python -c 'import platform; print(platform.python_implementation(), platform.python_version())'
CPython 3.10.13
$ poetry init -n
$ poetry add --lock git+https://github.com/pjonsson/vcrpy.git#permit-urllib3
Creating virtualenv tmp-knnp7e3clg in /tmp/tmp.KNnp7E3clG/.venv

Updating dependencies
Resolving dependencies... (0.4s)

Writing lock file

$ $ grep -B1 urllib3 poetry.lock
[[package]]
name = "urllib3"
--
files = [
    {file = "urllib3-1.26.18-py2.py3-none-any.whl", hash = "sha256:34b97092d7e0a3a8cf7cd10e386f401b3737364026c45e622aa02903dffe0f07"},
    {file = "urllib3-1.26.18.tar.gz", hash = "sha256:f8ecc1bba5667413457c529ab955bf8c67b45db799d159066261719e328580a0"},
--
brotli = ["brotli (==1.0.9)", "brotli (>=1.0.9)", "brotlicffi (>=0.8.0)", "brotlipy (>=0.6.0)"]
secure = ["certifi", "cryptography (>=1.3.4)", "idna (>=2.0.0)", "ipaddress", "pyOpenSSL (>=0.14)", "urllib3-secure-extra"]
--
[[package]]
name = "urllib3"
--
files = [
    {file = "urllib3-2.2.1-py3-none-any.whl", hash = "sha256:450b20ec296a467077128bff42b73080516e71b56ff59a60a02bef2232c4fa9d"},
    {file = "urllib3-2.2.1.tar.gz", hash = "sha256:d0570876c61ab9e520d776c38acbbb5b05a776d3f9ff98a5c8fd5162a444cf19"},
--
PyYAML = "*"
urllib3 = [
--
[package.extras]
tests = ["Werkzeug (==2.0.3)", "aiohttp", "boto3", "httplib2", "httpx", "pytest", "pytest-aiohttp", "pytest-asyncio", "pytest-cov", "pytest-httpbin", "requests (>=2.22.0)", "tornado", "urllib3"]
--
url = "https://github.com/pjonsson/vcrpy.git"
reference = "permit-urllib3"

So adding one more constraint for the remaining configurations forces an installation of some version of urllib3, but any version compatible with the other dependencies in the project is permitted.

@hartwork hartwork changed the title Permit urllib3 2.x for non-PyPy Python >=3.10 Permit urllib3 >=2 for non-PyPy Python >=3.10 in order to help users of Poetry Mar 10, 2024
@hartwork
Copy link
Collaborator

hartwork commented Mar 10, 2024

Hi @pjonsson,

thanks for sharing the reproducer! I tried it out now and I confirm that it makes urllib3 >=2 available to users of CPython 3.10 in practice. I find this behavior of Poetry weird at best. I will let the CI run now so that it shows its green nature, and approve, and add @jairhenrique for a chance on a second opinion.

PS: My personal quite-happy-with alternative to Poetry is this combo:

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.32%. Comparing base (6c4ba17) to head (52da776).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #830   +/-   ##
=======================================
  Coverage   92.32%   92.32%           
=======================================
  Files          27       27           
  Lines        1811     1811           
  Branches      338      338           
=======================================
  Hits         1672     1672           
  Misses         92       92           
  Partials       47       47           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

setup.py Outdated Show resolved Hide resolved
Poetry makes platform indepdent lock files,
so the PyPy marker is there even when using
CPython >= 3.10.

Add a third constraint that permits any urllib3
version when using Python >=3.10 and some
other implementation than PyPy.
@pjonsson
Copy link
Contributor Author

I agree with your assessment of a surprising interpretation of the constraint "<2 for PyPy" when it turns out to mean "<2 for anything" in reality, and thanks for your help @hartwork!

@hartwork hartwork merged commit f3147f5 into kevin1024:master Mar 11, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants