Repository: pytest-dev/pytest-xdist
Pull request #646 (2021-03-25 18:45:32)

Add explicit 'logical' choice to --numprocesses flag
With the change to physical CPU count in #560, test sets that are run with `psutil` installed and that are not CPU-bound (in my case, with lots of network activity) take roughly twice as long to execute on hyperthreaded CPUs. Add a new choice `logical` for flag `--numprocesses` for use cases like this.

This implementation is a bit weird because of the multiple fallback paths in `pytest_xdist_auto_num_workers()` which all return the logical CPU count already, but I don't see a better option.

Testing:
- Ran `tox` against all but `python3.5` on Linux:
```
$ python3.9 -m tox -p
...
✔ OK linting in 5.201 seconds
✔ OK py38-psutil in 7.751 seconds
✔ OK py39-pytestlatest in 39.017 seconds
✔ OK py38-pytestlatest in 39.792 seconds
✔ OK py38-pytestmain in 42.884 seconds
✔ OK py36-pytestlatest in 48.754 seconds
__________________________ summary __________________________ 
  linting: commands succeeded
  py36-pytestlatest: commands succeeded
  py38-pytestlatest: commands succeeded
  py39-pytestlatest: commands succeeded
  py38-pytestmain: commands succeeded
  py38-psutil: commands succeeded
  congratulations :)
```
- Ran a set of tests against this change and saw 12 workers when run with `--numprocesses logical` on my six-core hyperthreaded CPU.


Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index c9eb227..8801b79 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -11,15 +11,19 @@ jobs:
       fail-fast: false
       matrix:
         tox_env:
+          - "py35-pytestlatest"
           - "py36-pytestlatest"
           - "py37-pytestlatest"
           - "py38-pytestlatest"
           - "py39-pytestlatest"
           - "py38-pytestmain"
           - "py38-psutil"
+          - "linting"
 
         os: [ubuntu-latest, windows-latest]
         include:
+          - tox_env: "py35-pytestlatest"
+            python: "3.5"
           - tox_env: "py36-pytestlatest"
             python: "3.6"
           - tox_env: "py37-pytestlatest"
@@ -32,6 +36,8 @@ jobs:
             python: "3.8"
           - tox_env: "py38-psutil"
             python: "3.8"
+          - tox_env: "linting"
+            python: "3.7"
 
     steps:
     - uses: actions/checkout@v1
@@ -47,13 +53,31 @@ jobs:
       run: |
         tox -e ${{ matrix.tox_env }}
 
+  linting:
+
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v1
+    - name: Set up Python
+      uses: actions/setup-python@v1
+      with:
+        python-version: "3.7"
+    - name: Install tox
+      run: |
+        python -m pip install --upgrade pip
+        pip install tox
+    - name: Linting
+      run: |
+        tox -e linting
+
   deploy:
 
     if: github.event_name == 'push' && startsWith(github.event.ref, 'refs/tags')
 
     runs-on: ubuntu-latest
 
-    needs: build
+    needs: [build, linting]
 
     steps:
     - uses: actions/checkout@v1
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 2b65e8a..82fc078 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,6 +1,6 @@
 repos:
--   repo: https://github.com/psf/black
-    rev: 21.4b0
+-   repo: https://github.com/ambv/black
+    rev: 20.8b1
     hooks:
     -   id: black
         args: [--safe, --quiet, --target-version, py35]
@@ -12,12 +12,12 @@ repos:
     -   id: end-of-file-fixer
     -   id: check-yaml
     -   id: debug-statements
--   repo: https://github.com/PyCQA/flake8
-    rev: 3.9.1
+-   repo: https://gitlab.com/pycqa/flake8
+    rev: 3.9.0
     hooks:
     -   id: flake8
 -   repo: https://github.com/asottile/pyupgrade
-    rev: v2.13.0
+    rev: v2.10.1
     hooks:
     -   id: pyupgrade
         args: [--py3-plus]
diff --git a/README.rst b/README.rst
index da5daaf..ae3097b 100644
--- a/README.rst
+++ b/README.rst
@@ -149,16 +149,16 @@ where executing a high-scope fixture exactly once is important.
 Running tests in a Python subprocess
 ------------------------------------
 
-To instantiate a python3.9 subprocess and send tests to it, you may type::
+To instantiate a python3.5 subprocess and send tests to it, you may type::
 
-    pytest -d --tx popen//python=python3.9
+    pytest -d --tx popen//python=python3.5
 
-This will start a subprocess which is run with the ``python3.9``
+This will start a subprocess which is run with the ``python3.5``
 Python interpreter, found in your system binary lookup path.
 
 If you prefix the --tx option value like this::
 
-    --tx 3*popen//python=python3.9
+    --tx 3*popen//python=python3.5
 
 then three subprocesses would be created and tests
 will be load-balanced across these three processes.
@@ -371,7 +371,7 @@ You can also add default environments like this:
 .. code-block:: ini
 
     [pytest]
-    addopts = --tx ssh=myhost//python=python3.9 --tx ssh=myhost//python=python3.6
+    addopts = --tx ssh=myhost//python=python3.5 --tx ssh=myhost//python=python3.6
 
 and then just type::
 
diff --git a/changelog/646.feature.rst b/changelog/646.feature.rst
new file mode 100644
index 0000000..210c73e
--- /dev/null
+++ b/changelog/646.feature.rst
@@ -0,0 +1,3 @@
+Add ``--numprocesses=logical`` flag, which automatically uses the number of logical CPUs available, instead of physical CPUs with ``auto``.
+
+This is very useful for test suites which are not CPU-bound.
diff --git a/changelog/650.feature.rst b/changelog/650.feature.rst
deleted file mode 100644
index ce6dc39..0000000
--- a/changelog/650.feature.rst
+++ /dev/null
@@ -1,1 +0,0 @@
-Added new ``pytest_handlecrashitem`` hook to allow handling and rescheduling crashed items.
diff --git a/changelog/654.removal.rst b/changelog/654.removal.rst
deleted file mode 100644
index b2159e3..0000000
--- a/changelog/654.removal.rst
+++ /dev/null
@@ -1,1 +0,0 @@
-Python 3.5 is no longer supported.
diff --git a/setup.py b/setup.py
index 9a75ca5..0bc80f0 100644
--- a/setup.py
+++ b/setup.py
@@ -23,7 +23,7 @@ setup(
         "pytest11": ["xdist = xdist.plugin", "xdist.looponfail = xdist.looponfail"]
     },
     zip_safe=False,
-    python_requires=">=3.6",
+    python_requires=">=3.5",
     install_requires=install_requires,
     setup_requires=["setuptools_scm"],
     classifiers=[
@@ -40,6 +40,7 @@ setup(
         "Programming Language :: Python",
         "Programming Language :: Python :: 3",
         "Programming Language :: Python :: 3 :: Only",
+        "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
         "Programming Language :: Python :: 3.8",
diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py
index d517a8a..ab927fa 100644
--- a/src/xdist/dsession.py
+++ b/src/xdist/dsession.py
@@ -14,7 +14,7 @@ from queue import Empty, Queue
 
 
 class Interrupted(KeyboardInterrupt):
-    """signals an immediate interruption."""
+    """ signals an immediate interruption. """
 
 
 class DSession:
@@ -343,12 +343,6 @@ class DSession:
             nodeid, (fspath, None, fspath), (), "failed", msg, "???"
         )
         rep.node = worker
-
-        self.config.hook.pytest_handlecrashitem(
-            crashitem=nodeid,
-            report=rep,
-            sched=self.sched,
-        )
         self.config.hook.pytest_runtest_logreport(report=rep)
 
 
diff --git a/src/xdist/newhooks.py b/src/xdist/newhooks.py
index d2482af..da0f22a 100644
--- a/src/xdist/newhooks.py
+++ b/src/xdist/newhooks.py
@@ -15,36 +15,36 @@ import pytest
 
 
 def pytest_xdist_setupnodes(config, specs):
-    """called before any remote node is set up."""
+    """ called before any remote node is set up. """
 
 
 def pytest_xdist_newgateway(gateway):
-    """called on new raw gateway creation."""
+    """ called on new raw gateway creation. """
 
 
 def pytest_xdist_rsyncstart(source, gateways):
-    """called before rsyncing a directory to remote gateways takes place."""
+    """ called before rsyncing a directory to remote gateways takes place. """
 
 
 def pytest_xdist_rsyncfinish(source, gateways):
-    """called after rsyncing a directory to remote gateways takes place."""
+    """ called after rsyncing a directory to remote gateways takes place. """
 
 
 @pytest.mark.firstresult
 def pytest_xdist_getremotemodule():
-    """called when creating remote node"""
+    """ called when creating remote node"""
 
 
 def pytest_configure_node(node):
-    """configure node information before it gets instantiated."""
+    """ configure node information before it gets instantiated. """
 
 
 def pytest_testnodeready(node):
-    """Test Node is ready to operate."""
+    """ Test Node is ready to operate. """
 
 
 def pytest_testnodedown(node, error):
-    """Test Node is down."""
+    """ Test Node is down. """
 
 
 def pytest_xdist_node_collection_finished(node, ids):
@@ -53,7 +53,7 @@ def pytest_xdist_node_collection_finished(node, ids):
 
 @pytest.mark.firstresult
 def pytest_xdist_make_scheduler(config, log):
-    """return a node scheduler implementation"""
+    """ return a node scheduler implementation """
 
 
 @pytest.mark.firstresult
@@ -64,20 +64,3 @@ def pytest_xdist_auto_num_workers(config):
 
     .. versionadded:: 2.1
     """
-
-
-@pytest.mark.firstresult
-def pytest_handlecrashitem(crashitem, report, sched):
-    """
-    Handle a crashitem, modifying the report if necessary.
-
-    The scheduler is provided as a parameter to reschedule the test if desired with
-    `sched.mark_test_pending`.
-
-    def pytest_handlecrashitem(crashitem, report, sched):
-        if should_rerun(crashitem):
-            sched.mark_test_pending(crashitem)
-            report.outcome = "rerun"
-
-    .. versionadded:: 2.2.1
-    """
diff --git a/src/xdist/plugin.py b/src/xdist/plugin.py
index 1eba32b..f7d1ff8 100644
--- a/src/xdist/plugin.py
+++ b/src/xdist/plugin.py
@@ -5,13 +5,14 @@ import py
 import pytest
 
 
-def pytest_xdist_auto_num_workers():
+def pytest_xdist_auto_num_workers(config):
     try:
         import psutil
     except ImportError:
         pass
     else:
-        count = psutil.cpu_count(logical=False) or psutil.cpu_count()
+        use_logical = config.option.numprocesses == "logical"
+        count = psutil.cpu_count(logical=use_logical) or psutil.cpu_count()
         if count:
             return count
     try:
@@ -36,8 +37,8 @@ def pytest_xdist_auto_num_workers():
 
 
 def parse_numprocesses(s):
-    if s == "auto":
-        return "auto"
+    if s in ("auto", "logical"):
+        return s
     elif s is not None:
         return int(s)
 
@@ -51,9 +52,10 @@ def pytest_addoption(parser):
         metavar="numprocesses",
         action="store",
         type=parse_numprocesses,
-        help="shortcut for '--dist=load --tx=NUM*popen', "
-        "you can use 'auto' here for auto detection CPUs number on "
-        "host system and it will be 0 when used with --pdb",
+        help="Shortcut for '--dist=load --tx=NUM*popen'. With 'auto', attempt "
+        "to detect physical CPU count. With 'logical', detect logical CPU "
+        "count. If physical CPU count cannot be found, falls back to logical "
+        "count. This will be 0 when used with --pdb.",
     )
     group.addoption(
         "--maxprocesses",
@@ -190,7 +192,7 @@ def pytest_configure(config):
 @pytest.mark.tryfirst
 def pytest_cmdline_main(config):
     usepdb = config.getoption("usepdb", False)  # a core option
-    if config.option.numprocesses == "auto":
+    if config.option.numprocesses in ("auto", "logical"):
         if usepdb:
             config.option.numprocesses = 0
             config.option.dist = "no"
diff --git a/src/xdist/scheduler/each.py b/src/xdist/scheduler/each.py
index cfe99e7..b2a0442 100644
--- a/src/xdist/scheduler/each.py
+++ b/src/xdist/scheduler/each.py
@@ -101,14 +101,6 @@ class EachScheduling:
     def mark_test_complete(self, node, item_index, duration=0):
         self.node2pending[node].remove(item_index)
 
-    def mark_test_pending(self, item):
-        self.pending.insert(
-            0,
-            self.collection.index(item),
-        )
-        for node in self.node2pending:
-            self.check_schedule(node)
-
     def remove_node(self, node):
         # KeyError if we didn't get an add_node() yet
         pending = self.node2pending.pop(node)
diff --git a/src/xdist/scheduler/load.py b/src/xdist/scheduler/load.py
index f32caa5..e378d9a 100644
--- a/src/xdist/scheduler/load.py
+++ b/src/xdist/scheduler/load.py
@@ -151,14 +151,6 @@ class LoadScheduling:
         self.node2pending[node].remove(item_index)
         self.check_schedule(node, duration=duration)
 
-    def mark_test_pending(self, item):
-        self.pending.insert(
-            0,
-            self.collection.index(item),
-        )
-        for node in self.node2pending:
-            self.check_schedule(node)
-
     def check_schedule(self, node, duration=0):
         """Maybe schedule new items on the node
 
diff --git a/src/xdist/scheduler/loadscope.py b/src/xdist/scheduler/loadscope.py
index c25e476..31dbe26 100644
--- a/src/xdist/scheduler/loadscope.py
+++ b/src/xdist/scheduler/loadscope.py
@@ -243,9 +243,6 @@ class LoadScopeScheduling:
         self.assigned_work[node][scope][nodeid] = True
         self._reschedule(node)
 
-    def mark_test_pending(self, item):
-        raise NotImplementedError()
-
     def _assign_work_unit(self, node):
         """Assign a work unit to a node."""
         assert self.workqueue
diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py
index 8fed077..19ed73a 100644
--- a/src/xdist/workermanage.py
+++ b/src/xdist/workermanage.py
@@ -293,7 +293,7 @@ class WorkerController:
             self._shutdown_sent = True
 
     def sendcommand(self, name, **kwargs):
-        """send a named parametrized command to the other side."""
+        """ send a named parametrized command to the other side. """
         self.log("sending command {}(**{})".format(name, kwargs))
         self.channel.send((name, kwargs))
 
diff --git a/tox.ini b/tox.ini
index 0e4ad97..78f3bed 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,7 +1,7 @@
 [tox]
 envlist=
   linting
-  py{36,37,38,39}-pytestlatest
+  py{35,36,37,38,39}-pytestlatest
   py38-pytestmain
   py38-psutil
 
@@ -21,6 +21,14 @@ deps = pytest
 commands =
   pytest {posargs:-k psutil}
 
+[testenv:linting]
+skip_install = True
+usedevelop = True
+passenv = PRE_COMMIT_HOME
+deps =
+  pre-commit
+commands = pre-commit run --all-files --show-diff-on-failure
+
 [testenv:release]
 changedir=
 decription = do a release, required posarg of the version number


Test patch:
diff --git a/testing/test_newhooks.py b/testing/test_newhooks.py
index d2c2878..0318442 100644
--- a/testing/test_newhooks.py
+++ b/testing/test_newhooks.py
@@ -60,37 +60,3 @@ class TestHooks:
             ["*HOOK: gw0 test_a, test_b, test_c", "*HOOK: gw1 test_a, test_b, test_c"]
         )
         res.stdout.fnmatch_lines(["*3 passed*"])
-
-
-class TestCrashItem:
-    @pytest.fixture(autouse=True)
-    def create_test_file(self, testdir):
-        testdir.makepyfile(
-            """
-            import os
-            def test_a(): pass
-            def test_b(): os._exit(1)
-            def test_c(): pass
-            def test_d(): pass
-        """
-        )
-
-    def test_handlecrashitem(self, testdir):
-        """Test pytest_handlecrashitem hook."""
-        testdir.makeconftest(
-            """
-            test_runs = 0
-
-            def pytest_handlecrashitem(crashitem, report, sched):
-                global test_runs
-
-                if test_runs == 0:
-                    sched.mark_test_pending(crashitem)
-                    test_runs = 1
-                else:
-                    print("HOOK: pytest_handlecrashitem")
-        """
-        )
-        res = testdir.runpytest("-n2", "-s")
-        res.stdout.fnmatch_lines_random(["*HOOK: pytest_handlecrashitem"])
-        res.stdout.fnmatch_lines(["*3 passed*"])
diff --git a/testing/test_plugin.py b/testing/test_plugin.py
index c1aac65..5870676 100644
--- a/testing/test_plugin.py
+++ b/testing/test_plugin.py
@@ -69,6 +69,12 @@ def test_auto_detect_cpus(testdir, monkeypatch):
     assert config.getoption("numprocesses") == 0
     assert config.getoption("dist") == "no"
 
+    config = testdir.parseconfigure("-nlogical", "--pdb")
+    check_options(config)
+    assert config.getoption("usepdb")
+    assert config.getoption("numprocesses") == 0
+    assert config.getoption("dist") == "no"
+
     monkeypatch.delattr(os, "sched_getaffinity", raising=False)
     monkeypatch.setenv("TRAVIS", "true")
     config = testdir.parseconfigure("-nauto")
@@ -81,12 +87,16 @@ def test_auto_detect_cpus_psutil(testdir, monkeypatch):
 
     psutil = pytest.importorskip("psutil")
 
-    monkeypatch.setattr(psutil, "cpu_count", lambda logical=True: 42)
+    monkeypatch.setattr(psutil, "cpu_count", lambda logical=True: 84 if logical else 42)
 
     config = testdir.parseconfigure("-nauto")
     check_options(config)
     assert config.getoption("numprocesses") == 42
 
+    config = testdir.parseconfigure("-nlogical")
+    check_options(config)
+    assert config.getoption("numprocesses") == 84
+
 
 def test_hook_auto_num_workers(testdir, monkeypatch):
     from xdist.plugin import pytest_cmdline_main as check_options
@@ -101,6 +111,10 @@ def test_hook_auto_num_workers(testdir, monkeypatch):
     check_options(config)
     assert config.getoption("numprocesses") == 42
 
+    config = testdir.parseconfigure("-nlogical")
+    check_options(config)
+    assert config.getoption("numprocesses") == 42
+
 
 def test_boxed_with_collect_only(testdir):
     from xdist.plugin import pytest_cmdline_main as check_options