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

Missing mechanism to extend the --dist choices when adding a new scheduler results in pytest execution failure #970

Open
vitaly-krugl opened this issue Nov 14, 2023 · 8 comments · May be fixed by #1148

Comments

@vitaly-krugl
Copy link

vitaly-krugl commented Nov 14, 2023

Documentation says to use pytest_xdist_make_scheduler to add a new scheduler. However, command line parsing fails because the choices for the --dist command line option are fixed by xdist.plugin.pytest_addoption. Also, help string that documents each of the available options in --dist is not extensible to include the new scheduler choices.

$ pytest --basetemp ~/xdist-debug/basetemp/ -p my_xdist_schedulers.plugin -n 3 --dist mycoolnewscheduler
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: argument --dist: invalid choice: 'mycoolnewscheduler' (choose from 'each', 'load', 'loadscope', 'loadfile', 'loadgroup', 'worksteal', 'no')
@vitaly-krugl vitaly-krugl changed the title Missing hook to extend --dist choices when adding a new scheduler Missing mechanism to extend the --dist choices when adding a new scheduler results in failure Nov 14, 2023
@vitaly-krugl vitaly-krugl changed the title Missing mechanism to extend the --dist choices when adding a new scheduler results in failure Missing mechanism to extend the --dist choices when adding a new scheduler results in pytest execution failure Nov 14, 2023
@nicoddemus
Copy link
Member

Hi @vitaly-krugl,

Indeed this is a problem that we overlooked.

From the top of my head I don't know how we could solve this, TBH.

@RonnyPfannschmidt
Copy link
Member

We could consider a entry point

@vitaly-krugl
Copy link
Author

vitaly-krugl commented Nov 15, 2023

I ended up working around this limitation as follows:

  1. Handle pytest_addoption hook and add a group/option for my own scheduler's --custom-dist to the parser
  2. Handle pytest_cmdline_parse with hookwrapper=True where, after the non-hookwrappers run, I overwrite config.option.dist with the name of my own scheduler's --custom-dist option value, if it was passed on command line. This overwrite of config.option.dist was necessary - if I didn't overwrite it, then xdist's plugin.pytest_configure would find the default "no" in config.option.dist and bypass distributed execution altogether (wouldn't create DSession, etc.)
  3. Handle pytest_xdist_make_scheduler and instantiate my own scheduler if config.option.dist value matches my scheduler.

NOTE: I needed to pass my plugin via both the -p command line arg and also via the plugins' function argument to pytest.main()` to make this work.

  • Without -p, I got an exception when xdist re-parses the args in its remote worker - "error: unrecognized arguments: --custom-dist".

  • Without the plugins' function argument to pytest.main(), my hookwrapper for pytest_cmdline_parse` doesn't get called.

@vitaly-krugl
Copy link
Author

vitaly-krugl commented Nov 16, 2023

@nicoddemus and @RonnyPfannschmidt, the workaround that I documented in my previous comment isn't necessarily a substitute for a well-documented and supported scheduler plugin mechanism. However, it demonstrates a relatively-clean (I think) working prototype for bootstrapping an add-on xdist scheduler into xdist and into the pytest command-line. It also suggests that an add-on scheduler may have a need to set its own specific options via pytest command line.

@amezin
Copy link
Collaborator

amezin commented Nov 17, 2023

Maybe there should be a hook that returns a list of available schedulers (or a name => callable (factory) dictionary)?

@nicoddemus
Copy link
Member

Maybe there should be a hook that returns a list of available schedulers (or a name => callable (factory) dictionary)?

Yeah that's an idea, however it does not work currently on how options are added currently: pytest_addoption (which declares dist, along with the list of available options like no, load, each, etc) is called during discovery by pluggy (because it is pytest_addoption is marked as historic), and at this point, no other hooks have been "found" yet. This is something we would need to change in pluggy itself to make this work.

@nicoddemus
Copy link
Member

the workaround that I documented in my previous comment isn't necessarily a substitute for a well-documented and supported scheduler plugin mechanism

Definitely, unfortunately (at least to me) there is no clear way forward yet on how to support this nicely.

@vitaly-krugl
Copy link
Author

Hi @nicoddemus, here is a proposal that I think could work:

Presently, when xdist's plugin.pytest_configure would finds the default "no" in config.option.dist, it bypasses distributed execution altogether (wouldn't create DSession, etc.).

  1. The proposed change is to not do the above. Instead, consider config.option.dist to be the name of the built-in scheduler and assume that other scheduler providers may have their own command line options for their own scheduler.
  2. Modify xdist's handler of pytest_xdist_make_scheduler to return None if `config.option.dist == 'no'.
  3. Invoke the hook pytest_xdist_make_scheduler. If the hook doesn't return a scheduler, then - and only then - bypass distributed execution.

amezin added a commit to amezin/pytest-xdist that referenced this issue Nov 1, 2024
There seems to be no way to fix the argument validation, at least without
complicating things unnecessarily.

So simply remove it. Instead, check `pytest_xdist_make_scheduler` return
value.

Also, convert every builtin scheduler to a separate plugin.

Fixes: pytest-dev#970
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

Successfully merging a pull request may close this issue.

4 participants