skops
skops copied to clipboard
skops-dev
CI: Run inference tests only conditionally
As discussed here, only run the slow inference API tests on main or when the PR title contains the word "inference".
Addresses (though not really solves) #118.
As always, these things are hard to test.
To test the changes, I first created a PR containing the word "inference" and indeed all tests ran:
After that, I renamed the title to use "infe-rence" instead. Rerunning the CI didn't change anything (presumably the checks are not performed), so I had to push an empty commit. After that, indeed the inference tests were skipped:
This shows that we can now indeed skip the inference tests conditional on the PR title. I chose the PR title instead of commit message because the latter is harder to change after being pushed.
What I cannot test for obvious reasons is if the inference tests run indeed on the main branch, for this we would have to merge this PR.
Ready for review @skops-dev/maintainers
I would go with commit message rather than PR title. For some commits we might want to test and for some we might skip. If we forget the tag in a commit, we can add an empty commit with the tag.
But generally, I think this alternate solution might be better:
- have a crone job which pushes to the hub repos like the ones generated by
generate.py/run.shon a daily basis. - all inference tests would target those repos instead of creating a repo and testing immediately after.
We can even pin those repos to make sure they're always up and running for tests to run quite fast.
WDYT?
I would go with commit message rather than PR title. For some commits we might want to test and for some we might skip. If we forget the tag in a commit, we can add an empty commit with the tag.
Honestly, I don't think this comes up very often? And you can still change the PR title for that specific commit? This seems to be more flexible to me.
But generally, I think this alternate solution might be better:
- have a crone job which pushes to the hub repos like the ones generated by
generate.py/run.shon a daily basis.- all inference tests would target those repos instead of creating a repo and testing immediately after.
Not sure if I understand 100%. Those repos would presumably be created based on the main branch? If so, if there is a skops change in the PR that breaks stuff already at the repo creation stage (say, a change in the metainfo), the CI wouldn't catch that on the PR. Or do I misunderstand?
Regardless of that, for the time being, we could still go with this PR and then remove the inference GH action once we have added said script.
We can even pin those repos to make sure they're always up and running for tests to run quite fast.
I didn't know about that possibility.
Ok, so what about this:
- make inference tests run only on one CI (linux+latest python?)
- add a commit tag which would trigger inference tests on all CI
This would also fix our codecov issue.
I prefer commit message tags since they are more of a norm than PR title triggers, and I think it's more intuitive to have CI triggers on commits and their messages rather than on commits + whatever's on the PR title. At least in sklearn we have quite a few of those CI triggers.
Okay, then let's proceed like this.
What is the "right" way to get the commit message? I found github.event.head_commit.message but couldn't find any use in sklearn, so I assume there is another way?
in sklearn we don't do these in gh actions, instead we do them in shell, and there we use git.
I'm impartial to whether doing it through GH action stuff or in shell.
@adrinjalali So it seems not to work correctly. First try, inference-pytest-full was skipped and inference-pytest-partial ran, as expected:
Second try, after an empty commit containing "[CI inference]", inference-pytest-partial ran (why?) and inference-pytest-full is not listed at all??
Your two new workflows have the same name @BenjaminBossan , I wouldn't be surprised if that's the root of the issue. Maybe try two different names?
Your two new workflows have the same name
Good catch, I tried to have different names everywhere but missed the top level name :/
Unfortunately, this still doesn't work. First, without the name match, everything looks as expected:
But with the name match, full is still skipped and partial runs (ignore the other failing tests for now):
For reference, I used the logic described here to check the commit message (but also confirmed in other sources).
@adrinjalali Still the same issue, "full" never runs and "partial" always runs. So nothing changed, which is not too surprising, as
When you use expressions in an if conditional, you may omit the expression syntax (${{ }}) because GitHub automatically evaluates the if conditional as an expression.
At this point I'd suggest creating a new repo on GH and testing things there maybe? Just in case other things are interacting in a weird way with one another.
At this point I'd suggest creating a new repo on GH and testing things there maybe? Just in case other things are interacting in a weird way with one another.
I did a bunch of testing. I could make "full" run by not matching on [CI inference] but CI inference -- I assume that brackets require some kind of escaping, but I didn't bother to find out how exactly, I think this would be good enough.
However, I could not figure out how not to make partial run. For reference, here is the original conditional expression:
if ${{ (github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]')) }}
Things I tried:
- Not using bracktets
- Not using upper case
- Move
!before parens - Remove parens
- Swap position with
(github.repository == 'skops-dev/skops') - Match on a completely different word without whitespace
- Only do the
!contains(...)condition, not checking repository
None of these helped, it was always triggered. The only way to skip it was to do if ${{ false }}.
I found a blog post that uses an expression with ! + contains, which I copied verbatim and still no luck. I have no more ideas what else to test...
I tried in a new repo, and things seem to work as expected: https://github.com/adrinjalali/gh-action-test/tree/main
The last two commits show how it'd work.
Also, could this be of some help?
https://github.com/nektos/act
I tried in a new repo, and things seem to work as expected
I invited you to my repo. It still doesn't work there, even though I copy-pasted your expression (with just the repo name changed): https://github.com/BenjaminBossan/mops/pull/2
The whole setup is a bit closer than your example to what we have in skops, but those differences should theoretically not affect this expression.
Maybe merge the two workflows under the same one, just two different jobs?
Also, you could start with a minimal setup like the one I have in that repo, and see at what point things break maybe?
Maybe merge the two workflows under the same one, just two different jobs?
I copied your exact workflow (only changed repo name) and still the exact same issue:
https://github.com/BenjaminBossan/mops/pull/2
(latest 2 commits).
Any global repo settings that you changed? Otherwise, I'm ready to throw in the towel.
No, just created a repo, and created a workflow in it, nothing special. This is really odd!
Okay, so where do we go from here? To recap, none of the suggested methods seem to work for getting the commit message for "pull_request" triggers.
potential related solution: https://github.com/adrinjalali/gh-action-test/pull/1