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

Build: cancel a build when a new one for the same branch/PR is triggered #8961

Closed
humitos opened this issue Feb 22, 2022 · 13 comments · Fixed by #9549
Closed

Build: cancel a build when a new one for the same branch/PR is triggered #8961

humitos opened this issue Feb 22, 2022 · 13 comments · Fixed by #9549
Assignees
Labels
Accepted Accepted issue on our roadmap Feature New feature Sprintable Small enough to sprint on

Comments

@humitos
Copy link
Member

humitos commented Feb 22, 2022

Now that we implemented a way to revoke Celery tasks we are able to cancel a running build. This is useful for people to manually cancel a build they are not interested in finishing for different reasons and free up the concurrency limit they have.

There are some scenarios where we can handle this automatically because we already know that the running build is useless and can be canceled:

  • a new commit was pushed to a branch
  • a build for the same branch was triggered (do not confuse with "Duplicated Build")
  • ...

Related #8850

@humitos humitos added Feature New feature Accepted Accepted issue on our roadmap labels Feb 22, 2022
@humitos humitos added the Sprintable Small enough to sprint on label Mar 2, 2022
@humitos humitos self-assigned this Mar 3, 2022
@humitos humitos moved this to Planned in 📍Roadmap Mar 29, 2022
humitos added a commit that referenced this issue Mar 29, 2022
When the exception is an exception we know it does not require communicating the
VCS service about its state, we avoid sending the build status to the VCS
service.

This is currently used to avoid sending a "failure" (red cross) notification
when the build is marked as duplicated. In the future, it could also be used
when the build is auto-cancelled because a new commit on the same branch was
done (see #8961)

Closes #9040
humitos added a commit that referenced this issue Mar 30, 2022
When the exception is an exception we know it does not require communicating the
VCS service about its state, we avoid sending the build status to the VCS
service.

This is currently used to avoid sending a "failure" (red cross) notification
when the build is marked as duplicated. In the future, it could also be used
when the build is auto-cancelled because a new commit on the same branch was
done (see #8961)

Closes #9040
@aiwalter
Copy link

that would be a really nice feature :)

@jbusecke
Copy link

This would help us over at https://github.com/xgcm/xgcm quite a bit. +1 👍

@astrojuanlu
Copy link
Contributor

I think the SunPy and Astropy folks would love having this too :) @nabobalis @pllim

@pllim
Copy link
Contributor

pllim commented Jul 18, 2022

Astropy folks

I cannot speak for all of Astropy but personally I would love auto-cancel. Otherwise, sometimes when we get commit spam, RTD gives us a long queue.

Thanks for the ping, @astrojuanlu !

@nabobalis
Copy link

Yeah from the SunPy side, we waste so much of your CI time (with how we commit) and implementing this would help.

I try to cancel builds manually but I don't keep on top of this.

@Zeitsperre
Copy link

Also chiming in that this would be a welcome addition for our PRs at https://github.com/Ouranosinc/xclim

humitos added a commit that referenced this issue Aug 25, 2022
This commit implements a simple logic to cancel old running builds when a new
build for the same project/version arrives:

1. look for running builds for the same project/version
2. if there are any, it cancels them all one by one via Celery's revoke method
3. trigger a new build for the current commit received

Note that this new feature is behind a feature flag (`CANCEL_OLD_BUILDS`) for
now so we can start testing it on some projects that have shown their interest
on this feature.

The current behavior for `DEDUPLICATE_BUILDS` will be replaced by this new logic
in the future. However, it was not removed in this commit since it's still
useful for projects that won't be using the new feature flag yet.

Closes #8961
@humitos
Copy link
Member Author

humitos commented Aug 25, 2022

I went ahead and opened #9549 as a starting point. Let's see if we can have it reviewed and merged soon to start testing this new feature.

Repository owner moved this from Planned to Done in 📍Roadmap Aug 29, 2022
@humitos
Copy link
Member Author

humitos commented Aug 29, 2022

I'm targeting this feature to go out tomorrow. Once deployed, I will enable this feature on the following projects:

  • xclim
  • sunpy
  • astropy
  • xgcm
  • poliastro

Let me know if there are other people subscribed to this issue that want to try this feature at this beta phase and I will enable it for their projects as well. Hopefully, after some feedback from you 🙏🏼 , we can enable it by default 👍🏼 . Thank you all!

@humitos
Copy link
Member Author

humitos commented Aug 30, 2022

This is currently deployed and all the projects listed in my previous comment have the feature enabled already. I quickly QAed this on production and it did work fine. Please, open an issue if you find something that's not working as you expected or contact support if you want to provide feedback to us! 💪🏼

@Zeitsperre
Copy link

It seems to be doing the job:

image

Thanks again for this!

@pllim
Copy link
Contributor

pllim commented Sep 16, 2022

Hello. I think it works but I keep forgetting to check. Can you please enable it for this also? This one has less contributors to keep track of and I think we push subsequent commits pretty frequently there, so it is easier for me to confirm. Thanks! 🙏

@humitos
Copy link
Member Author

humitos commented Sep 16, 2022

@pllim Thanks for your feedback. I enabled it on https://jdaviz.readthedocs.io/en/latest/ which I think is the Read the Docs project linked to that GH repository.

My main concern here is to not cancel valid builds. I mean, if we are not always cancelling a build that should be cancelled it's not a big problem. However, if we are cancelling a build that shouldn't be cancelled that's a problem.

In theory, this has been working fine since we enabled it. We haven't received any complain yet and the tests I've personally done did work. However, I suppose that projects doing many small pushes with one commit and multiple collaborators may have better feedback than myself.

Let me know if you think it works as you expected. Thanks again for your feedback.

BTW, we plan to enable it for all the projects in the following weeks if we don't receive any complain 😄

@pllim
Copy link
Contributor

pllim commented Oct 4, 2022

Haven't noticed any problems. Thanks again! 🙇‍♀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature Sprintable Small enough to sprint on
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants