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

WORK-IN-PROGRESS - contributing isoscope scheduler #1126

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

vitaly-krugl
Copy link

@vitaly-krugl vitaly-krugl commented Sep 6, 2024

WORK-IN-PROGRESS - READY FOR PREVIEW;

Implementation of the Distributed Scope Isolation scheduler for pytest-xdist.

Properties of this scheduler:

  1. Executes one test scope/class at a time.
  2. Distributes tests of the executing scope/class to the configured XDist Workers.
  3. Guarantees that the Setup of the executing scope/class completes in all XDist Workers BEFORE any of those Workers start processing the Teardown of that test scope/class.
  4. Guarantees that the Teardown phase of the executing test scope/class completes in all XDist Workers before the Setup phase begins for the next test scope/class.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Make sure to include reasonable tests for your change if necessary

  • We use towncrier for changelog management, so please add a news file into the changelog folder following these guidelines:

    • Name it $issue_id.$type for example 588.bugfix;

    • If you don't have an issue_id change it to the PR id after creating it

    • Ensure type is one of removal, feature, bugfix, vendor, doc or trivial

    • Make sure to use full sentences with correct case and punctuation, for example:

      Fix issue with non-ascii contents in doctest text files.
      

@nicoddemus
Copy link
Member

nicoddemus commented Sep 18, 2024

Hi @vitaly-krugl thanks for the PR!

Although this PR has very high quality, to be honest I'm hesitant of accepting this PR, because it is somewhat complex and pytest-xdist does not have much maintenance bandwidth and it adds to that.

But this is my opinion only, @RonnyPfannschmidt @bluetech what do you think?

Also consider that this might be published as a separate plugin, it does not need to be included in pytest-xdist: the package would implement pytest_xdist_make_schedule to return the custom scheduler; if there is anything else missing in pytest-xdist to accommodate an external scheduler we would be glad to adapt pytest-xdist for that.

@RonnyPfannschmidt
Copy link
Member

first i want to recognize the effort and creativity that went into this pr,
the implementation sort out a very complex coordination problem in "just" a few thousand lines of code

those few thousand lines are conceptually very dense, and currently come without additional tests,
i have no idea if/when i can dedicate a few days to actually gnaw trough it and properly comprehend it

we have a very bad match up here for the maintenance situation - the new contributed code is without tests and of necessarily high complexity

most volunteer managed opensource projects are unable to take in such a contribution easily, in particular if it comes in without any deep prior engagement/discussion with the maintainer team

if the goal is to make this quickly available, then i strongly recommend publishing this as a "plugin" to xdist,
in which case we are happy to integrate ci in a way that ensures that future xdist changes do not break this plugin

if the goal is to land this in xdist, then its likely to take a long time, as the contribution size and complexity of the solved problem is immense

i agree that the code itself is high quality, but the high complexity of the solved problem, the sheer size of this contribution, the need for tests to be written and our own awareness of our very limited maintenance capabilities
turn this contribution into something that has quite the intimidating factor as both, due diligence in review was well as continued maintenance

its going to be a while before im in a place to give it the due diligence it needs, and i suspect this will be a problem for the initial contribution

i also suspect that multi host aware architecture of the coordinator will look different than initially imagined

my current recommendation would be to begin this as own pypi package, adding a own testsuite and to coordinate with xdist to ensure xdist changes dont break it

in addition if this is not a direct part of xdist, its more easy to declare multi host testing not being supported in the feature and engage as we eventually enable to pull this in

i absolutely want to enable coordinated fixture/resource creation/destruction
i also beleive that the full conclusion of supporting managing resources will need it to be able to be declarative
(and creating that api wont be a walk in the par)

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 this pull request may close these issues.

3 participants