Improve compute sanitizer workflow - #360
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
bdice
changed the title
bdice
changed the title
|
Tested here and it works as expected. |
misiugodfrey
left a comment
misiugodfrey
left a comment
There was a problem hiding this comment.
Looks good. Just a few minor comments for your consideration.
| needs: | ||
| - resolve-image | ||
| - discover-sanitizer-tests | ||
| if: ${{ !cancelled() }} |
There was a problem hiding this comment.
Does !cancelled() override the default behavior of "skip if dependent job failed"?
My understanding of the intent here is that we want to run this even if discover-sanitizer-tests was skipped (i.e. if tests to run were manually specified), but I don't think we want to run if it, or the resolve-image steps failed.
Perhaps something like: if: ${{ !cancelled() && (inputs.test_names != '' }} conveys the intend more explicitly?
There was a problem hiding this comment.
I will check into this.
| tool_name: "synccheck" | ||
| test_names: ${{ needs.discover-tests.outputs.tests }} | ||
| velox_image: ${{ needs.discover-tests.outputs.velox_image }} | ||
| gh workflow run velox-compute-sanitizer-run.yaml "${args[@]}" |
There was a problem hiding this comment.
IIUC this job will now be green as long as the sub-jobs have been dispatched - regardless of whether they pass or not. Is this ok with how we intend to view/report these results?
I don't think we want to leave the triggering job spinning as it waits for the other jobs to finish; I just want to make sure we aren't going to miss failures if they do occur by looking at the wrong reporting. If those jobs are hooked into slack or something then that's probably fine.
There was a problem hiding this comment.
Yeah, this succeeds if the trigger succeeds. I think that’s fine. We can’t spin-wait here because the actual runs take too long.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
There was a problem hiding this comment.
I don't think trigger-sanitizer needs the checkout action any more? Since it's just calling gh workflow run now, I think we can remove this action (and possibly the contents: read requirement).
There was a problem hiding this comment.
Probably so. I will check that.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
There was a problem hiding this comment.
I'm not familiar with workflow management with yaml in general. Claude flagged a minor issue. Not sure if it makes sense or not. Just fyi:
velox-compute-sanitizer-trigger.yaml:35 — the actions/checkout step is unnecessary. The job's only real step is gh workflow run …, which is a pure REST call (uses GH_TOKEN + the auto-set GITHUB_REPOSITORY); it never reads the working tree. The checkout — and the contents: read permission it forces at line 26 — is dead work that can be removed, leaving the job with just actions: write.
| type: string | ||
| required: false | ||
| default: '' | ||
| default: "" |
There was a problem hiding this comment.
Is there any difference between using single/double quotes in this yaml file? Or is this simply to maintain format consistency and use double quotes wherever possible?
3 participants
Summary
velox-compute-sanitizer-trigger.yamlinto a lightweight scheduler/manual dispatcher that starts separatevelox-compute-sanitizer-run.yamlworkflows formemcheck,racecheck, andsynccheck.velox-compute-sanitizer-run.yaml, matching the cuDF pattern where the run workflow owns discovery and execution.velox-latest-gpu-cuda13.1and copy the clearer GHCR image guidance into the manual dispatch inputs.Context
The previous
triggerworkflow was not just a trigger: it discovered tests and ran the full weekly sanitizer matrix through reusable workflow calls. The cuDF workflow uses a cleaner split where the trigger workflow schedules separate run workflows, and the run workflow handles discovery plus execution. This PR reshapes Velox in that direction while preserving its current weekly tool coverage.Validation
yq e . .github/workflows/velox-compute-sanitizer-trigger.yamlyq e . .github/workflows/velox-compute-sanitizer-run.yamlgit diff --checkactionlintwas not installed in this local environment.