Skip to content

Conversation

@cdce8p
Copy link
Member

@cdce8p cdce8p commented Jan 31, 2022

Description

Followup to #5752. After merging the first one, I noticed timeout issues on my personal fork. PyCQA/pylint seems to be fine. Maybe it's related to the number of concurrent runners available?

Anyway, I increased the timeouts so that they don't trigger accidentally anymore.

@cdce8p cdce8p added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jan 31, 2022
@cdce8p cdce8p added this to the 2.13.0 milestone Jan 31, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1774944226

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

Totals Coverage Status
Change from base Build 1774754642: 0.0%
Covered Lines: 14767
Relevant Lines: 15730

💛 - Coveralls

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.

Yeah we have a limited number of runner, we need a compromise for when we push a lot of commits (generally just before release time) but I think if there's too much commit it's actually a good thing to time out some pipeline.

@Pierre-Sassoulas Pierre-Sassoulas merged commit ade493a into pylint-dev:main Jan 31, 2022
@cdce8p cdce8p deleted the ci-timeouts-2 branch February 1, 2022 13:03
@cdce8p
Copy link
Member Author

cdce8p commented Feb 1, 2022

Yeah we have a limited number of runner, we need a compromise for when we push a lot of commits (generally just before release time) but I think if there's too much commit it's actually a good thing to time out some pipeline.

@Pierre-Sassoulas For these case I would suggest looking into the concurrency option, especially the cancel-in-progress option. https://docs.github.com/en/actions/using-jobs/using-concurrency

This is what Home-Assistant does. It should work for us too if you like to add it.
https://github.com/home-assistant/core/blob/2021.12.10/.github/workflows/ci.yaml#L18-L20

@DanielNoord
Copy link
Collaborator

Personally I really like that we keep running CI over all commits as its easier to backtrack and find a fault commit. But I can see why in the interest of decreasing the number of runners being used we want to add concurrency.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 1, 2022

Personally I really like that we keep running CI over all commits as its easier to backtrack and find a fault commit. But I can see why in the interest of decreasing the number of runners being used we want to add concurrency.

Generally, me too. In some cases it can make sense though. Especially for longer running tasks, like the primer tests. I would leave that up to you two to decide.

@DanielNoord
Copy link
Collaborator

For primer concurrency is enabled! I'll leave this to @Pierre-Sassoulas, I don't have a particularly strong opinion.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 1, 2022

For primer concurrency is enabled! I'll leave this to @Pierre-Sassoulas, I don't have a particularly strong opinion.

Didn't know that. In that case, not sure it's worth it for the regular CI tests.

@DanielNoord
Copy link
Collaborator

You have yourself to thank for that > #5359 (comment)
😄

@cdce8p
Copy link
Member Author

cdce8p commented Feb 1, 2022

🤣

@Pierre-Sassoulas
Copy link
Member

Haha, Marc is doing so much for pylint that even himself can't keep track.

I think we had one occurrence of the pipeline taking longer than 6 hours (well we also had a lot of that when we were doing inefficient primer with duplicate-code checks...) and one occurrence where a MR was merged with green tests and then failed on main because another incompatible one was also merged at the same time. But we did not have to check every commits because we knew what was causing the problem as we don't merge that much MR daily. So I think we can save some runner time and some time waiting for job to finish before merging without much drawbacks.

DanielNoord added a commit that referenced this pull request Feb 22, 2022
Pierre-Sassoulas added a commit that referenced this pull request Feb 22, 2022
* Revert "Increase timeouts (#5755)"

This reverts commit ade493a.

Co-authored-by: Pierre Sassoulas <[email protected]>
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