[gh actions] run artifacts job always

[stas00]

I see that the recently added artifacts job won't run if the test job failed, which defeats the purpose. (example)

After some research it appears that adding if: always() may do the trick. Supposedly such a job should always be run regardless of the outcome of the previous jobs. Found it here, documented here.

Let's merge and see if it fixes the issue.

@LysandreJik, @sgugger, @sshleifer

[sshleifer]

Please check if it worked :)

[stas00]

Hmm, I guess it'd be have been more practical to experiment with the push job and not scheduled - but ok, let's wait - if it's not working, I will attack it on all fronts.

[stas00]

One more thing:

run_all_tests_torch_and_tf_gpu has 3 independent test suite runs and currently if one fails the others don't run! Which is not what is wanted I believe. I suggest that we add if: always() to the last 2 test suites as they are independent from the first one.

[sshleifer]

I'm fine with that as long as the workflow run can still be correctly marked as a failure.

To rephrase the requirement at the risk of redundancy, we want a red x next to the job when any test fails:

Screenshot shows that we are meeting this requirement at the moment.

[stas00]

Yes, that's the idea. I think the intention of the if condition is to define only whether a job is to be run, and not impact the total outcome. But we will see that already with the results of this PR - as artifact upload job will surely succeed. If the total outcome is [x] and artifacts have run, then we can replicate that condition to the other test suites.

[stas00]

OK, It did the trick, see: https://github.com/huggingface/transformers/actions/runs/334754818
Specifically, as requested: the final result is [x] and the artifact job did run regardless.

So we can apply this if: always condition to other pytest jobs on the same workflow. There is a nuance of a possibility of pre-pytest jobs failing and the pytest jobs running anyway with this condition, but if that situation arrives, it makes no difference - those jobs will just immediately fail.

Notes:

• It puts the artifact files in the same place from different jobs, so we need to call that artifact upload job differently for each job
• The so-so part is that the artifacts on github actions are provided as a single zipped file, so you have to first download the file, unpack it and only then you can see the results.
• Moreover it doesn't show the artifact file until all jobs have completed, despite saying that the file was successfully uploaded.

A Possible workaround:

One possible optimization here could be to cat reports/report_tests_failures.txt right after pytest, in a separate mini-step, so that you can immediately see just the failures and not wait for everything else to finish and go through the multiple steps to get to this file. (It has to be a separate step (name+run) not to affect the success/failure exit status of the pytest step.

Please review the outcome/my notes and let me know whether we proceed with this to other jobs.

Specifically to moving forward, we probably need to wait for this to be merged: #8007 as it has multiple changes to the CI files.

[sshleifer]

i have read your report. It is very clear, thank you. let's try a careful cat solution where we keep the size of the results as small as reasonably possible. one or two screens of text that show which tests failed (and short tracebacks (pytest --tb=short) ). Thanks for the help this is going to be so much easier to use than the status quo. Let me know if further clarifications/decisions would be helpful, and feel free to push back if implementation is difficult.

[stas00]

wrt/ proposed workaround:

Since the proposed quick cat is going to be in its own collapsible "tab" and will have only failures, let's start with just cat reports/report_tests_failures.txt and we can create other types of reports should it prove too verbose, and just cut those instead.

But also I could probably create reports/report_tests_failures_short.txt report which will emulate pytest --tb=short, so that we will have both long and short reports.

wrt/ the rest:

it still stands, correct? i.e. we still want the full artifacts in github actions

[sshleifer]

we still want the full artifacts in github actions

Yes, don't see any downside.

[stas00]

It looks like the errors are generated with either --tb=long or --tb=short at run time, so when the reports time comes they are already saved as one or the other, but not both.

So if we want the short and the long reports, one possibility is to generate the long report and then to try to make it shorter with some regex or some simple truncation - resulting in a short report.

Another approach that might work is collecting the failures as they happen - I need to investigate whether I can control the format in that hook or not without impacting the global reporting, as I'm sure that ideally we do want the full long report too. Please correct me if I'm wrong and --tb=short is sufficient for CIs (i.e. there will be no full long failures report anywhere - neither terminal logs nor report files), then it's easy.

[sshleifer]

I trust you to make those choices as you see fit. feel free to ignore tb=short.

[stas00]

I nailed it, got the cake and ate it too.