Skip to content

Increase new primer timeout to 60m #7156

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

Merged
merged 1 commit into from
Jul 10, 2022

Conversation

jacobtylerwalls
Copy link
Member

See last run on main. The 3.7 job sometimes takes just barely over 45 minutes. The old primer timeout was set to 60m.

Clearly we need to add some concurrency. Rather than parallelizing the primer message comparison infrastructure, my hope is that we can just (later this year or next) solve the underlying issues with --jobs (see label topic: multiprocessing) and use that.

@jacobtylerwalls jacobtylerwalls added Maintenance Discussion or action around maintaining pylint or the dev workflow primer Skip news 🔇 This change does not require a changelog entry labels Jul 10, 2022
@Pierre-Sassoulas
Copy link
Member

We can't count on the github runner machine to have more than one core consistently, so we probably need both parallelization

@jacobtylerwalls
Copy link
Member Author

Oooh, that's a much larger ask then. I looked at that briefly a month ago and then realized there's no good solution for streaming JSON results to a file.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2645542754

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

Totals Coverage Status
Change from base Build 2644912543: 0.0%
Covered Lines: 16700
Relevant Lines: 17508

💛 - Coveralls

@jacobtylerwalls jacobtylerwalls merged commit 23a70e6 into pylint-dev:main Jul 10, 2022
@jacobtylerwalls jacobtylerwalls deleted the primer-timeout branch July 10, 2022 18:29
@DanielNoord
Copy link
Collaborator

DanielNoord commented Jul 10, 2022

We can't count on the github runner machine to have more than one core consistently, so we probably need both parallelization

I do think the "partial" cores the runner machines offer could be used if we created a more barebones way of parallelisation. Instead of creating a completely new PyLinter just pass a FileItem and some basic options and run check_file.

I really don't have time to look into this in the coming weeks due to deadlines and holidays, but I would certainly be interested in a (Discord/Zoom) call in August or September to discuss some possibilities. It seems this requires such large refactoring efforts that it would be good to both discuss the approach and perhaps even divide some tasks.

@jacobtylerwalls
Copy link
Member Author

My time is also limited these next few months but in general 👍🏻

@DanielNoord
Copy link
Collaborator

DanielNoord commented Jul 10, 2022

@jacobtylerwalls https://github.com/PyCQA/pylint/runs/7271165374?check_suite_focus=true

Am I overseeing something or is pip install -e . in the Run pylint primer step overwriting the install of astroid on the latest commit on main? 😅

Edit: Ah, this is funny. Because we haven't upped the constraint in pylint yet the commit on main doesn't satisfy the version requirement so we're downloading the latest version available on PyPI. This is also why all primer comments are messed up.

Anybody got a good solution?

@Pierre-Sassoulas
Copy link
Member

Should we relax the requirement even more ? I don't think a lot could go wrong (as long as we set it correctly before release). We can use tbump for that.

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 primer Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants