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

Don't pin dependency to exact version #361

Closed
wants to merge 1 commit into from

Conversation

saschpe
Copy link
Contributor

@saschpe saschpe commented Mar 20, 2014

While this expresses with which versions urllib3 is tested to work with,
almost all distros ship different package versions. To accomodate that
(and to avoid having them to patch away these hard requirements) only
use '>='.

While this expresses with which versions urllib3 is tested to work with,
almost all distros ship different package versions. To accomodate that
(and to avoid having them to patch away these hard requirements) only
use '>='.
@piotr-dobrogost
Copy link

According to https://caremad.io/blog/setup-vs-requirement/ that's how it should be. Just do not use requirements.txt of urllib3.

@saschpe
Copy link
Contributor Author

saschpe commented Mar 20, 2014

That blog post is either wrong or least misses the full picture. Pinning versions may make sense for Django-based web-sites which you want to deploy from source with exact version numbers. On the other hand, a "library" module (such as urllib3) should work with as many versions of it's dependencies as possible. Simply because all distros differ on exact module version they ship (especially across releases). So yes, they would just replace the fixed versions with >=.
Even foo>=1.1,<2.0 would be better, if you want to be totally on the safe side.

It's even more annoying for those distros that auto-generate dependencies from setup.py and *requirements.txt files (such as openSUSE).

@piotr-dobrogost
Copy link

We are talking about test requirements here. This library otherwise has exactly 0 requirements. It could be argued that importing requirements from requirements file into setup.py is not the right thing to do (if you agree with the blog post cited). However these are only test requirements thus it shouldn't be hard to come out with a working test environment...

That blog post is either wrong or least misses the full picture.

@dstufft
See, you should have known better :)

@dstufft
Copy link
Contributor

dstufft commented Mar 20, 2014

Heh, well actually the blog post is perfectly accurate. The problem here is that urllib3 does exactly what my blog says not to do :) Reading the requirements.txt into setup.py is an anti pattern.

@saschpe
Copy link
Contributor Author

saschpe commented Mar 20, 2014

@piotr-dobrogost At openSUSE, we run testsuite during RPM build whenever possible. This helps a lot in having matching package sets that work together. 50% of distro Python-related patches are about fixing upstream dependencies ;-) (BTW. some more are about in-tree libraries and "example" certs or local pem-bundle copies :-).

@dstufft Seems like I should read your post. However, having test-requirements.txt as test_requires in setup.py isn't really an anti-pattern IMO. Especially not if "test_suite" is set to sth. meaningful.

@dstufft
Copy link
Contributor

dstufft commented Mar 20, 2014

requirements.txt are designed to represent the requirements of an environment, typically used with exact == dependencies to pin to exact versions in order to be able to reproduce the environment.

Dependencies specified in setup.py are for packages, typically as wide open as possible because you're not trying to reproduce an environment but instead declare what versions of things are acceptable.

@shazow
Copy link
Member

shazow commented Mar 20, 2014

The reason we started pinning exact versions is because there were several occasions when newer versions had bugs in them which broke urllib3's extensive test suite. In the beginning we'd track the bug and eventually go back and unpin but that was too much work, so now we pin it and update whenever it makes sense which has been less work.

Does that make sense? Do you feel that the reason to unpin is more compelling?

@shazow
Copy link
Member

shazow commented Mar 20, 2014

Heh, well actually the blog post is perfectly accurate. The problem here is that urllib3 does exactly what my blog says not to do :) Reading the requirements.txt into setup.py is an anti pattern.

@dstufft I think one of us may be misunderstanding. Could you expand on how urllib3 does what your blog says not to do? (I've just read your blog and I'm having trouble finding the conflict.)

Edit: Note that urllib3 only uses specific pinning for the test build. The reason we load it in setup.py as well as provide a test-requirements.txt is because there are approximately a billion ways to run the tests, and different developers/continuous integration suites have different limitations/preferences. This has been the easiest way we could make it consistent for everyone.

@kennethreitz
Copy link

requirements.txt files should always specify explicit versions when being used for deployment.

That is not the case here, as @saschpe has pointed out. :)


Bikeshed! :)

@saschpe
Copy link
Contributor Author

saschpe commented Mar 21, 2014

I think @kennethreitz nailed it. I'm happy to keep a local patch in the distro, it's for sure not the only one. However, regardless of what pip files have been designed for, people use it as an authoritative source to express deps under any circumstances these days. TBO, very few devs really distinguish between development, test, doc and production environments there. Hardly any dev cares about the difference of build-time and run-time. But neither pip files nor current setup.py metadata make that exactly easy either, so it may not be their fault alone.

/me keeps on dreaming about http://legacy.python.org/dev/peps/pep-0426/

It may be worth talking about deployment though. From my PoV this makes sense when talking about web apps and their dep trees. It's (again IMO) pointless for "library" modules. People (should) expect those to come from fancy distros, they don't want that from PyPI. The always-working killer argument is that you want the SSL cert store from the distro, not the local / in-tree copy of mozilla's pem bundle from last year. Really, security is the daily business of distros. I would argue that urllib3 matches this category of "library" modules.

@shazow
Copy link
Member

shazow commented Mar 21, 2014

I don't understand what @kennethreitz nailed and what we're still debating.

@saschpe Could you spell out for me what the remaining question is for this issue?

@piotr-dobrogost
Copy link

I don't understand what kennethreitz nailed

I do not, either :)

@saschpe
Copy link
Contributor Author

saschpe commented Mar 24, 2014

@shazow Actually, I hoped the longish answer would do that already :-)

Pinning your testing deps means you support only this exact set of deps, which is unrealistic if people don't just "pip install" your stuff. Even if they do, they may end up with a conflicts in their virtualenv if another lib has other opinions on it's fixed set of deps. The only thing that makes sense is to specify which minimum versions you need. That is reasonable since it implies "I need feature X only present since version Y". It's along the "be liberal and play nice with others" lines.

Back on the "pinning makes sense for deployment" of @kennethreitz. What he means is it makes sense for your private web app to pin it's deps when you deploy onto machines (should you want to use your distro's packages for whatever reasons). But never one of your libraries should pin since there is a 100% chance that they'll pin different things. Imagine, you created $BLOG_APP which needs pyfoo and pybar. pyfoo has requests==1.0 and pybar has requests==1.1. Both claim "we only tested with X". Not nice. You end up writing patches and tell them they should play nice together.

@piotr-dobrogost
Copy link

If we agree with the blog post (which I think we do) then the only problem is that urllib3 pulls in test-requirements.txt inside setup.py (and the fact that someone generates requirements based on requirements.txt files is plain wrong.) setup.py should only exclude versions known not to work.

@saschpe
Copy link
Contributor Author

saschpe commented Mar 24, 2014

Having it that way seems to make everybody happy.

@shazow
Copy link
Member

shazow commented Mar 24, 2014

Do test_requirements get pulled in during install into the global site-packages automagically in some distribution you maintain? I was under the impression it only affects python setup.py test.

Assuming we want the tests to actually pass, we can only guarantee that with specific versions of these modules. I suppose I could remove the test_requirements param from setup.py altogether if it makes your life easier.

I do not want to put up with constantly breaking tests because one of the libs we depend on introduced a bug or changed behaviour. If you would like to volunteer to do that kind of thing for us, I'd be more open to it. :)

@saschpe
Copy link
Contributor Author

saschpe commented Mar 25, 2014

Do test_requirements get pulled in during install into the global site-packages automagically in some distribution you maintain?

No, but they are used to generate BuildRequires for RPM's %clean section.

I was under the impression it only affects python setup.py test.

The test runner doesn't matter at all (use nosetest or testrepository or whatever), it matters that the dependencies need to be satisfied.

Assuming we want the tests to actually pass, we can only guarantee that with specific versions of these modules.

I can tell you, tests pass with different versions.

I suppose I could remove the test_requirements param from setup.py altogether if it makes your life easier.

That won't change a single thing. Again, we want to run testsuites whenever possible. Luckily, you've got one. But we need some dependencies for that. Distros don't download from the internet, they ship RPMs. These RPM python modules do have versions different to what you intend. It is impossible to ship exactly what you want, we have more than 1300 python modules packaged and ~150 for python3 and all their deps have to match. That ignores the huge pile of modules added by OpenStack and others, so consider it a natural fact. Also, Fedora or Debian may ship even different versions and you want to run on those too, right?

I do not want to put up with constantly breaking tests because one of the libs we depend on introduced a bug or changed behaviour.

While there are modules which are more subject to disruptive changes (sqlalchemy or webob for instance), mock, nose and coverage have a really good track record. I don't know enough about tornado though. But that doesn't matter much IMO, this is how open source works. People release new versions of their software and people that use it have to verify their stuff still works. If it doesn't, you either have to fix it or move to other solutions. Denial won't solve anything and it can be considered unfair to put this responsibility onto distro developers.

If you would like to volunteer to do that kind of thing for us, I'd be more open to it. :)

I certainly won't volunteer to test every combination possible, but I can certainly fix the combinations we have.

@shazow
Copy link
Member

shazow commented Mar 25, 2014

While there are modules which are more subject to disruptive changes (sqlalchemy or webob for instance), mock, nose and coverage have a really good track record. I don't know enough about tornado though. But that doesn't matter much IMO, this is how open source works. People release new versions of their software and people that use it have to verify their stuff still works.

You're right, from the top of my head, we've had no recent troubles with nose, mock, and coverage, so we can unpin those. Tornado was the one we had most problems with, and other dependencies pre-Tornado which we got rid of.

Would that be sufficient?

If it doesn't, you either have to fix it or move to other solutions.

If you'd like to migrate the urllib3 testing framework to a different more stable solution, I'm open to that. But it's a ton of work (we have done it twice already) and saying "fix your dependency or move to something else" is not a reasonable proposal in this case.

Denial won't solve anything and it can be considered unfair to put this responsibility onto distro developers.

Saying that this is denial is harsh.

I feel like we're talking past each other. I'm trying to give you concrete reasons why we did this. I apologize if I make your life as a distro developer harder—this is not something I want to do. We experienced real maintenance pains dealing with these issues and this was the solution we came to. Last thing I want to do is create maintenance pain for others, too.

I apologize if I'm being dense or missing some of the points you've made. I'm sure you understand that everyone working on these things has their own lives and sometimes limited time/attention we can spare on open source community projects. I appreciate your patience.

@schlamar
Copy link
Contributor

Tornado was the one we had most problems with

This was a major update (2.x to 3.x) so breaking changes can be expected. IMO the most reasonable way would be to limit dependencies to the current major version, e.g. >=3.1.1,<4.0.

@shazow
Copy link
Member

shazow commented Apr 29, 2014

The pin in question: 69b7558

That was going 2.1.1 -> 2.3.

#76 (comment)

Good times.

@schlamar
Copy link
Contributor

That was going 2.1.1 -> 2.3.

Ah interesting, didn't know about that (this was before my time in urllib3 involvement ;) However, at that time the Tornado test base was horribly broken by race conditions (see #171, #212) so this could have been a usage problem. :)

@shazow shazow closed this in 7193214 Jun 24, 2014
@shazow
Copy link
Member

shazow commented Jun 24, 2014

I think I found a good compromise for this.

setup.py now has no version pinning for test_requires, while dev-requirements.txt continues to have specific pinning (which is used by our Makefile).

As a distro package maintainer, as long as you're using python setup.py test, you should get no version demands. As a urllib3 developer, you'd use pip install -r dev-requirements.txt or make test and you'll get all the specific versions that we all build against. Also I'm going to make an effort to keep these up to date using piprot.

Hopefully this makes everyone's life easier.

See also #415 for changes to the development/testing configuraiton layout that may affect repackaging. (This will probably be published as part of v1.9)

@dstufft
Copy link
Contributor

dstufft commented Jun 24, 2014

That matches my definitions of how these files should be used too ;) (For whatever that's worth)

@shazow
Copy link
Member

shazow commented Jun 24, 2014

Now that dev-requirements.txt is becoming more than just the bare-minimum of what you'd need to run the tests, this scenario makes a lot more sense to me too. :)

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.

None yet

6 participants