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

Limit to major releases rather than second point. #5342

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

cjw296
Copy link
Contributor

@cjw296 cjw296 commented Feb 17, 2020

requests should trust dependent packages to do semver rather than artificially limiting version compatibility, which causes problems for pip.

Fixes #5341, #5337 and supercedes #5226.

setup.py Outdated
'urllib3>=1.21.1,<1.26,!=1.25.0,!=1.25.1',
'chardet>=3.0.2,<4',
'idna>=2.5,<3',
'urllib3>=1.21.1,<2,!=1.25.0,!=1.25.1',
Copy link
Member

@sethmlarson sethmlarson Feb 17, 2020

Choose a reason for hiding this comment

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

I agree with the other dependencies, however probably best to leave the urllib3 version locks. Requests and urllib3 coordinate releases usually so the version locks are safe. :)

Copy link
Contributor Author

@cjw296 cjw296 Feb 17, 2020

Choose a reason for hiding this comment

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

If releases are coordinated, surely there's no need for a lockstep version pin?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Seth that the maintainers should be managing the urllib3 version bump.

Requests digs into the internals of urllib3 in a handful of places. While urllib3 may release a non-breaking change for external APIs, those changes have broken Requests in the past. There have also been cases where a release of urllib3 introduces a non-trivial bug that requires a few weeks to fix. For that reason we usually wait a few days to a week to do the official bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, any reason not to just specify urllib3>=1.25,<1.26?

Copy link
Member

Choose a reason for hiding this comment

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

The != declarations were made because the first couple releases of urllib3 1.25 had breaking changes. We'll clean those up once the next minor version is out and we bump the pin to < 1.27.

Copy link
Contributor Author

@cjw296 cjw296 Feb 18, 2020

Choose a reason for hiding this comment

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

Sure, but can you think of a reason why anyone would need, or indeed want, to stick to urllib3 1.21.1 to 1.24.x? (I ask, because as we've learned, pip's "solver" doesn't like complicated version requirements, so anything we can do to simplify, the better).

Copy link
Member

Choose a reason for hiding this comment

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

We’ve left the wider gap open since individuals may not have complete control over their ecosystem and what version of urllib3 that’s being vended.

Eventually we’ll need to start pulling the tail up, but we haven’t really talked about when to do that yet. I’d say we’d want to support at least 3 trailing minor versions. Let’s just leave it for now.

@nateprewitt
Copy link
Member

Thanks for the patch, @cjw296! I'm a bit torn on this one. I agree that logistically this is a painful process for all parties involved. A minor version bump should be backwards compatible, but inevitably, that rule will be broken. The question comes down to how should a library used in a number of pieces of critical infrastructure handle that case?

We've had a lot of discussion on this in #4673 (Reasoning: 1, 2). The specifics of that ticket are for urllib3, but it applies similarly to chardet and idna. The short of it being we'd rather be explicit on what we support in the event issues arise. The way pip handles conflicting dependencies makes this case more complicated, but that's unfortunately the environment we're currently in.

I'm happy to have the discussion again if we've got opinions from Seth or others on the Requests team.

@cjw296
Copy link
Contributor Author

cjw296 commented Feb 18, 2020

@sethmlarson seems in agreement with what I've suggested, and I didn't realise how intertwined the urllib3 usage was, so what you've both said makes sense. Would you be amenable to merge and release if I rebase, and change the urllib3 spec to the one I suggest above?

@nateprewitt
Copy link
Member

nateprewitt commented Feb 18, 2020

chardet is pretty stable and dormant now, so I'm not so concerned about that pin. idna is kind of an unknown. It's probably safe, I'm just not excited on having to do an emergency release to pin it in the future after any potential damage has already been done.

If Seth wants to approve and merge, I'm ok with it once the urllib3 pins are reverted.

@cjw296
Copy link
Contributor Author

cjw296 commented Feb 18, 2020

To be clear: I've had to do emergency releases of at least three packages as a result of this pin and the idna release. It's a risk call for you, the requests maintainers: do you want to do emergency releases of requests on every new release of a dependency, or do you want to do an emergency release under the, hopefully rare, circumstances that a dependency doesn't do semver right.

@cjw296
Copy link
Contributor Author

cjw296 commented Feb 18, 2020

@sethmlarson - over to you :-)

The idna is in the install_requires already,
so it makes sense to remove it from extra requirements.

Also, poetry has been confused by this duplicated requirement.
python-poetry/poetry#1449
@jacobtomlinson
Copy link

jacobtomlinson commented Feb 18, 2020

Just to add weight to @cjw296 I have also had to take emergency action as a result of the idna release and requests pinning it to a minor version.

From my perspective the risk of a minor release being breaking is valid but hopefully low. Though I completely acknowledge that it must be frustrating for the requests maintainers to have to resolve it with an emergency release when it inevitably happens.

However debugging the issues caused today by the idna release, because requests has pinned a minor version, has been unpleasant. One specific example is that a package I maintain does not depend on requests, but a dependency does. Another dependency has idna as a dependency but is pinned on major versions. It seems pip was unable to resolve this dependency graph correctly and pulled in 2.9, breaking requests, the dependency of my package and therefore the whole package. Adding requests as a dependency to my package and pinning to <2.9 has resolved it and resulted in an emergency release.

This has been frustrating to untangle and has fallen on those that depend on requests to resolve.

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks good to me, @nateprewitt are you going to do a release of this post-merge?

@sethmlarson sethmlarson merged commit c46f55b into psf:master Feb 18, 2020
@cjw296 cjw296 deleted the patch-1 branch February 18, 2020 15:12
daviddavis pushed a commit to daviddavis/pulpcore that referenced this pull request Feb 18, 2020
More info: psf/requests#5342

fixes #6169
daviddavis pushed a commit to daviddavis/pulpcore that referenced this pull request Feb 18, 2020
More info: psf/requests#5342

[noissue]
@nateprewitt
Copy link
Member

Yep, I'll work on getting a cleaned up release out today or tomorrow.

aless10 pushed a commit to aless10/requests that referenced this pull request Feb 19, 2020
maxdotdotg added a commit to maxdotdotg/CTFd that referenced this pull request Aug 9, 2020
…lopment.txt

related PRs to address this conflict in later versions
* moto: getmoto/moto#2807
* requests: psf/requests#5342
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Oct 12, 2020
ports r552091 updated idna to 2.10, breaking www/py-requests, and its
dependents, with:

raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'idna<2.9,>=2.5' distribution was not found and is required by requests

Backport an upstream commit [1] that kinda addresses "most" of the root
cause problem of this for future dependency updates, by not pinning on
minor versions.

[1] psf/requests#5342

PR:		245938
Reported by:	lbartoletti, ohauer
MFH:		2020Q3
X-MFH-With:	552091


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@552095 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Oct 12, 2020
ports r552091 updated idna to 2.10, breaking www/py-requests, and its
dependents, with:

raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'idna<2.9,>=2.5' distribution was not found and is required by requests

Backport an upstream commit [1] that kinda addresses "most" of the root
cause problem of this for future dependency updates, by not pinning on
minor versions.

[1] psf/requests#5342

PR:		245938
Reported by:	lbartoletti, ohauer
MFH:		2020Q3
X-MFH-With:	552091
Jehops pushed a commit to Jehops/freebsd-ports-legacy that referenced this pull request Oct 12, 2020
ports r552091 updated idna to 2.10, breaking www/py-requests, and its
dependents, with:

raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'idna<2.9,>=2.5' distribution was not found and is required by requests

Backport an upstream commit [1] that kinda addresses "most" of the root
cause problem of this for future dependency updates, by not pinning on
minor versions.

[1] psf/requests#5342

PR:		245938
Reported by:	lbartoletti, ohauer
MFH:		2020Q3
X-MFH-With:	552091


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@552095 35697150-7ecd-e111-bb59-0022644237b5
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idna requirement causing problems now idna 2.9 has been released.
5 participants