Restore old initial batch distribution logic in LoadScheduling #812

amezin · 2022-09-04T15:34:43Z

pytest orders tests for optimal sequential execution - i. e. avoiding
unnecessary setup and teardown of fixtures. So executing tests in consecutive
chunks is important for optimal performance.

Commit 09d79ac optimized test distribution for
the corner case, when the number of tests is less than 2 * number of nodes.
At the same time, it made initial test distribution worse for all other cases.
If some tests use some fixture, and these tests fit into the initial batch,
the fixture will be created min(n_tests, n_workers) times, no matter how many
other tests there are. With the old algorithm (before
09d79ac), if there are enough tests not using
the fixture, the fixture was created only once.

So restore the old behavior for typical cases where the number of tests is
much greater than the number of workers (or, strictly speaking, when there
are at least 2 tests for every node).

In my test suite, where fixtures create Docker containers, this change reduces
total run time by 10-15%.

This is a partial revert of commit 09d79ac

amezin · 2022-09-04T16:03:22Z

There will be a failing test because of changed scheduling, see #813

nicoddemus

Thanks @amezin!

Appreciate the PR, please take a look at my comments. 👍

nicoddemus · 2022-09-06T20:43:37Z

testing/test_dsession.py

@@ -115,18 +115,18 @@ def test_schedule_batch_size(self, pytester: pytest.Pytester) -> None:
        # assert not sched.tests_finished
        sent1 = node1.sent
        sent2 = node2.sent
-        assert sent1 == [0, 2]
-        assert sent2 == [1, 3]
+        assert sent1 == [0, 1]


This test was updated to match the new distribution method, could you please add a new test which checks that we are using round robin for smaller collections?

There is already such test, called test_schedule_fewer_than_two_tests_per_node. Also, the update of this test is simply part of the revert too - it was modified in 09d79ac, and I'm just undoing that change.

nicoddemus · 2022-09-06T20:44:46Z

src/xdist/scheduler/load.py

-        for i in range(initial_batch):
-            self._send_tests(next(nodes), 1)
+        if len(self.pending) < 2 * len(self.nodes):
+            # distribute tests round-robin


Can you please elaborate the comment to explain why we use round-robin for small collections?

Honestly, I don't know. To me, round-robin distribution it doesn't look optimal even for small collections. But I just want to stay within "trivial changes and reverts".

I see that 09d79ac improved the case with small number of tests because it distributed tests to all workers, not just first n / 2. But that doesn't mean you should do round-robin distribution. I'm just keeping the original code where it works better, trying to achieve the result with minimal changes.

But that doesn't mean you should do round-robin distribution.

Actually you got a point... I can't find the original PR with that change.

@RonnyPfannschmidt do you recall why that was change to use round robin?

Maybe it's just simpler to implement than adjusting "sequential" distribution code for the corner case. I've added a comment.

PR: #13 (commit 09d79ac has a link to it)

Thanks! I think one of the reasons is that you have a test module with very slow tests (disregarding fixtures), it is better to distribute that evenly across many workers rather than scheduling most/all of them a single worker.

I agree with the approach of keeping the changes to a minimal.

@nicoddemus Hm... Maybe I chose wrong condition for switching between "old" and "new" algorithms then.

check_schedule() has a heuristic for long-running tests. But it will have a chance to rebalance anything only if there are at least 3 tests per node.

Hmm I see. Perhaps let's change this condition to if len(self.pending) < 3 * len(self.nodes): then?

src/xdist/scheduler/load.py

changelog/812.feature

amezin · 2022-09-07T15:45:58Z

Not sure what's going on with the failed test/what to do about it

changelog/812.feature

nicoddemus

LGTM, thanks a lot @amezin!

nicoddemus · 2022-09-09T12:14:44Z

@RonnyPfannschmidt would you like to review too? Otherwise I will merge this in the next few days. 👍

RonnyPfannschmidt · 2022-09-09T12:42:55Z

Not sure if I get the bandwidth, it's a nice to have,

pytest orders tests for optimal sequential execution - i. e. avoiding unnecessary setup and teardown of fixtures. So executing tests in consecutive chunks is important for optimal performance. Commit 09d79ac optimized test distribution for the corner case, when the number of tests is less than 2 * number of nodes. At the same time, it made initial test distribution worse for all other cases. If some tests use some fixture, and these tests fit into the initial batch, the fixture will be created min(n_tests, n_workers) times, no matter how many other tests there are. With the old algorithm (before 09d79ac), if there are enough tests not using the fixture, the fixture was created only once. So restore the old behavior for typical cases where the number of tests is much greater than the number of workers (or, strictly speaking, when there are at least 2 tests for every node). In my test suite, where fixtures create Docker containers, this change reduces total run time by 10-15%. This is a partial revert of commit 09d79ac

4767: Update pytest-xdist requirement from ~=2.5.0 to ~=3.0.2 r=jenshnielsen a=dependabot[bot] Updates the requirements on [pytest-xdist](https://github.com/pytest-dev/pytest-xdist) to permit the latest version. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-xdist/blob/master/CHANGELOG.rst">pytest-xdist's changelog</a>.</em></p> <blockquote> <h1>pytest-xdist 3.0.2 (2022-10-25)</h1> <h2>Bug Fixes</h2> <ul> <li><code>[#813](pytest-dev/pytest-xdist#813) <https://github.com/pytest-dev/pytest-xdist/issues/813></code>_: Cancel shutdown when a crashed worker is restarted.</li> </ul> <h2>Deprecations</h2> <ul> <li> <p><code>[#825](pytest-dev/pytest-xdist#825) <https://github.com/pytest-dev/pytest-xdist/issues/825></code>_: The <code>--rsyncdir</code> command line argument and <code>rsyncdirs</code> config variable are deprecated.</p> <p>The rsync feature will be removed in pytest-xdist 4.0.</p> </li> <li> <p><code>[#826](pytest-dev/pytest-xdist#826) <https://github.com/pytest-dev/pytest-xdist/issues/826></code>_: The <code>--looponfail</code> command line argument and <code>looponfailroots</code> config variable are deprecated.</p> <p>The loop-on-fail feature will be removed in pytest-xdist 4.0.</p> </li> </ul> <h2>Improved Documentation</h2> <ul> <li> <p><code>[#791](pytest-dev/pytest-xdist#791) <https://github.com/pytest-dev/pytest-xdist/issues/791></code>_: Document the <code>pytest_xdist_auto_num_workers</code> hook.</p> </li> <li> <p><code>[#796](pytest-dev/pytest-xdist#796) <https://github.com/pytest-dev/pytest-xdist/issues/796></code>_: Added known limitations section to documentation.</p> </li> <li> <p><code>[#829](pytest-dev/pytest-xdist#829) <https://github.com/pytest-dev/pytest-xdist/issues/829></code>_: Document the <code>-n logical</code> option.</p> </li> </ul> <h2>Features</h2> <ul> <li> <p><code>[#792](pytest-dev/pytest-xdist#792) <https://github.com/pytest-dev/pytest-xdist/issues/792></code>_: The environment variable <code>PYTEST_XDIST_AUTO_NUM_WORKERS</code> can now be used to specify the default for <code>-n auto</code> and <code>-n logical</code>.</p> </li> <li> <p><code>[#812](pytest-dev/pytest-xdist#812) <https://github.com/pytest-dev/pytest-xdist/issues/812></code>_: Partially restore old initial batch distribution algorithm in <code>LoadScheduling</code>.</p> <p>pytest orders tests for optimal sequential execution - i. e. avoiding unnecessary setup and teardown of fixtures. So executing tests in consecutive chunks is important for optimal performance.</p> <p>In v1.14, initial test distribution in <code>LoadScheduling</code> was changed to round-robin, optimized for the corner case, when the number of tests is less than <code>2 * number of nodes</code>. At the same time, it became worse for all other cases.</p> <p>For example: if some tests use some "heavy" fixture, and these tests fit into the initial batch, with round-robin distribution the fixture will be created</p> </li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/eed37d4771f7379156303f63e0d09131f1ac8dee"><code>eed37d4</code></a> Update CHANGELOG</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/3e9284b6e0b474acaf6f25f8f8db68c78c9cd187"><code>3e9284b</code></a> Merge remote-tracking branch 'upstream/master' into release-3.0.0</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/ccdab727a29cef7075b853e550eb03aa8de076c1"><code>ccdab72</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-xdist/issues/831">#831</a> from pytest-dev/fix-setup</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/0c981d35475ab6eaee2e6a5ead44442bf07d6e61"><code>0c981d3</code></a> fix and update packaging</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/38dcf52ae69188b0249eb6307fe6745e2e4174dc"><code>38dcf52</code></a> Release 3.0.1</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/f2633f1b372dbc58a221cd612c9cb4e79f59eea3"><code>f2633f1</code></a> Fetch all tags, as required by setuptools-scm</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/192193197bee27228e1e4b44855b3777397e31e5"><code>1921931</code></a> Update CHANGELOG for 3.0</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/0f58a14def50b9b9112ec25cef97b9593ce86b8a"><code>0f58a14</code></a> Fix towncrier command for latest version</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/bd23c2460f00129fa0ace7151c8ae18c0a70d5ad"><code>bd23c24</code></a> Fix changelog fragments</li> <li><a href="https://github.com/pytest-dev/pytest-xdist/commit/794f28bcf4a3711acd074e9637ce8674fb6215be"><code>794f28b</code></a> Use modern syntax for towncrier</li> <li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-xdist/compare/v2.5.0...v3.0.2">compare view</a></li> </ul> </details> <br /> You can trigger a rebase of this PR by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [pytest-xdist](https://togithub.com/pytest-dev/pytest-xdist) ([changelog](https://pytest-xdist.readthedocs.io/en/latest/changelog.html)) | `==2.5.0` -> `==3.3.1` | [![age](https://badges.renovateapi.com/packages/pypi/pytest-xdist/3.3.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pytest-xdist/3.3.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pytest-xdist/3.3.1/compatibility-slim/2.5.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pytest-xdist/3.3.1/confidence-slim/2.5.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pytest-dev/pytest-xdist</summary> ### [`v3.3.1`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-331-2023-05-19) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v3.3.0...v3.3.1) \=============================== ## Bug Fixes - `#907 <https://github.com/pytest-dev/pytest-xdist/issues/907>`\_: Avoid remote calls during startup as `execnet` by default does not ensure remote affinity with the main thread and might accidentally schedule the pytest worker into a non-main thread, which breaks numerous frameworks, for example `asyncio`, `anyio`, `PyQt/PySide`, etc. A more safe correction will require thread affinity in `execnet` (`pytest-dev/execnet#96 <https://github.com/pytest-dev/execnet/issues/96>`\__). ### [`v3.3.0`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-330-2023-05-12) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v3.2.1...v3.3.0) \=============================== ## Features - `#555 <https://github.com/pytest-dev/pytest-xdist/issues/555>`\_: Improved progress output when collecting nodes to be less verbose. ### [`v3.2.1`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-321-2023-03-12) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v3.2.0...v3.2.1) \=============================== ## Bug Fixes - `#884 <https://github.com/pytest-dev/pytest-xdist/issues/884>`\_: Fixed hang in `worksteal` scheduler. ### [`v3.2.0`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-320-2023-02-07) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v3.1.0...v3.2.0) \=============================== ## Improved Documentation - `#863 <https://github.com/pytest-dev/pytest-xdist/issues/863>`\_: Document limitations for debugging due to standard I/O of workers not being forwarded. Also, mention remote debugging as a possible workaround. ## Features - `#855 <https://github.com/pytest-dev/pytest-xdist/issues/855>`\_: Users can now configure `load` scheduling precision using `--maxschedchunk` command line option. - `#858 <https://github.com/pytest-dev/pytest-xdist/issues/858>`*: New `worksteal` scheduler, based on the idea of `work stealing <https://en.wikipedia.org/wiki/Work_stealing>`*. It's similar to `load` scheduler, but it should handle tests with significantly differing duration better, and, at the same time, it should provide similar or better reuse of fixtures. ## Trivial Changes - `#870 <https://github.com/pytest-dev/pytest-xdist/issues/870>`\_: Make the tests pass even when `$PYTEST_XDIST_AUTO_NUM_WORKERS` is set. ### [`v3.1.0`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-310-2022-12-01) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v3.0.2...v3.1.0) \=============================== ## Features - `#789 <https://github.com/pytest-dev/pytest-xdist/issues/789>`\_: Users can now set a default distribution mode in their configuration file: .. code-block:: ini [pytest] addopts = --dist loadscope - `#842 <https://github.com/pytest-dev/pytest-xdist/issues/842>`\_: Python 3.11 is now officially supported. ## Removals - `#842 <https://github.com/pytest-dev/pytest-xdist/issues/842>`\_: Python 3.6 is no longer supported. ### [`v3.0.2`](https://togithub.com/pytest-dev/pytest-xdist/blob/HEAD/CHANGELOG.rst#pytest-xdist-302-2022-10-25) [Compare Source](https://togithub.com/pytest-dev/pytest-xdist/compare/v2.5.0...v3.0.2) \=============================== ## Bug Fixes - `#813 <https://github.com/pytest-dev/pytest-xdist/issues/813>`\_: Cancel shutdown when a crashed worker is restarted. ## Deprecations - `#825 <https://github.com/pytest-dev/pytest-xdist/issues/825>`\_: The `--rsyncdir` command line argument and `rsyncdirs` config variable are deprecated. The rsync feature will be removed in pytest-xdist 4.0. - `#826 <https://github.com/pytest-dev/pytest-xdist/issues/826>`\_: The `--looponfail` command line argument and `looponfailroots` config variable are deprecated. The loop-on-fail feature will be removed in pytest-xdist 4.0. ## Improved Documentation - `#791 <https://github.com/pytest-dev/pytest-xdist/issues/791>`\_: Document the `pytest_xdist_auto_num_workers` hook. - `#796 <https://github.com/pytest-dev/pytest-xdist/issues/796>`\_: Added known limitations section to documentation. - `#829 <https://github.com/pytest-dev/pytest-xdist/issues/829>`\_: Document the `-n logical` option. ## Features - `#792 <https://github.com/pytest-dev/pytest-xdist/issues/792>`\_: The environment variable `PYTEST_XDIST_AUTO_NUM_WORKERS` can now be used to specify the default for `-n auto` and `-n logical`. - `#812 <https://github.com/pytest-dev/pytest-xdist/issues/812>`\_: Partially restore old initial batch distribution algorithm in `LoadScheduling`. pytest orders tests for optimal sequential execution - i. e. avoiding unnecessary setup and teardown of fixtures. So executing tests in consecutive chunks is important for optimal performance. In v1.14, initial test distribution in `LoadScheduling` was changed to round-robin, optimized for the corner case, when the number of tests is less than `2 * number of nodes`. At the same time, it became worse for all other cases. For example: if some tests use some "heavy" fixture, and these tests fit into the initial batch, with round-robin distribution the fixture will be created `min(n_tests, n_workers)` times, no matter how many other tests there are. With the old algorithm (before v1.14), if there are enough tests not using the fixture, the fixture was created only once. So restore the old behavior for typical cases where the number of tests is much greater than the number of workers (or, strictly speaking, when there are at least 2 tests for every node). ## Removals - `#468 <https://github.com/pytest-dev/pytest-xdist/issues/468>`\_: The `--boxed` command-line option has been removed. If you still need this functionality, install `pytest-forked <https://pypi.org/project/pytest-forked>`\__ separately. ## Trivial Changes - `#468 <https://github.com/pytest-dev/pytest-xdist/issues/468>`\_: The `py` dependency has been dropped. - `#822 <https://github.com/pytest-dev/pytest-xdist/issues/822>`\_: Replace internal usage of `py.log` with a custom solution (but with the same interface). - `#823 <https://github.com/pytest-dev/pytest-xdist/issues/823>`\_: Remove usage of `py._pydir` as an rsync candidate. - `#824 <https://github.com/pytest-dev/pytest-xdist/issues/824>`\_: Replace internal usages of `py.path.local` by `pathlib.Path`. </details> --- ### Configuration 📅 **Schedule**: Branch creation - "every weekend" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/fluencelabs/spell).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

amezin force-pushed the loadsched-initial-batch branch from 6b6dabd to 562c6da Compare September 4, 2022 15:57

amezin marked this pull request as ready for review September 4, 2022 15:59

nicoddemus requested changes Sep 6, 2022

View reviewed changes

amezin force-pushed the loadsched-initial-batch branch from 19f99fd to e5967d2 Compare September 7, 2022 14:55

nicoddemus reviewed Sep 9, 2022

View reviewed changes

changelog/812.feature Outdated Show resolved Hide resolved

nicoddemus approved these changes Sep 9, 2022

View reviewed changes

amezin and others added 2 commits October 22, 2022 07:07

Apply suggestions from code review

fc2553e

nicoddemus force-pushed the loadsched-initial-batch branch from 6d08cb0 to fc2553e Compare October 22, 2022 10:07

nicoddemus merged commit 80130d5 into pytest-dev:master Oct 22, 2022

mikelane mentioned this pull request Oct 26, 2022

Bump pytest-xdist from 2.5.0 to 3.0.2 mikelane/reddit-get#83

Merged

mkoura mentioned this pull request Dec 10, 2022

New LoadScheduling batch distribution logic doesn't work for long-running tests #855

Closed

PrincipalsOffice mentioned this pull request Feb 15, 2023

What's the reason for ensuring 2 tests per node? #876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore old initial batch distribution logic in LoadScheduling #812

Restore old initial batch distribution logic in LoadScheduling #812

amezin commented Sep 4, 2022 •

edited

Loading

amezin commented Sep 4, 2022

nicoddemus left a comment

nicoddemus Sep 6, 2022

amezin Sep 7, 2022

nicoddemus Sep 6, 2022

amezin Sep 7, 2022

nicoddemus Sep 7, 2022

amezin Sep 7, 2022

amezin Sep 7, 2022

nicoddemus Sep 7, 2022

amezin Sep 9, 2022

nicoddemus Sep 12, 2022

amezin commented Sep 7, 2022

nicoddemus left a comment

nicoddemus commented Sep 9, 2022

RonnyPfannschmidt commented Sep 9, 2022

Restore old initial batch distribution logic in LoadScheduling #812

Restore old initial batch distribution logic in LoadScheduling #812

Conversation

amezin commented Sep 4, 2022 • edited Loading

amezin commented Sep 4, 2022

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amezin commented Sep 7, 2022

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Sep 9, 2022

RonnyPfannschmidt commented Sep 9, 2022

amezin commented Sep 4, 2022 •

edited

Loading