CI: Run inference tests only conditionally by BenjaminBossan · Pull Request #147 · skops-dev/skops

BenjaminBossan · 2022-09-21T10:14:33Z

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.

As discussed, only run the slow inference API tests on main or when the PR title contains the word "inference". As always, these things are hard to test.

For better differentiation from normal pytest.

Maybe we should change that...

BenjaminBossan · 2022-09-21T10:54:05Z

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

adrinjalali · 2022-09-26T12:01:04Z

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.sh on 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?

BenjaminBossan · 2022-09-27T12:54:16Z

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.sh on 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.

adrinjalali · 2022-09-29T10:27:15Z

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.

BenjaminBossan · 2022-09-29T12:19:12Z

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?

adrinjalali · 2022-09-29T12:24:54Z

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.

- Check commit message instead of PR title - Have one job to run inference tests only for one matrix set - Have one job to run inference tests for full matrix if commit message contains "inference"

adrinjalali · 2022-09-29T12:54:14Z

@@ -0,0 +1,48 @@
+# Tests hitting the inference API are slow and prone to hanging, resulting in
+# timeouts. Therefore, we only run inference tests on the full test matrix if
+# the head commit message contains the word "inference". See #118


maybe something like [CI inference] instead?

adrinjalali · 2022-09-29T12:55:48Z

+  cancel-in-progress: true
+
+jobs:
+  conditional-inference-pytest:


maybe inference-pytest-full and inference-pytest-partial for the other one?

adrinjalali · 2022-09-29T12:56:20Z

+  - pull_request
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}


we can also disable this when [CI inference] is in the commit message, since the other job does this as well.

- skip partial test if full test runs - wording: use "full" and "partial" instead of "conditional" and "unconditional"

BenjaminBossan · 2022-09-29T14:01:02Z

@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??

adrinjalali · 2022-09-29T14:18:17Z

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?

BenjaminBossan · 2022-09-29T15:36:51Z

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

Maybe expression issue?

adrinjalali · 2022-09-30T08:54:26Z

+  inference-pytest-partial:
+
+    runs-on: ${{ matrix.os }}
+    if: "(github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]'))"


shouldn't this be

Suggested change

if: "(github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]'))"

if: ${{ (github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]')) }}

ref: https://docs.github.com/en/actions/learn-github-actions/expressions#example-expression-in-an-if-conditional

I'll try that, fingers crossed. If this is really the correct syntax, does this line work correctly:

skops/.github/workflows/build-test.yml

Line 15 in 95a718a

if: "github.repository == 'skops-dev/skops'"

?

BenjaminBossan · 2022-09-30T10:14:58Z

@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.

link

adrinjalali · 2022-09-30T11:23:38Z

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.

BenjaminBossan · 2022-09-30T15:47:14Z

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...

adrinjalali · 2022-10-04T09:09:57Z

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

BenjaminBossan · 2022-10-04T09:40:32Z

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): BenjaminBossan/mops#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.

adrinjalali · 2022-10-04T10:06:08Z

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?

BenjaminBossan · 2022-10-04T10:53:40Z

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:

BenjaminBossan/mops#2

(latest 2 commits).

Any global repo settings that you changed? Otherwise, I'm ready to throw in the towel.

adrinjalali · 2022-10-04T11:37:39Z

No, just created a repo, and created a workflow in it, nothing special. This is really odd!

BenjaminBossan · 2022-10-18T12:29:01Z

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.

adrinjalali · 2022-11-09T16:58:15Z

potential related solution: adrinjalali/gh-action-test#1

Only if the commit message contains "[CI inference]" will those tests be run. Second attempt after skops-dev#147 failed, look there for more context.

adrinjalali · 2022-11-10T13:21:32Z

Fixed in #212

BenjaminBossan added 4 commits September 21, 2022 12:13

CI Run inference tests only conditionally

1c7604c

As discussed, only run the slow inference API tests on main or when the PR title contains the word "inference". As always, these things are hard to test.

Change name of inference workflow

5ab407f

For better differentiation from normal pytest.

Fix install for inference tests

e216ba1

Docs dependencies are needed to run tests

7fa4b76

Maybe we should change that...

BenjaminBossan changed the title ~~CI: Run inference tests only conditionally~~ CI: Run infe-rence tests only conditionally Sep 21, 2022

Empty commit to trigger CI

1d730cc

BenjaminBossan mentioned this pull request Sep 23, 2022

MNT Add matplotlib and pandas as test dependencies #151

Merged

Merge branch 'main' into CI-skip-inference-tests

b6a77af

BenjaminBossan added 2 commits September 29, 2022 14:43

Reviewer comments

ab3ccfe

- Check commit message instead of PR title - Have one job to run inference tests only for one matrix set - Have one job to run inference tests for full matrix if commit message contains "inference"

Rename job for better distinction

62a6f75

adrinjalali reviewed Sep 29, 2022

View reviewed changes

BenjaminBossan added 2 commits September 29, 2022 15:02

Reviewer comments

e58dbce

- skip partial test if full test runs - wording: use "full" and "partial" instead of "conditional" and "unconditional"

Empty commit to trigger [CI inference]

c584da9

adrinjalali approved these changes Sep 29, 2022

View reviewed changes

BenjaminBossan changed the title ~~CI: Run infe-rence tests only conditionally~~ CI: Run inference tests only conditionally Sep 29, 2022

BenjaminBossan added 2 commits September 29, 2022 16:25

Use different names for workflows

b7018a8

Empty commit to trigger [CI inference]

269053e

adrinjalali reviewed Sep 30, 2022

View reviewed changes

BenjaminBossan added 2 commits September 30, 2022 11:42

Change expression syntax

adc42c1

Empty commit to trigger [CI inference]

3b58d69

Empty commit that should not trigger CI [skip ci]

86b0c4d

BenjaminBossan mentioned this pull request Nov 10, 2022

CI Skip inference tests by default #212

Merged

adrinjalali closed this Nov 10, 2022

	if: "(github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]'))"
	if: ${{ (github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]')) }}

Conversation

BenjaminBossan commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenjaminBossan commented Sep 21, 2022

Uh oh!

adrinjalali commented Sep 26, 2022

Uh oh!

BenjaminBossan commented Sep 27, 2022

Uh oh!

adrinjalali commented Sep 29, 2022

Uh oh!

BenjaminBossan commented Sep 29, 2022

Uh oh!

adrinjalali commented Sep 29, 2022

Uh oh!

adrinjalali Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

adrinjalali Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

adrinjalali Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Sep 29, 2022

Uh oh!

adrinjalali commented Sep 29, 2022

Uh oh!

BenjaminBossan commented Sep 29, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Sep 30, 2022

Uh oh!

adrinjalali commented Sep 30, 2022

Uh oh!

BenjaminBossan commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Oct 4, 2022

Uh oh!

BenjaminBossan commented Oct 4, 2022

Uh oh!

adrinjalali commented Oct 4, 2022

Uh oh!

BenjaminBossan commented Oct 4, 2022

Uh oh!

adrinjalali commented Oct 4, 2022

Uh oh!

BenjaminBossan commented Oct 18, 2022

Uh oh!

adrinjalali commented Nov 9, 2022

Uh oh!

adrinjalali commented Nov 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BenjaminBossan commented Sep 21, 2022 •

edited

Loading

BenjaminBossan commented Sep 30, 2022 •

edited

Loading