Update Primer venv caching [ci]

[cdce8p]

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

[coveralls]

Pull Request Test Coverage Report for Build 3393900183

• 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.381%

---

Totals
Change from base Build 3393010334:	0.0%
Covered Lines:	17241
Relevant Lines:	18076

---

💛 - Coveralls

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on sentry:
The following messages are now emitted:

1. no-member:
   Generator 'generator' has no 'enter' member
   https://github.com/getsentry/sentry/blob/d6c3d0079301f681be0f174071fc44af5076b4fd/src/sentry/utils/pytest/fixtures.py#L441
2. no-member:
   Generator 'generator' has no 'exit' member
   https://github.com/getsentry/sentry/blob/d6c3d0079301f681be0f174071fc44af5076b4fd/src/sentry/utils/pytest/fixtures.py#L443
3. no-member:
   Generator 'generator' has no 'enter' member
   https://github.com/getsentry/sentry/blob/d6c3d0079301f681be0f174071fc44af5076b4fd/src/sentry/models/file.py#L214
4. no-member:
   Generator 'generator' has no 'exit' member
   https://github.com/getsentry/sentry/blob/d6c3d0079301f681be0f174071fc44af5076b4fd/src/sentry/models/file.py#L216

This comment was generated for commit 8bad978

[Pierre-Sassoulas]

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

[jacobtylerwalls]

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.

[cdce8p]

Maybe we could run the pylint primer on astroid bleeding edge directly in astroid's CI.

That would be awesome. At the moment, I manually check bigger astroid PRs against Home Assistant just to have a first sanity check.

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.

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 astroid bump PR which I think I would prefer.

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.

[jacobtylerwalls]

Without testing bleeding edge, it would show up in the astroid bump PR which I think I would prefer.

That's fine, it's just the case that the original contributor would be long gone. Unless...

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.

👍🏻

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?

Yes, but without the "use" of the astroid feature being intended. As astroid gets more powerful, more Uninferables turn into real nodes, and this increases the surface area of the primer. So someone's pylint PR for a small positive in something completely unrelated crashes when something obscure in flask is newly inferred correctly as a lambda and doesn't have a .__qname__ that the pylint contributor was relying on, and we get the benefit of knowing without waiting for the astroid upgrade. The idea was to have the largest possible surface area for the primer at the moment we have the contributor's attention.

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.