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

Run nightly Homebrew cron job only on the main repo, not on forks #4271

Merged

Conversation

rgommers
Copy link
Contributor

I noticed this because GitHub emailed me that it would disable the nightly job because it hadn't changed for 3 months. It currently takes 30-50 minutes daily, and by default runs on all forks of the main repository that have the relevant workflow yaml file. That serves little purpose and wastes quite a bit of energy - so disable the runs outside of the main repo.

This will not disable the runs on forks already made in the past that contain this workflow file, but it does save 3 months worth of runs on every new fork that is created.

[skip ci]

I noticed this because GitHub emailed me that it would disable the
nightly job because it hadn't changed for 3 months. It currently takes
30-50 minutes daily, and by default runs on all forks of the main
repository that have the relevant workflow yaml file. That serves little
purpose and wastes quite a bit of energy - so disable the runs outside
of the main repo.

This will not disable the runs on forks already made in the past that
contain this workflow file, but it does save 3 months worth of runs on
every new fork that is created.

[skip ci]
@martin-frbg
Copy link
Collaborator

Oops, that was rather excessive indeed. Thanks for fixing - I wonder if our other gh runners are similarly overzealous.

@martin-frbg martin-frbg merged commit 68906a9 into OpenMathLib:develop Oct 19, 2023
29 of 31 checks passed
@rgommers rgommers deleted the homebrew-nightly-on-main-repo branch October 19, 2023 11:34
@rgommers
Copy link
Contributor Author

I wonder if our other gh runners are similarly overzealous.

There are no other cron jobs, and pushing another branch to one's own fork doesn't trigger any jobs - unless you're updating your own develop branch. I just did that, and it triggers this:

image

Some of those are matrix builds - so that's quite a lot of builds. dynamic_arch.yml alone is 17 jobs:

image

It's usually beginning programmers that develop on the default branch rather than properly named branches. In NumPy/SciPy we add similar if: github.repository guards to all of our jobs. However, we have tons of inexperienced contributors there. I don't know if it's necessary for OpenBLAS - but if you think it's a good idea, I'd be happy to open a follow-up PR.

@martin-frbg
Copy link
Collaborator

I wonder if this happens everytime someone syncs their gh-hosted fork - even in preparation for creating a named branch for some future PR. (And I recently got some notifications that cirun jobs timed out after hours of waiting as there was no AWS worker configured for my personal account - probably worth looking into)

@rgommers
Copy link
Contributor Author

I wonder if this happens everytime someone syncs their gh-hosted for

Yes, I think it does indeed.

And I recently got some notifications that cirun jobs timed out

Those get triggered via Cirrus, and by default those trigger, unless:

  • the commit message contains [skip ci]
  • the "Require approval for builds from users without write permissions" in the repository settings in the Cirrus UI is used.
  • there's some custom logic to pre-process CI triggers (e.g., like this for numpy)

I don't think any of those apply to the OpenBLAS repo at the moment? There's also the cost associated with running out of credits - not sure if you're worried about that. I've got a fair bit of experience with this now. Happy to sync some of the customizations from numpy/scipy to this repo if you think that's useful.

@martin-frbg
Copy link
Collaborator

Not sure I understand how Cirrus specifically would trigger a Cirun-based job (except that one recent occurence was from me updating cirrus.yml in a PR). Suspect it might be bc the workflow file for graviton also lacks an "if repository is the main one" conditional ? Certainly no harm done unless I manage to DDOS Cirun with spurious requests...
(And yes I may have to remove some of the expensive Mac jobs from Cirrus - for now I've fed them some pocket money after we ran out of free credits yesterday)

rgommers added a commit to rgommers/OpenBLAS that referenced this pull request Nov 10, 2023
This is a follow-up to OpenMathLibgh-4271. At the moment, when a contributor
pushes the latest `develop` to their own branch to bring their own
fork in sync with `main`, or if they push another branch, this triggers
30 CI jobs to run. Most will complete silently and only burn CPU
time unnecessarily. If there's a failure, this may result in unexpected
failure notifications. And the AWS Graviton3 run won't complete at all
and time out, since the Cirun hook will only work when triggered from
the main repo.
rgommers added a commit to rgommers/OpenBLAS that referenced this pull request Nov 10, 2023
This is a follow-up to OpenMathLibgh-4271. At the moment, when a contributor
pushes the latest `develop` to their own branch to bring their own
fork in sync with `main`, or if they push another branch, this triggers
30 CI jobs to run. Most will complete silently and only burn CPU
time unnecessarily. If there's a failure, this may result in unexpected
failure notifications. And the AWS Graviton3 run won't complete at all
and time out, since the Cirun hook will only work when triggered from
the main repo.
@rgommers
Copy link
Contributor Author

Followed up on this now in gh-4305, sorry that took a while.

Not sure I understand how Cirrus specifically would trigger a Cirun-based job

My bad, I confused this with the original plan about triggering via Cirrus - that's not the case, it triggers via the GitHub Actions app.

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.

2 participants