Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-nauto isn't smart enough #853

Open
boatcoder opened this issue Dec 2, 2022 · 8 comments
Open

-nauto isn't smart enough #853

boatcoder opened this issue Dec 2, 2022 · 8 comments

Comments

@boatcoder
Copy link

-nauto is fine for deciding how many processes you need based on processors, but it should wait and make it's decision AFTER tests have been collected and filtered and if the number of tests to run is less than the number of processors, it should scale back and only start as many processes as are needed. This will probably mean that it takes a tiny bit longer to get started but if you are using vscode and debugging tests, you won't spawn 10 processes and only use 1 of them. Maybe this behavior is only needed if there are tests specified on the command line (which would mean it wouldn't slow down full test runs).

@nicoddemus
Copy link
Member

In the current architecture this is not possible, because we start by spawning workers, which themselves do the collection.

@kurt-cb
Copy link

kurt-cb commented Mar 4, 2023

I am doing this, but it takes a bit of monkeypatch or changes to xdist. I have added code to xdist to start another node dynamically when all the nodes are busy. This also allows removal of nodes that are no longer needed.
The solution involves waiting for only one worker to start, then only assign that worker a single task. Then, the scheduler can determine based on heuristics if more workers are need and simply add another worker node.
I will have a PR that implements this technique and also solves the stupid "each worker needs two tests" problem created by pytest's nextitem requirement.

This bit of "magic" is required to be inserted into dsession (either by monkepatching or just modify your version) to support this:

    def start_new_node_with_env(self, env):
        """ start a new node with specified environment variables
        this can be used to start a worker with a specific configuration
        and scheduler can assign work based on that configuration.
        """
        import execnet

        spec = execnet.XSpec("popen")
        self.nodemanager.group.allocate_id(spec)
        self.trdist._specs.append(spec)
        self.trdist.setstatus(spec, "I", show=True)
        spec.env.update(env)
        node =self.nodemanager.setup_node(spec, self.queue.put)
        self._active_nodes.add(node)
        return node

There are some issues schedulers logic getting confused when a new node comes along that need tweaked also. But the changes are minor and relate to collection tracking.

My fork has this change as well as several other improvements to scheduling. There looks to be some other development revolving around work stealing, however I don't understand how pytest teardown works with such a concept.

@DetachHead
Copy link

DetachHead commented Mar 7, 2024

i came up with this workaround which seems to work well in vscode:

// .vscode/settings.json

{
    "python.testing.pytestArgs": ["-n", "auto"]
}
# conftest.py

@hookimpl(wrapper=True)
def pytest_xdist_auto_num_workers(config: Config) -> Generator[None, int, int]:
    """determine how many workers to use based on how many tests were selected in the test explorer"""
    num_workers = yield
    if "vscode_pytest" in config.option.plugins:
        return min(num_workers, len(config.option.file_or_dir))
    return num_workers

this works because vscode passes each test to pytest as separate arguments (eg. pytest test_foo.py::test_foo test_foo.py::test_bar), so you can guess how many tests are going to run before they get collected.


edit: looks like this relies on vscode's new test adapter, which isn't yet enabled for all users. to enable it, you need to add this to your user settings.json (doesn't work in the workspace one unfortunately):

{
    "python.experiments.optInto": ["pythonTestAdapter"]
}

@boatcoder
Copy link
Author

boatcoder commented Mar 7, 2024

The above patch from @DetachHead almost worked for me. I did have to add
num_workers = num_workers.get_result() or it would fail during the comparison.

@DetachHead
Copy link

that's odd. what version of xdist are you using? for me it just returns an int. i'm on 3.5.0

@boatcoder
Copy link
Author

boatcoder commented Mar 9, 2024

pytest==7.4.0
pytest-base-url==2.0.0
pytest-celery==0.0.0
pytest-cov==4.1.0
pytest-django==4.5.2
pytest-forked==1.4.0
pytest-random-order==1.1.0
pytest-xdist==3.3.1

I'll try updating and see if I can remove that line.

@boatcoder
Copy link
Author

pytest-xdist-3.5.0
and python 3.10.13
the problem persists. I'm just going to leave the line in and it works.

@DetachHead
Copy link

ah, it sounds like you're using @hookimpl(hookwrapper=True) instead of @hookimpl(wrapper=True). wrapper is the new one and hookwrapper is deprecated. https://pluggy.readthedocs.io/en/stable/index.html#wrappers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants