MNT: do not let one Python version cancel the other in the slow matrix - #1100
Conversation
|
This PR ran against itself, so here is the before and after on the same crash, an hour apart. #1098, on This branch, same crash on the same job: One more thing I had not noticed until I went looking at #1098 this morning. The API says |
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
the fail fast exists to save machine time.
however, we had never readly faced a limit on our github actions. So I'm ok with proceeding with this PR, let's make it
|
Heads up on both red marks here — neither is anything you did, and the fix is a rebase. The conflict. #1084 landed about forty minutes after this run started, and it already carries So half of this is already in, which is what made the PR go The CI failure. It is #1078, and the run predates the fix: That is the un-isolated Worth noting your own change did its job in that failing run: the other five legs ran to completion and reported real results instead of being cancelled. The PR that fixes cancellation got taken down by the very crash that motivated it, and only the crashing leg went red. One thing from my #1084 review that is relevant to you, since you have the measurements: the retry there keys on exit code 138 only, which is |
RocketPy-Team#1084 added fail-fast: false to the main test matrix while this was open, so the only half left is the slow one. Same reason: 3.10 failing says nothing about 3.14, so cancelling it costs a result and saves nothing worth having. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1bb8f23 to
806a0d7
Compare
RocketPy-Team#1084 retries the animation tests when they die on 138, which is SIGBUS on macOS. Counting the last 40 Tests runs, the crash was 139 eight times and 138 twice, so the common case fell straight through the retry. Linux SIGBUS is 135 rather than 138, so that missed as well. Also sets fail-fast: false on the slow matrix, which RocketPy-Team#1084 left out. 3.10 failing says nothing about 3.14, so cancelling it costs a result and saves nothing worth having. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Taken, thanks. Both in as You are right about the exit codes and it is worse than a rounding error. Counting the last 40 The condition is now a match against all three, in both workflow files: if [[ ! "$status" =~ ^(135|138|139)$ || "$attempt" == "$attempts" ]]; then
exit "$status"
fiI checked the branch rather than the regex by eye, since an anchor slip here would silently retry everything: I had dropped the So this is now three things: One thing worth flagging before this goes in: what you approved was a different patch. At the time it set |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1100 +/- ##
===========================================
+ Coverage 82.18% 82.53% +0.34%
===========================================
Files 122 128 +6
Lines 16355 16556 +201
===========================================
+ Hits 13441 13664 +223
+ Misses 2914 2892 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follows from #1078. Rescoped after #1084 landed, see below.
Pull request type
Current behavior
test-pytest-slow.yamlruns 3.10 and 3.14 as a matrix and sets nofail-fast, so the default oftrueapplies and the first version to fail cancels the other. The cancelled job then reports as a failure without having finished.New behavior
3.10 failing tells you nothing about 3.14, so cancelling it costs a result and saves nothing worth having.
Breaking change
Runner time on a failing slow run goes from "stops at the first failure" to "both finish". The slow workflow is not on pull requests, so this is not in anyone's review loop.
What changed since the approval
@Gui-FernandesBR approved this when it also set
fail-fast: falseon the mainPytestmatrix. #1084 merged in the meantime and added exactly that, so that half is now redundant and I have dropped it. What is left is three lines on the slow matrix.I also dropped the comment I had put above the main matrix setting. It referenced #1078 as a live problem, which #1084 has since fixed, and annotating someone else's line from a different PR seemed like the wrong place for it.
So this is smaller than what you looked at, and entirely a subset of it. Re-approving or closing are both reasonable; I would not want it merged on the strength of a review of the larger version.