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

CI Linux incremental: Set max_parallel = 8, reduce standard-sitepackages platforms #36697

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Nov 10, 2023

To reduce the impact of the PRs that run CI Linux incremental on the GH Actions queue, in particular after a release (for example: https://github.com/sagemath/sage/actions/runs/6827962772)

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please try it first with my initial suggestion, before we loose ourself in optimizing to find a local minima.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2023

This change here is for the incremental build on PRs that make changes to packages.

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Nov 11, 2023

Yes, I can read. But it also changes the load on the github actions so comparison with previous releases will get harder.

For this reason, I propose the following strategy:

  • Wait until next week so that your fine-tuning PR gets merged.
  • For the week after, merge my original PR reducing the max-parallel for the ci-linux runs.
  • Maybe give it one more week (+ PR) for fine tuning
  • Make a decision which approach worked the best
  • Implement further improvements such as this PR (if still needed/adapted to the chosen solution)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2023

  • For the week after, merge my original PR reducing the max-parallel for the ci-linux runs.

As I just wrote over in #36610, you can just run that in your own repo and report back.

…uce platforms tested with standard-sitepackages
@mkoeppe mkoeppe force-pushed the ci_linux_incremental_less_parallel branch from 05491bb to 5ac6d13 Compare November 13, 2023 00:25
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2023

@orlitzky Objections or other comments on reducing the tested sitepackages platforms?

@orlitzky
Copy link
Contributor

@orlitzky Objections or other comments on reducing the tested sitepackages platforms?

I agree. Having them fail repeatedly doesn't make sense without someone actively working on them.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 13, 2023

For this reason, I propose the following strategy:

  • Wait until next week so that your fine-tuning PR get merged.

Let's wait until the fine-tuning PR and also this PR gets merged.

  • For the week after, merge my original PR reducing the max-parallel for the ci-linux runs.

Using ~4 hours until "default" is built to finish as many "standard" jobs as possible is an objective rather than a means. If the problem (PR jobs gets no chance) persists after the ~4 hours, then, I think, your PR should focus on improving the situation after the ~4 hours.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2023

Thanks!

Copy link

Documentation preview for this PR (built with commit 5ac6d13; changes) is ready! 🎉

@tobiasdiez
Copy link
Contributor

I'm a big fan of the content of this PR, but as I've explained above I think its to early to merge it.

  • For the week after, merge my original PR reducing the max-parallel for the ci-linux runs.

Using ~4 hours until "default" is built to finish as many "standard" jobs as possible is an objective rather than a means.

Please don't rewrite the history. The purpose of #36610 is clearly stated "Currently, the ci-linux workflow runs a lot of parallel runs which prevents other workflows (from PR) to be executed. We add a few more max_parallel statements to spread the stress on the ci over a longer time." (i.e improve the execution time for PR workflows). In #36616 (comment) you reviewed Matthias PR with the words "Both of you proposed solutions of the same problem." I don't know why it's so hard to admit that Matthias' idea, while being a good one, completely failed to improve the situation for PR workflows. But anyway, this is not very relevant to this PR.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2023

Note that this PR was not set to "blocker", so it won't merged until the 10.3 release cycle anyway.
So I'm restoring "positive review".

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 13, 2023

I'm a big fan of the content of this PR, but as I've explained above I think its to early to merge it.

  • For the week after, merge my original PR reducing the max-parallel for the ci-linux runs.

Using ~4 hours until "default" is built to finish as many "standard" jobs as possible is an objective rather than a means.

The purpose of #36610 is clearly stated "Currently, the ci-linux workflow runs a lot of parallel runs which prevents other workflows (from PR) to be executed. We add a few more max_parallel statements to spread the stress on the ci over a longer time." (i.e improve the execution time for PR workflows).

Yes. I understood that.

In #36616 (comment) you reviewed Matthias PR with the words "Both of you proposed solutions of the same problem."

Yes. But as I said in #36616 (comment), I realized that Matthias has the objective (finish early as many "standard" jobs as possible) as well as "the same problem", but you are not sharing this.

I don't know why it's so hard to admit that Matthias' idea, while being a good one, completely failed to improve the situation for PR workflows.

He is still fine-tuning his idea. Why it's so hard to wait for himself to admit his ultimate failure?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2023

Let's move forward here.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2023

I see no meaningful objection here, so let's merge this.

@tobiasdiez
Copy link
Contributor

I see no meaningful objection here, so let's merge this.

I don't think this is how it's supposed to work.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2023

@tobiasdiez Here on this PR you have not even raised a reviewer concern. It has been positively reviewed. You are just demanding that something else is merged first -- the PR #36610, which does not have any support.

But when something is merged is up to the release manager to decide.

@tobiasdiez
Copy link
Contributor

Still to early in my opinion. I would be fine with setting it to "pending" and "positive-review" (as a signal to the release manager that it should not yet be merged), but in the absence of such a mechanism I'll remove the positive review label again.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 7, 2023

All of what is written in #36697 (comment) is moot, and you are not giving any reason.

@tobiasdiez
Copy link
Contributor

All of what is written in #36697 (comment) is moot, and you are not giving any reason.

I do, just because you don't agree with them they are not "moot".

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2023

I do

Where?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2023

just because you don't agree with them they are not "moot".

That's correct! That's not the mechanism!

They are moot because #36697 (comment) refers to timing some experiments in weeks that have passed, and relative to your PR #36610, which does not have any support.

@vbraun vbraun merged commit c451382 into sagemath:develop Dec 10, 2023
37 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 10, 2023
@mkoeppe mkoeppe deleted the ci_linux_incremental_less_parallel branch December 10, 2023 17:17
@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: scripts disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ p: critical / 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants