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

[primer] Parallelize primer workflow into package batches #8697

Merged
merged 6 commits into from
May 20, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
✨ New feature

Description

The primer run now completes in half the time (53m -> 28m) by dividing primer packages into batches.

The upside mainly consists of separating home-assistant from pandas, the two largest projects.

Motivations

Notes

  • We could shave a few more seconds off if home-assistant and pandas are given their own exclusive runs, but the few seconds it takes to lint another package in a batch after pandas is not substantial. My hope is that this is good enough and more maintainable.
  • Since the new arguments are optional, this is backwards-compatible with the existing way of running the primer locally without batching.

@jacobtylerwalls jacobtylerwalls added primer Skip news 🔇 This change does not require a changelog entry labels May 17, 2023
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.

Thanks for doing this! Very nice!

.github/workflows/primer_run_pr.yaml Show resolved Hide resolved
.github/workflows/primer_comment.yaml Show resolved Hide resolved
pylint/testutils/_primer/primer_run_command.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #8697 (9ae6173) into main (d7baf5d) will increase coverage by 0.00%.
The diff coverage is 78.57%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8697   +/-   ##
=======================================
  Coverage   95.80%   95.81%           
=======================================
  Files         172      172           
  Lines       18303    18330   +27     
=======================================
+ Hits        17536    17562   +26     
- Misses        767      768    +1     
Impacted Files Coverage Δ
pylint/testutils/_primer/primer_run_command.py 31.81% <0.00%> (-0.49%) ⬇️
pylint/testutils/_primer/primer.py 96.15% <100.00%> (+0.23%) ⬆️
pylint/testutils/_primer/primer_compare_command.py 96.59% <100.00%> (+0.24%) ⬆️

... and 5 files with indirect coverage changes

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.

Looks pretty good ! I think the python parts could be tested in tests/testutils/. (This would be reused in a pylint-testutil package accessible to all pylint plugins maintainer).

@jacobtylerwalls
Copy link
Member Author

Okay, I can take a look at that, but I don't think it's the best use of time. In terms of the code coverage statistics, they're not covered right now because these blocks won't execute until the files exist on main (after merging this).

@Pierre-Sassoulas
Copy link
Member

I think we had issues with primers in the past that would not have happened if the python parts were covered from the get go (even if it's impossible to test the github actions code). That being said, the goal of this MR is not to add tests for everything that is not tested at the moment (only the new batch processing mechanism). I'd approve anyway if you don't want to invest the time this is clearly an improvement compared to what we have right now.

@jacobtylerwalls
Copy link
Member Author

@Pierre-Sassoulas @DanielNoord Does anyone know why the prettier pre-commit hook is failing?

@Pierre-Sassoulas
Copy link
Member

It seems as if prettier was outputting the path of every file in the repository without separators ?

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.

LGTM. I fixed pre-commit but the tests now fails (?)

@jacobtylerwalls
Copy link
Member Author

I fixed pre-commit but the tests now fails (?)

Just looks like a transient failure from the Action we're using.

Thanks for the reviews!

@jacobtylerwalls jacobtylerwalls merged commit 503d360 into pylint-dev:main May 20, 2023
@jacobtylerwalls jacobtylerwalls deleted the parallelize-primer branch May 20, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
primer Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants