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

Add an upper limit to all runtime dependency requirements #7841

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
🐛 Bug fix

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

@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 Maintenance Discussion or action around maintaining pylint or the dev workflow backport maintenance/2.15.x labels Nov 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.7 milestone Nov 24, 2022
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 24, 2022

Pull Request Test Coverage Report for Build 3726120036

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.445%

Totals Coverage Status
Change from base Build 3721302538: 0.0%
Covered Lines: 17663
Relevant Lines: 18506

💛 - Coveralls

Copy link
Collaborator

@DanielNoord DanielNoord left a 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...

@Pierre-Sassoulas
Copy link
Member Author

There's two side of the argument for sure. I think the downside of each needs to be examined in details here.

  • No upper limit downside:

    • A release of a new dependency can break all available versions of pylint
    • If you're using an unmaintained pylint you have to fix the issue yourself because a maintainer fixing all old versions is not realistic
    • Or you if you're using the latest pylint, wait for a maintainer to release a new version and remove pylint from your pipeline in the meantime
    • The only way to fix it yourself otherwise is to understand the problem and fix the upper limit yourself (you have to debug for a long time to understand that a new dependency came out. It's hard to reproduce because you have old installation of pylint that work, and new installation of pylint that break)
  • Upper limit downside:

    • You can't upgrade your dependencies instantly /silently without pylint's maintainers intervention
    • If you really need the new version of the dependency because you have another dependency that is incompatible you have to force pip to install it. (Is it even possible ?)

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 ?

@cdce8p
Copy link
Member

cdce8p commented Nov 25, 2022

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).

A release of a new dependency can break all available versions of pylint

Technically this isn't correct. Only new installs will be broken. If you upgrade pylint alone, pip won't update the dependencies. (Might be different for poetry though.)

If you're using an unmaintained pylint you have to fix the issue yourself because a maintainer fixing all old versions is not realistic

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.

You can't upgrade your dependencies instantly /silently without pylint's maintainers intervention

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.)

@Pierre-Sassoulas
Copy link
Member Author

Only new installs will be broken.
What's likely to happen is that someone will copy the error and create a new issue.

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).

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).

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.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.7, 2.15.8 Nov 27, 2022
@jacobtylerwalls jacobtylerwalls changed the title Add an upper limit to all runtime dependencie requirements Add an upper limit to all runtime dependency requirements Dec 3, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs decision 🔒 Needs a decision before implemention or rejection label Dec 4, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.8, 2.16.0 Dec 4, 2022
In order to avoid old releases from becoming unusable if one of our dependencie release a major with breaking changes.
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the upper-limit-for-dependencies branch from d48a07d to c75c358 Compare December 18, 2022 18:44
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Dec 20, 2022

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)

Thanks to Python steering council member Brett Cannon, Python core developer Paul Ganssle, fellow PyPA members Bernát Gábor, Pradyun Gedam and @layday, fellow RSE Troy Comi, and fellow IRIS-HEP member Alex Held for their comments on early drafts.

@Pierre-Sassoulas Pierre-Sassoulas deleted the upper-limit-for-dependencies branch December 20, 2022 16:23
@Pierre-Sassoulas Pierre-Sassoulas removed Bug 🪲 Needs decision 🔒 Needs a decision before implemention or rejection backport maintenance/2.15.x labels Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants