Skip to content

Conversation

@DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

This fixes the CI issues we were seeing in #5778 and #5805.
The second argument of these tuples is used to determine the number of jobs. However, the Linux runner only has two cores available. I'm not sure why this is different on Linux and Windows, but I think because we were trying to use more processes than available cores the test became either very slow or completely locked up.
You can see that we these changes both #5778 and #5805 pass the tests in expected times on all environments.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Feb 17, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Feb 17, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1858098748

  • 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 93.991%

Totals Coverage Status
Change from base Build 1840800912: 0.0%
Covered Lines: 14921
Relevant Lines: 15875

πŸ’› - Coveralls

@cdce8p
Copy link
Member

cdce8p commented Feb 17, 2022

Should we can consider reverting #5755 then?

@DanielNoord
Copy link
Collaborator Author

Should we can consider reverting #5755 then?

We could I think. It might make sense, although I'm not sure if there is a difference between organisations' and individual runners and if those could affect run time (I don't think so).

@cdce8p
Copy link
Member

cdce8p commented Feb 17, 2022

Should we can consider reverting #5755 then?

We could I think. It might make sense, although I'm not sure if there is a difference between organisations' and individual runners and if those could affect run time (I don't think so).

AFAIK the main / only difference is the number of concurrent runners.

@DanielNoord
Copy link
Collaborator Author

I'd support revering then!

@Pierre-Sassoulas
Copy link
Member

So we're closing this one and reverting #5755 then ? Just a note, it seems the underlying issue would be fixed by #5809

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Feb 17, 2022

@Pierre-Sassoulas No! This needs to be merged first as this is what's causing all the time outs. Subsequently we can rebase all the dependabot PRs and revert #5755.

Haven't looked at the code yet, but I don't think that PR fixes this. I believe the way this test is invoked sets the number of jobs automatically to 10 (or 5). That PR is referring to when jobs is set to 0, which makes pylint try to figure it out itself. That's a different case I believe.

@DanielNoord
Copy link
Collaborator Author

Btw, thinking about this: should we check if there are two cores available as well? I feel like that's a bare minimum for any test runner nowadays, but that could still crash/hang these tests..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Discussion or action around maintaining pylint or the dev workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants