Better primer tests (faster, less duplication, more diverse)

[Pierre-Sassoulas]

Current problem

Right now the primer tests take more than double the time of the next slower tests. Also the CI configuration is duplicated between primer for stdlib and primer for external tests. The choice of repository to check is also arbitrary.

Desired solution

• Update primer tests to own file, separate batches and concurrent runs #5425 > Burst the primer-external CI job into multiple jobs to cut down on CI run time
• Update primer tests to own file, separate batches and concurrent runs #5425 > Remove the duplication in the GitHub action configuration and move primer to its own workflow file
• Clarify the criteria for choosing a repository and update the documentation
• Remove repositories that are redundant or less useful considering what we already have
• Fixing dependency to a specific commit to be able to cache repository
• Caching of repository for primers
• Remove duplication in primer jobs by using a job template
• Update primer tests to own file, separate batches and concurrent runs #5425 > Only run when something changes in pylint internals or primer testutils
• Improve error message on crashes and fatal errors within the primer logs
• Check and see if CI workflow file can be shortened/improved

Additional context

#5173

[DanielNoord]

@Pierre-Sassoulas Just something I thought of:
I'm not sure if we get unlimited amount of CI minutes from Github, but if we need to be conservative with them we could consider not running the external primer on all three versions.

[Pierre-Sassoulas]

We definitely need to cache everything we can too. (Setting the hash of the repository we lint would help).

[cdce8p]

I'm not sure if we get unlimited amount of CI minutes from Github

@DanielNoord AFAIK public repos are only limited in the number of concurrent job runners, not the actual time.

• https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#about-billing-for-github-actions
• https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

[cdce8p]

One suggestion for something I saw recently. If the primer tests are moved to a new workflow, you could also add concurrency control. I.e. it doesn't make sense to continue with primer tests if a new commit has been pushed in the meantime.
https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#concurrency

This is how Home Assistant does it: Home-Assistant -> ci.yaml, although it probably isn't necessary to cancel all other jobs as they are quite fast and even partial feedback could be useful.

[Pierre-Sassoulas]

Huge first step was done in #5425 by @DanielNoord we still can do:

• Fixing dependency to a specific commit to be able to cache repository
• Caching of repository for primers
• Remove duplication in primer jobs by using a job template

[Pierre-Sassoulas]

Moving to 2.14 as we have quite a lot to do already for 2.13.

[DanielNoord]

With the work on the new primer most of the TODOs in this issue have become redundant.

I think there are a couple more things I would like to do before closing this:

• Update documentation
• Re-consider all projects on the list/move all projects to the new linter
• One more review of all workflow files to check for improvements
• Re-add home-assistant, see be07298

The last point is mainly because syntax highlighting for yaml (and especially other languages within yaml) is quite poor and I think we might have missed some stuff. I tend to forget to rename steps for example.

[jacobtylerwalls]

Once again, terrific work @DanielNoord!

[DanielNoord]

For point 2, I'd like to get 2/3 namespace packages as well. Preferably one that uses and one that doesn't use pylint. I have no good candidates myself, but if anybody can think of one please mention them here!

[hmc-cs-mdrissi]

google packages are a good one. especially google.cloud ones as they have two levels of namespace nesting which is uncommon. Both google is a namespace package and google.cloud is also namespace package.

[Pierre-Sassoulas]

@hmc-cs-mdrissi are you talking about something in https://github.com/orgs/GoogleCloudPlatform/repositories?q=&type=all&language=python&sort= ?

[cdce8p]

Thanks for all the work here @DanielNoord, much appreciated! It will make all our lives easier when working on a new feature or bug fix.

Some question since I didn't follow the discussion closely.

1. Does the current implementation install project dependencies, too?
2. Is useless-suppression enabled? I think it would nice to know if those change, e.g. when a PR fixes a bunch of false-positives.
3. Why was Home Assistant removed / what is necessary to add it back? I saw some mention of performance issues. As a data point, my pylint run usually takes around 8-10min.
4. Any reason the primer message doesn't use permalinks? They might not look that pretty, but at least they will continue to work if the files change. Example: Emit used-before-assignment for variables only defined under always false tests #6677 (comment) -> check the pytest links which all don't work anymore.

[DanielNoord]

1. Does the current implementation install project dependencies, too?

No. This is something we should do, as we're running into issues with this with #6780. Should be fairly easy with some additional caching of environments.

2. Is useless-suppression enabled? I think it would nice to know if those change, e.g. when a PR fixes a bunch of false-positives.

It should be! We only disable duplicate-code and cyclic-import and enable all other messages and extensions.

3. Why was Home Assistant removed / what is necessary to add it back? I saw some mention of performance issues. As a data point, my pylint run usually takes around 8-10min.

The full run was taking more than 45 minutes with Home Assistant compared to the 24 minutes it takes now.
However, @jacobtylerwalls also theorised some of the slowdown could be due the size of the dictionary that stores all emitted messages and how we write that to file. Improving this process might also speed up the linting of Home Assistant.

4. Any reason the primer message doesn't use permalinks? They might not look that pretty, but at least they will continue to work if the files change. Example: Emit used-before-assignment for variables only defined under always false tests #6677 (comment) -> check the pytest links which all don't work anymore.

Oh huh. I thought using main would make them permalinks. What is the correct structure for permalinks then? Commit tag instead of branch name?

[jacobtylerwalls]

As a data point, my pylint run usually takes around 8-10min.

The new primer enables all extensions. There could be some molasses in there.

the size of the dictionary that stores all emitted messages and how we write that to file.

I doubt that's a big performance impact, but I'll still put together a little refactor; if anything, it would help with more intermediate feedback when using the tool locally.

[jacobtylerwalls]

I just fired up the primer tool locally and encountered an infinite loop in astroid while linting pandas. I didn't dig in yet.

[cdce8p]

3. Is useless-suppression enabled? I think it would nice to know if those change, e.g. when a PR fixes a bunch of false-positives.

It should be! We only disable duplicate-code and cyclic-import and enable all other messages and extensions.

Not sure it's all that useful to enable all other messages and extensions. At least for Home Assistant, we carefully decided what to and what not. to enable. I would recommend to stick with that, even if we might miss some things then. The only overwrites which makes sense IMO are useless-suppression, and the ones you mentioned duplicated-code and cyclic-import.

7. Any reason the primer message doesn't use permalinks? They might not look that pretty, but at least they will continue to work if the files change. Example: Emit used-before-assignment for variables only defined under always false tests #6677 (comment) -> check the pytest links which all don't work anymore.

Oh huh. I thought using main would make them permalinks. What is the correct structure for permalinks then? Commit tag instead of branch name?

Yes. An example from pylint.

https://github.com/PyCQA/pylint/blob/main/pylint/__main__.py#L7
https://github.com/PyCQA/pylint/blob/e52f3f5bb17984256e6716aa8ca1fd950521105c/pylint/__main__.py#L7

[DanielNoord]

Not sure it's all that useful to enable all other messages and extensions. At least for Home Assistant, we carefully decided what to and what not. to enable. I would recommend to stick with that, even if we might miss some things then. The only overwrites which makes sense IMO are useless-suppression, and the ones you mentioned duplicated-code and cyclic-import.

I don't think specific disables and enables matter that much. The primer only shows changes between main and PR. So if main emits a warning that is disabled in a configuration file it will also show up in the PR output and thus cancel out. Only differences between main and PR will show up in the final comment.

So we can show that a warning that is disabled in configuration no longer shows up due to a PR fixing a false positive.

Yes. An example from pylint.

Ah, this should be fixed then!

[jacobtylerwalls]

There are some frequently-disabled messages that are a little opinionated or lead to false positives. I think we still want primer results on those. So we can't just use the project's enable/disable list.

I just noticed the old primer run was also enabling all extensions, so that shouldn't be it. I'll add Home Assistant back to the old primer and we can see what the run time is.

[Pierre-Sassoulas]

There is a very interesting list of possible project that was done for benchmarks.

[DanielNoord]

Let's close this at some point.

FINAL TODO FOR THIS ISSUE:

• Update documentation about primer. What is it? Which projects are primed?
• Do one final check for potential speed ups
• Move last remaining projects to the new primer as well as home-assistant

[mbyrnepr2]

Sorry if this is not the right place but wanted to ask - could we add coverage to the primer suite? The author opened an issue in our tracker recently so I thought it would be a good one to include since its a popular tool & it is using Pylint.

[DanielNoord]

Makes sense! Do you want to open the PR?

[mbyrnepr2]

Sure @DanielNoord. It'll force me to take a closer look at the logic. I'll aim for next couple of days

[Pierre-Sassoulas]

Spoiler, but:

You need to modify this file, or this file.