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
Update Primer venv caching [ci] #7708
Conversation
Pull Request Test Coverage Report for Build 3393900183
💛 - Coveralls |
🤖 Effect of this PR on checked open source code: 🤖 Effect on sentry:
This comment was generated for commit 8bad978 |
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.
It make sense to remove astroid bleeding edge imo, as we're not running on astroid bleeding edge anywhere else so it's confusing. Maybe we could run the pylint primer on astroid bleeding edge directly in astroid's CI. I don't know on with which pylint, as long as there are no breaking change in astorid the latest published pylint is the best one but it's hard to know what to use during the time where astroid has breaking change and bleeding edge pylint is not ready for it yet.
Let's wait for primer experts review (@jacobtylerwalls , @DanielNoord ) before merging :)
My intent behind installing astroid bleeding edge in the primer was that it provides the greatest opportunity for finding new false positives caused in pylint PRs. It would be difficult to look through the haystack of closed PRs for examples, but I thought I saw potential regressions on PRs that were only evident when using astroid bleeding edge. I also wasn't too worried about breaking changes, because breaking-changes-on-main-causing-astroid-error == breaking-changes-on-pr-causing-astroid-error, but I guess that has the potential to hide some false positives, outweighing the original motivation. I guess it comes down to how often we forecast having breaking changes in astroid versus having regressions on pylint PRs we could otherwise catch. |
That would be awesome. At the moment, I manually check bigger astroid PRs against Home Assistant just to have a first sanity check.
Not so sure about this one but maybe I'm missing something. Isn't the only case this would be useful if both an unreleased astroid change and some potential pylint change that uses the astroid one come together? Even then, we'll need to bisect astroid either way. Idk tbh, this seems like a fairly remote case. Without testing bleeding edge, it would show up in the Either way the best workaround is probably to do frequent astroid releases if there are any new features. We have gotten a lot better with those lately. The new release process has definitely helped a lot. |
That's fine, it's just the case that the original contributor would be long gone. Unless...
👍🏻
Yes, but without the "use" of the astroid feature being intended. As astroid gets more powerful, more Ultimately I think you have a good point--the faster release cadence is a good reason to remove astroid bleeding edge in the primer. Was just adding some context. |
* Use release version for astroid * Use better cache key * Mirror create environment * Update comments
* Use release version for astroid * Use better cache key * Mirror create environment * Update comments
Description
Followup to #7651 (comment)
To prevent a
Primer / Run
failure if the hash changes, add separate step to create venv in that workflow too. With Github actions cache scoping, the cache entry can only be used for the specific PR.Additionally, remove install of bleeding-edge astroid. That only works as long as the
main
astroid branch doesn't contain any breaking changes.With the new cache key, it's no longer necessary to update the
CACHE_VERSION
when doing an astroid update./CC: @DanielNoord, @Pierre-Sassoulas