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: add cancel-in-progress to ci workflow #2013

Closed
wants to merge 1 commit into from

Conversation

afuetterer
Copy link
Contributor

When I added multiple commits to #1978 in quick succession, the current CI setup runs the tests for each commit individually, which leads to waiting time.

Desirable would be to cancel previous commits early and run the most recent one instead.

This is what concurrency.cancel-in-progress does.

Refs:

@freddyheppell
Copy link
Contributor

I don't think this will actually make it faster? It shouldn't wait for previous workflows on the PR to finish before it runs on a new head commit, they'll run in parallel. In fact, setting concurrency is what would cause it to wait. Also cancelling running workflows could be undesirable as if you were to break something in a series of rapid commits, you wouldn't be able to tell which one as the old commit workflows would be cancelled.

@afuetterer
Copy link
Contributor Author

As I said earlier, the commits got queued. They did definitely not run in parallel. So in my opinion it will get faster.
But sure, if you want to test each and every commit, please close this PR.
I would argue, that only the last commit should be tested.

@freddyheppell
Copy link
Contributor

If you're talking about the 3 latest commits on that PR, they definitely ran concurrently.

The job logs say the (Python 3.8, for example) jobs started at 06:58:30, 06:58:37 (+7s) and 06:58:45 (+8s). The runs take 10-11 minutes to finish, so the first had definitely not finished by the time the second and third started.

It may have appeared to be waiting, as acquiring a runner can take a few seconds (or possibly longer for a larger matrix job like this repo), but that will always happen every time a run starts.

I'm not a maintainer by the way, I'm just another contributor but I saw the title of this PR and was confused.

@afuetterer
Copy link
Contributor Author

Alright, if you checked the run start times of the CI jobs, that is settled then.

But I still think that not all of the commits need to run through CI, and early cancellation is a valid practice. This also saves compute time and resources.

Let @MaartenGr decide. I am more than willing to close this PR if the change is not desired.

@MaartenGr
Copy link
Owner

In general, I'm a big fan of saving resources where necessary. If you have a bunch of commits quickly after each other, then we indeed only need to test the very last commit. As long as all builds run (almost) simultaneously and not one after another, then it's fine by me.

@MaartenGr MaartenGr mentioned this pull request Jun 6, 2024
4 tasks
@afuetterer
Copy link
Contributor Author

If you agree, that testing only the last commit is fine, this PR makes sense. If you want to test all the commits as @freddyheppell suggests, I can close the PR. I don't think that is necessary though.

Your decision. You can close the PR as well, of course.

@MaartenGr
Copy link
Owner

@afuetterer This is tricky but let's close this for now as I want to prevent not knowing which commit might have broken the code which can make it hard to backtrack to the source of the issue. Thanks for your work on this and perhaps I'll re-open this later on if what I previously mentioned hardly ever happens...

@MaartenGr MaartenGr closed this Jun 25, 2024
@afuetterer
Copy link
Contributor Author

Sure, of course!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants