CI First step towards moving Azure CI to GHA #31832
Conversation
|
So the first thing is that the required check for "Run unit tests" is pending because this check does not exist any more in this PR. I propose that we removed this required check to be able to merge this PR, setting is here. Required checks are set on a job level, medium term I think we would need a job that aggregates all matrix entries results so that we can make the required checks on this single aggregating job. See https://github.com/orgs/community/discussions/4324#discussioncomment-13335158 or there is the GitHub action https://github.com/re-actors/alls-green that seems to handle this use case. |
|
Why is the global "Run unit tests" step stuck with "Expected — Waiting for status to be reported" while the "Lint" and "pymin_conda_forge_arm (pull_request)" are reported as successful? |
|
Nice to see that the ARM runner is so fast even without ccache enabled. |
|
We should rename the workflows with a convention that makes the operating system visible in the report UI: 
So far we only run the arm tests on linux, but this could change in the future. |
The required check is based on a job status named "Run unit tests", with this PR the equivalent Linux arm job is named differently (e.g. "Linux pymin_conda_forge_arm" in the last commit). The required check is waiting for a job status named "Run unit tests" that will never come. See my earlier comment #31832 (comment) where I tried to explain in more details. |
|
Sorry for not having read your previous comment... Let me finalize my review. |
| activate-environment: "" | ||
|
|
||
| - name: Build scikit-learn | ||
| run: bash -l build_tools/azure/install.sh |
There was a problem hiding this comment.
I suppose we can rename the "azure" folder once the migration is complete but it's fine to keep it unchanged during the transition.
There was a problem hiding this comment.
Yes that was my plan indeed to move the script once the full migration to GHA is finished.
| if build_source_version_message is None: | ||
| # We are not on Azure: behaviour based on commit-message is not | ||
| # supported for now. | ||
| # TODO: this should be implemented at one point for GHA. |
There was a problem hiding this comment.
I asked perplexity about this and it suggested this:
https://www.perplexity.ai/search/how-to-condition-the-execution-4hfi.cTjT9.uI60Jb2iJtg
name: Conditional Job on PR Commit Message
on:
pull_request:
types: [opened, synchronize]
jobs:
extract_commit_message:
runs-on: ubuntu-latest
outputs:
matched: ${{ steps.match.outputs.matched }}
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 2 # fetch enough history
- name: Get latest commit message
id: commit_msg
run: |
msg="$(git log -1 --pretty=format:'%s')"
echo "message=$msg" >> $GITHUB_OUTPUT
- name: Evaluate message contents
id: match
run: |
if [[ "${{ steps.commit_msg.outputs.message }}" == *"[trigger-job]"* ]]; then
echo "matched=true" >> $GITHUB_OUTPUT
else
echo "matched=false" >> $GITHUB_OUTPUT
fi
conditional_job:
needs: extract_commit_message
if: needs.extract_commit_message.outputs.matched == 'true'
runs-on: ubuntu-latest
steps:
- run: echo "Job is executing because the commit message contains [trigger-job]"
Note that the above does not explicitly sanitize the contents of the commit message before injecting it into the $GITHUB_OUTPUT but I don't know if any sanitization is needed or not in this case but it seems that GITHUB_OUTPUT output is not meant to store multiple line payloads.
Maybe a better way to do it would be to write a single Python or bash script that is both in charge of looking up the git commit message and then matching it with a and only printing "true" or "false" to avoid having to store the unsanitized commit message into $GITHUB_OUTPUT as an intermediate step.
There was a problem hiding this comment.
I tried at one point to have something that worked for both Azure and GHA and realized this was more complicated than I hoped. I thought it was better to first have the basic structure in place and tackle each problem separately in further PRs.
Note that at the moment, we have already a commit-based trigger in GHA for [cd build] in wheels.yml, that seems somewhat similar to your suggestion.
|
@lesteve merged! |
install.shandtest_script.shPlenty of stuff left for the future:
[float32]