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

Partially revert astroid install requirement #4130

Closed
wants to merge 1 commit into from
Closed

Partially revert astroid install requirement #4130

wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 22, 2021

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Partially revert the install_requires version for astroid. It seems to cause too many issues for users and developers if it's pinned to a specific version.

A better solution might be to update the min dependency every so often after a new release turned out to be stable. For this MR I updated it from >=2.4.0 to >=2.4.2 for example.

--

I didn't update the requirements files, since it still might be best to run the CI checks against the latest released version. If that would be of interest, I could add another CI workflow for it. Test different astroid versions maybe once per day.?

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

#4116 (comment)
#4119 (comment)

@coveralls
Copy link

coveralls commented Feb 22, 2021

Coverage Status

Coverage remained the same at 91.452% when pulling cf134f3 on cdce8p:revert-astroid-requires into 8cbcf87 on PyCQA:master.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I think the problem we had for 2.7.0 was because we did not launch the benchmark tests in CI. With this change we would have to test multiple version of astroid for each pylint version. Shouldn't we theoretically have a matrix of astroid version from 2.4.2 to 2.7 in CI for each python interpreter? Plus we can't test the version of astroid that are not yet released

@cdce8p
Copy link
Member Author

cdce8p commented Feb 23, 2021

I was thinking about this from another angle. In theory the minor releases from astroid should not break things. I would guess that most projects that use pylint also use some kind of CI system, travis, github or even just running tox. They will only upgrade the pylint version if everything still works. So we don't necessarily need to force their hand here.

Just to illustrate an example. A project currently uses pylint 2.6.2 and wants to upgrade. They install 2.7.0 and notice a fatal error because of astroid 2.5.0. It might still work fine with 2.4.2 but they can't use it.

--

What I'm trying to say is that we would still recommend using the current astroid version and only actively validate that combination. Once we receive bug reports that a newer combination doesn't work, we could simply update the dependency and do a new bug-fix release. Like what you did for 2.6.0 here.

Additionally we could regularly update the min required astroid version, maybe before a new minor release.

Adding a new CI workflow that checks different astroid versions once per day could also help to identify if a combination breaks.

@Pierre-Sassoulas
Copy link
Member

Hum, I see your point. I feel though that users being able to tinker with astroid version when if we release a broken versions is actually not that good of a thing 😅 ? The fix I did in #4093 (comment) was because pylint broke when a new incompatible astroid was released so I view fixing astroid version as a solution to prevent us from having to do that kind of thing. Imho the reasonable thing would be "astroid>=2.x.z,<2.x+1", because it means we can release new minor astroid without having to do a pylint release but we have no "big" version change to take care of either.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 23, 2021

Hum, I see your point. I feel though that users being able to tinker with astroid version when if we release a broken versions is actually not that good of a thing 😅 ?

True, 😂

"astroid>=2.x.z,<2.x+1"

I agree with >=2.x.z now. However <2.x+1 will cause issue during development. Installing the latest master branch causes a pip issue:

pylint 2.8.0.dev1 requires astroid<2.6,>=2.4.2, but you have astroid 2.6.0.dev0 which is incompatible.

Just to make our lives easier <2.x+2 would probably be better.

it means we can release new minor astroid without having to do a pylint release but we have no "big" version change to take care of either.

If it's an important bugfix for astroid we would probably still need to release a new pylint version with the updated dependency. I don't expect many would update astroid separately.

@Pierre-Sassoulas
Copy link
Member

Ha ok, I see the problem with the continuous integration that need <=x+2. Maybe we can add a step to the release documentation where we make the upper bound smaller just for the release ?

@cdce8p
Copy link
Member Author

cdce8p commented Feb 23, 2021

Ha ok, I see the problem with the continuous integration that need <=x+2.

CI works fine without it! It only uses the release versions (tags) so that's not the issue. It becomes a problem if a developer manually once to test a future dev version of astroid.

Maybe we can add a step to the release documentation where we make the upper bound smaller just for the release ?

IMHO I wouldn't bother updating it assuming that a new minor release of pylint follows shortly after a new one from astroid. Users only noticed the issue since there was a larger gap between astroid 2.5.0 and pylint 2.7.0. It's worth to keep in mind that if something breaks, it's by accident and not design. So it might also just work fine.

@cdce8p cdce8p mentioned this pull request Feb 28, 2021
4 tasks
@Pierre-Sassoulas
Copy link
Member

Closed in favor of #4152

@cdce8p cdce8p deleted the revert-astroid-requires branch February 28, 2021 21:03
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.

3 participants