CI Improvements for stability - #5908
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
…e serial tests when better isolation is created.
| CONDA_CHANNEL_LABEL: ${{ matrix.conda-version == 'canary' && 'conda-canary/label/dev::' || '' }} | ||
| CONDA_VERSION: ${{ contains('canary|release', matrix.conda-version) && 'conda' || format('conda={0}', matrix.conda-version) }} | ||
| PYTEST_MARKER: ${{ matrix.test-type == 'serial' && 'serial' || 'not serial' }} | ||
| # Serial tests run without xdist (-n auto) to avoid race conditions |
There was a problem hiding this comment.
| # Serial tests run without xdist (-n auto) to avoid race conditions | |
| # Serial tests run without xdist worker processes (-n auto) to avoid race conditions |
Is there a higher-level place that a non-repeated env can go?
There was a problem hiding this comment.
Sadly, it cannot. Matrix.test-type is on the job level.
beckermr
left a comment
There was a problem hiding this comment.
I am a strong -1 on this PR. My understanding of the original issue is that changes were made so that things that had been marked as serial no longer needed to be serial. Further the conda-build tests run fine after a few retries.
OTOH this pr is producing CI times in excess of 1.5 hours and the CI is still failing.
We need to go through each test and determine how it needs to be run, removing serial markers as much as possible. Then we can merge this PR for the remaining tests that actually need to be serial.
|
Indeed see the original issue text:
|
|
I feel that the code has a few issues with running in parallel. Thus moving the ones necessary to really run in serial will overall provide less restarts and thus saved time. There currently is a high chance of having to restart jobs due to parallelization. I would love to be able to run in parallel as well but I think we actually save time if we don't have to restart jobs. Note that the serial jobs have not failed, they are only the parallel ones. I also think we could move some of the bigger tests to another nightly thread so we don't have to build pytorch every single push BUT this PR was about reducing run failures. I think we should seek stability over speed. The claim here is restarting a failed jobs over and over again costs more than waiting for it to pass when run serially. |
|
There is also #5904, could we combine these PRs so they get merged at some point? |
|
We won't see real improvements here unless we spend the effort to understand the code and why things break. If we don't have time, that's fine, we can move on to other things. If we are going to put effort in, let's spend it on fixing bugs and understanding the code. |
Description
This PR seeks to reduce CI flakiness by fixing issue #5078 due to the fact that tests marked by serial were not running serially. This is done by using the py-test marker to run a slightly different pytest command. Also, marked a few more tests as serial during testing that were causing issues on parallel. Went more conservative thinking Stability > Speed.
Changes
Impact
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?