-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add an upper limit to all runtime dependency requirements #7841
Add an upper limit to all runtime dependency requirements #7841
Conversation
Pull Request Test Coverage Report for Build 3726120036
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like this but it is a bit of a chicken and egg. We don't know that we don't support this versions so we are now unnecessarily blocking people from upgrading their dependencies if they are on an older version of pylint
. I really don't like that.
I prefer fixing dependencies issues as they arise instead of prematurely blocking everything, potentially unnecessarily. But that might be my own opinion...
There's two side of the argument for sure. I think the downside of each needs to be examined in details here.
I think not being able to upgrade immediately is a small price to pay for the stability this brings. In one case you do not have the latest feature of a dependency you might not be even using directly. For the other case everything break in strange way and a build that was working one day do not work the next day. It feel like pylint is not reliable. In all case you will have to wait for maintainers intervention. But would you rather have to wait to upgrade or to wait for your pipeline to be green again ? |
I don't like this either. Pylint, and other linting tools for that matter, are in a difficult spot. Usually the recommendation is for applications to pin their dependencies to one specific release. However, that only works if they are installed inside a clean environment. Thus this doesn't work for linting tools which often require to be installed inside the working venv (especially pylint and mypy).
Technically this isn't correct. Only
I don't see it that way. In case we don't pin an upper bound the issue will likely also be present with the current pylint version. What's likely to happen is that someone will copy the error and create a new issue. We'll investigate it and discover the dependency issue. The fix then will just be to pin the dependency which caused the issue in the first place. Something you can do / try even for old pylint versions. Btw. this is something we have to do quite regularly for Home Assistant since we only pin primary dependencies and build new venvs from scratch each time a dependencies changes. Thus also fetching new secondary deps. We even have a dedicated constraints file just for that.
The reverse is also true however and I believe this is the most important point. If you're using an old pylint version, at some point the upper limits might be outdated. Even if a new major version would just work fine, you can't install it because we have pinned a upper bound (unnecessarily). You can fix a missing upper limit, but not an unnecessarily low one. (At least one easily.) |
I suppose pylint is very often launched in CI in a fresh env. So the CI will breaks, local env will work, multiple persons will spend hours of their time to fix it in parallel, starting by debugging their env, then their tool, then the dependency of their tool, until the issue is created and the problem become searchable. (I did that for autoflake, all CI were broken for 4 hours then all feature branch had to be rebased. I was the first to open the issue in autoflake but not the only one facing the problem).
But aren't the person not updating pylint the least likely to also want to have the freshest possible pylint's dependencies ? Those who have an up to date pylint will get the new version of pylint when it's ready and nothing will be broken in the meantime. |
In order to avoid old releases from becoming unusable if one of our dependencie release a major with breaking changes.
d48a07d
to
c75c358
Compare
I looked at https://iscinumpy.dev/post/bound-version-constraints/#tldr because of PyCQA/isort#1877 and it seems that indeed adding upper limit is not the preferred way. (according to a lot of persons)
|
Type of Changes
Description
In order to avoid old releases from becoming unusable if one of our dependencie release a major with breaking changes. Inspired by the release of pyflakes 3 that broke all version of autoflake at once: PyCQA/autoflake#186