always run all prototype tests in CI #6633

pmeier · 2022-09-22T14:47:02Z

In #6587 we decided to split the transforms tests into three CI steps. This makes it easier to find failing tests, but has one glaring downside that we missed: if the first step fails, the other steps aren't run at all. Imagine the CI is toasted for some external reason and now it won't run the tests your PR touches.

This PR refines the step triggers to run even if the previous tests failed.

This reverts commit b97d2fb.

This reverts commit 5bf250c.

.github/workflows/prototype-tests.yml

NicolasHug

No opinion from my side, accepting to unblock (let's just make sure this is fine with @YosuaMichael as well before merging)

As a side Q:

In #6587 we decided to split the transforms tests into three CI steps. This makes it easier to find failing tests

Is there a way to tell GA to report the failing tests separately, something like what CircleCI does with a dedicated "failing tests" tab ?

YosuaMichael · 2022-09-23T09:36:59Z

.github/workflows/prototype-tests.yml

        shell: bash
        run: pytest --durations=20 test/test_prototype_transforms*.py

      - name: Run prototype models tests
+        if: ${{ success() || steps.datasets.conclusion == 'failure' || steps.transforms.conclusion == 'failure' }}


@pmeier If I understand correctly, since we have:

if: ${{ success() || steps.datasets.conclusion == 'failure' }}

on the transforms test, therefore it will not be skipped and will have state of either success or failure.

In this case, I think we dont need steps.datasets.conclusion == 'failure' conditions here since it is kinda redundant. I might be wrong on this though, could you confirm this @pmeier ?

Just to make it clear, I think the following should be enough:

if: ${{ success() || steps.transforms.conclusion == 'failure' }}

therefore it will not be skipped and will have state of either success or failure.

failure is not concrete enough. It will not be skipped in case of failure of the datasets tests. If the failure happens in any other step, e.g. torchvision installation, the step will be skipped.

In this case, I think we dont need steps.datasets.conclusion == 'failure' conditions here since it is kinda redundant.

They are not redundant. If we leave them out, we are left with if: success() which is the default behavior if you don't define if: at all. Meaning, the step will only be run if all previous steps have succeeded.

We need this step to run if all setup steps (everything before the first test step) have succeeded, regardless of the state of the datasets tests. IMO success() || steps.datasets.conclusion == 'failure' is shortest expression that gets us this behavior, but if you can find a better one, I'm all ears.

NicolasHug · 2022-09-23T11:19:41Z

.github/workflows/prototype-tests.yml

        shell: bash
        run: pytest --durations=20 test/test_prototype_transforms*.py

      - name: Run prototype models tests
+        if: ${{ success() || steps.datasets.conclusion == 'failure' || steps.transforms.conclusion == 'failure' }}


They are not redundant. If we leave them out, we are left with if: success() which is the default behavior if you don't define if: at all. Meaning, the step will only be run if all previous steps have succeeded.

I don't think @YosuaMichael is suggesting to remove all XYZ == 'failure' checks. From what I understand I think he's suggesting the following change only at this specific line (which I agree seems like it should work):

Suggested change

if: ${{ success() || steps.datasets.conclusion == 'failure' || steps.transforms.conclusion == 'failure' }}

if: ${{ success() || steps.transforms.conclusion == 'failure' }}

In more general terms, step N only needs to check the status of step N - 1; it does not need to check the status of step N - 2 (which is what the PR is currently doing)

Although I guess that

success() || steps.transforms.conclusion == 'failure'

will evaluate to False if transforms passed but datasets failed...? In which case I agree we need the long form.

What if steps.datasets.conclusion == 'failure' and steps.transforms.conclusion == 'success'? success() will be false since we have at least one failure.

In this case success() || steps.transforms.conclusion == 'failure' boils down to false || false -> false and the step is not run.

With my version we get false || true || false -> true and the step will be run.

In general we need an expression that makes sure that the setup has succeeded. This is achieved by checking success() or failure of any of the steps afterwards.

Branching is a pain in CI in general, but I think with these three conditions it is somewhat tolerable. Given that this sparked quite a bit of confusion maybe we can refactor this a little. I see two options there:

Change the condition to if: steps.$LAST_SETUP_STEP.conclusion == 'success'.

Add a another dummy step after the setup that we can check for success

From these two I like 2. better since then we only need to remember to keep it as a last step of the setup but can do whatever we want before it.

I've implemented your suggestion in 89f8306: https://github.com/pytorch/vision/actions/runs/3112539248/jobs/5046070450 prototype tests are skipped although they should be run.

Sorry, was otw to the office just now. Yeah, what I meant is like what @NicolasHug suggest, and from @pmeier explanation I can understand why my suggestion doesn't work (I thought that success() only look at the last success, and this is wrong) , thanks for that.

I also saw your changes now using:

success() || ( failure() && steps.setup.conclusion == 'success' )

I think this is good (and avoid chaining)! Thanks for this!

pmeier · 2022-09-23T11:44:22Z

Is there a way to tell GA to report the failing tests separately, something like what CircleCI does with a dedicated "failing tests" tab ?

I wondered this myself a few months back and I found three possible options:

Unfortunately all of these options were not exactly what I was looking for. Either they had no real pytest support, i.e. the output looked quite weird for what we want, or they added quite a bit of noise that we also don't want.

I tried to implement my own solution until I realized that I can only use container actions when running on ubuntu. For cross-platform support the action needs to be written in JS, but my skills with that are not good enough to get to a usable state in my limited free time. If you want this, I would be happy to pick it up though during work 😇

This reverts commit 89f8306.

This reverts commit 1850e04.

This reverts commit 0617a41.

…to if-prototype-ci

pmeier · 2022-09-23T12:34:35Z

It seems if: steps.setup.conclusion == 'success' is not sufficient if there is an failure earlier even if the condition is otherwise true. Thus, I went with if: success() || ( failure() && steps.setup.conclusion == 'success' ) which seems to do the job.

A little more complex than I had hoped before, but avoid the condition chain for multiple steps and thus should still be easier than the initial proposal. LMK what you think.

pmeier · 2022-09-23T12:50:27Z

The current ongoing prototype datasets breakage illustrates the need for this PR pretty well: In #6627 the breakage in the datasets prevented the transforms tests from running. Thus we needed to manually remove the datasets tests from the CI config there to get a signal from the transforms tests.

In this PR, the breakage is still there, but all other tests are run as well. Thus, we can simply ignore the failing tests and move on.

Reviewed By: YosuaMichael Differential Revision: D39885423 fbshipit-source-id: 800f0a996f8aeb6bf02c53cceea231bedf8a7b56

pmeier added 3 commits September 22, 2022 16:45

always run all prototype tests in CI

5192336

[DEBUG] test logic

5bf250c

[SKIP CI] only CircleCI

f4449e1

facebook-github-bot added the cla signed label Sep 22, 2022

pmeier added 3 commits September 22, 2022 16:48

[SKIP CI] more debug

b97d2fb

Revert "[SKIP CI] more debug"

f0884b9

This reverts commit b97d2fb.

Revert "[DEBUG] test logic"

8452002

This reverts commit 5bf250c.

pmeier commented Sep 22, 2022

View reviewed changes

.github/workflows/prototype-tests.yml Outdated Show resolved Hide resolved

pmeier marked this pull request as ready for review September 22, 2022 14:54

pmeier requested review from vfdev-5 and YosuaMichael September 22, 2022 14:54

pmeier added module: ci code quality prototype labels Sep 22, 2022

NicolasHug approved these changes Sep 23, 2022

View reviewed changes

YosuaMichael reviewed Sep 23, 2022

View reviewed changes

NicolasHug reviewed Sep 23, 2022

View reviewed changes

pmeier added 12 commits September 23, 2022 13:48

[NEEDS REVERT] proof of concept

0617a41

[NEEDS REVERT] fix exit codes

1850e04

[NEEDS REVERT] fix exit codes ... again

89f8306

Revert "[NEEDS REVERT] fix exit codes ... again"

8ecdad0

This reverts commit 89f8306.

Revert "[NEEDS REVERT] fix exit codes"

5075038

This reverts commit 1850e04.

Revert "[NEEDS REVERT] proof of concept"

5626801

This reverts commit 0617a41.

simplify condition

7e86f2e

Merge branch 'main' into if-prototype-ci

7a9eb1a

[SKIP CI] debug

9be34bc

Merge branch 'if-prototype-ci' of https://github.com/pmeier/vision in…

ab8109e

…to if-prototype-ci

[SKIP CI] debug with failure

40c281c

[SKIP CI] debug other condition

6957e45

pmeier added 3 commits September 23, 2022 14:28

[SKIP CI] debug try without moustache

8a8961d

revert debug

515868b

readd test dependency

2cee66c

YosuaMichael approved these changes Sep 23, 2022

View reviewed changes

Merge branch 'main' into if-prototype-ci

c92c2a7

pmeier merged commit 784ee2b into pytorch:main Sep 23, 2022

pmeier deleted the if-prototype-ci branch September 23, 2022 12:54

facebook-github-bot pushed a commit that referenced this pull request Sep 29, 2022

[fbsync] always run all prototype tests in CI (#6633)

3720f83

Reviewed By: YosuaMichael Differential Revision: D39885423 fbshipit-source-id: 800f0a996f8aeb6bf02c53cceea231bedf8a7b56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

always run all prototype tests in CI #6633

always run all prototype tests in CI #6633

Uh oh!

pmeier commented Sep 22, 2022 •

edited

Loading

Uh oh!

Uh oh!

NicolasHug left a comment

Uh oh!

YosuaMichael Sep 23, 2022 •

edited

Loading

Uh oh!

pmeier Sep 23, 2022

Uh oh!

NicolasHug Sep 23, 2022 •

edited

Loading

Uh oh!

NicolasHug Sep 23, 2022

Uh oh!

pmeier Sep 23, 2022

Uh oh!

pmeier Sep 23, 2022

Uh oh!

YosuaMichael Sep 23, 2022 •

edited

Loading

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

pmeier commented Sep 23, 2022 •

edited

Loading

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

Uh oh!

	if: ${{ success() \|\| steps.datasets.conclusion == 'failure' \|\| steps.transforms.conclusion == 'failure' }}
	if: ${{ success() \|\| steps.transforms.conclusion == 'failure' }}

always run all prototype tests in CI #6633

always run all prototype tests in CI #6633

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmeier Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

pmeier Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

pmeier Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

pmeier commented Sep 23, 2022

Uh oh!

Uh oh!

pmeier commented Sep 22, 2022 •

edited

Loading

YosuaMichael Sep 23, 2022 •

edited

Loading

NicolasHug Sep 23, 2022 •

edited

Loading

YosuaMichael Sep 23, 2022 •

edited

Loading

pmeier commented Sep 23, 2022 •

edited

Loading