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

Limit scheduling jobs to iree-org #19224

Merged
merged 6 commits into from
Dec 12, 2024
Merged

Conversation

marbre
Copy link
Member

@marbre marbre commented Nov 20, 2024

Limits the execution of cron-scheduled jobs to in iree-org, while allowing to trigger via workflow_dispatch also from forks.

@marbre marbre requested a review from ScottTodd as a code owner November 20, 2024 15:13
@@ -25,6 +25,7 @@ concurrency:

jobs:
setup:
if: ${{ github.repository_owner == 'iree-org' || github.event_name != 'schedule' }}
Copy link
Member

Choose a reason for hiding this comment

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

We could also change jobs like this to configure their runs-on based on the repository owner, so if the workflow does run on pull_request triggers, it has a chance of succeeding without needing to add self-hosted runners to the repo.

# runs-on: azure-linux-scale
runs-on: ${{ github.repository_owner == 'iree-org' && 'azure-linux-scale' ||  'ubuntu-20.04'  }}

(Same comment for all workflows that have pull_request triggers.... except Windows right now)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think would work for ci_linux_x64_clang_debug.yml and ci_linux_x64_clang_tsan.yml but not for ci_linux_arm64_clang.yml? Generally I agree and that it would be a great improvement, but do you think we should do it in addition or instead of disabling running on schedule?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the failure mode for requesting a label that is not available in the repository is pretty poor: the workflow just waits until it times out. I think extending this pull request to also handle at least those Linux jobs that have the pull_request trigger makes sense.

Could either:

A) Keep current behavior (so PRs on forks would wait until GitHub's timeout, and the scheduled runs would similarly fail)
B) Skip unless the event is workflow_dispatch (sorta weird, would unbreak pull_request and schedule triggers but keep workflow_dispatch broken unless someone goes through the effort of registering a self-hosted running with the same label(s))
C) Switch runner based on the github.repository_owner
D) Switch runner based on the github.repository_owner and also skip if the event is not workflow_dispatch
E) Switch runner based on the github.repository_owner and also skip if the event is schedule (so they would run on pull_request but be slowwww)

I like option D or E.

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched it for some of the jobs where it makes sense. The runner is changed to a GH-hosted one only for those job where a run would be triggered by a workflow dispatch.

@@ -21,6 +21,7 @@ concurrency:

jobs:
windows_x64_msvc:
if: ${{ github.repository_owner == 'iree-org' || github.event_name != 'schedule' }}
Copy link
Member

Choose a reason for hiding this comment

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

Here we could ideally also select the runs-on based on the repository[_owner], if this comment were addressed: #18967 (comment)

Ideally we could make this workflow more agnostic to the specific runner, so it can run in forks. One code pattern for that is https://github.com/openxla/stablehlo/blob/b1c1115f070493845ad92bd4a4461f77a2aa628d/.github/workflows/buildAndTestBazel.yml#L39

    runs-on: ${{ github.repository == 'openxla/stablehlo' && 'ubuntu-22.04-64core' ||  'ubuntu-22.04'  }}

Something to aim for later. All the build directory setup that is specific to these Azure runners will make that difficult and will limit workflow debugging to only developers with write access to this project.

A middle ground is possible where the runs-on field has a branch and the Create build dir step only runs if ${{ github.repository == or if ${{ runs-on == arc-runner-set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in particular the Windows build wont run on a GH-hosted runner (due to all the sccache and Azure stuff).

@marbre marbre force-pushed the limit-scheduled-jobs branch 2 times, most recently from 859ca9f to ddc54a4 Compare December 11, 2024 20:44
@marbre marbre requested a review from ScottTodd December 11, 2024 20:45
@marbre marbre force-pushed the limit-scheduled-jobs branch from ef59ef0 to f9ae84d Compare December 11, 2024 22:21
@marbre marbre merged commit d68cd28 into iree-org:main Dec 12, 2024
42 checks passed
@marbre marbre deleted the limit-scheduled-jobs branch December 12, 2024 09:38
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