Allow custom scheduler class implementation

[wronglink]

Hi. I've seen several tickets when people need more control over tests scheduling (see #18). I think, it would be much better, if xdist would give a a way to implement custom test scheduler class and pass it to DSession object. I've came up with this:

1. We freeze a schedule interface.

2. One can have his one scheduler class implementation, available for import.

   from _pytest.runner import CollectReport
   import py
   from xdist.dsession import report_collection_diff
   
   
   class CustomScheduling:
       def __init__(self, numnodes, log=None, config=None):
           self.numnodes = numnodes
           self.log = log
           self.config = config
   
       # ... custom scheduling logic implementation

3. Python import path should be passed to xdist module through optional --dc command line parameter:

   pytest --boxed -nauto --dc=custom.CustomScheduling tests.py
   

[nicoddemus]

Hi, thanks for the PR!

I like the idea; we have prior discussion on how to implement support for customizing test distribution on workers, but alas nobody had the time to implement it yet. I think the solution you are proposing is a good middle ground.

Let's see what are other's opinions on this.

[nicoddemus]

importlib is available only on Python >= 3.1, so unfortunately we can't use it like this. I suggest using __import__ which is available in all Python versions we have to support.

[wronglink]

Thanks, I guess I can use __import__ for that case. I thought that pytest has general workaround for that case, but as I can see it has only specialised methods like import_plugin.

[nicoddemus]

Also we should add some documentation for this feature on README

[wronglink]

Also we should add some documentation for this feature on README

Got it. I'll do that.

[RonnyPfannschmidt]

this is a nice addition, however we must mark the scheduler api as unstable
(the plan is to break it , which is reasonably simple when its not exposed and expensive when it is exposed)

personally i had a pytest_setup_sheduler hook in mind

[wronglink]

we must mark the scheduler api as unstable

Do you mean mark in code or add a notice in README file?

[RonnyPfannschmidt]

i think a notice in the readme is a good fit

[wronglink]

personally i had a pytest_setup_sheduler hook in mind

Actually, it would be even more native way and would free us from importing classes. @nicoddemus what do you think of adding a hook rather then adding a CLI parameter?

[nicoddemus]

Actually, it would be even more native way and would free us from importing classes. @nicoddemus what do you think of adding a hook rather then adding a CLI parameter?

Wholeheartedly agree! 😁 This makes it easy for plugins to provide a custom scheduler.

How about naming it pytest_xdist_make_scheduler, to follow the convention of other hooks such as pytest_runtest_makereport, pytest_pycollect_makeitem, etc?:

def pytest_xdist_make_scheduler(log, config):
    """Return a scheduler implementation, or None.   
    """

(numnodes can be obtained from config.getoption('numnodes') if the implementation cares about it)

We should document the expected interface, or declare an official interface using the abc module.

[nicoddemus]

Oh in case it wasn't clear, I think the hook should return an instance instead of a class.

Implementations should be free to declare any constructor signature and obtain those from the config object as necessary.

[nicoddemus]

Hey @wronglink, thanks for following up!

The pytest-2.6 environments failed, probably due to the new way to use hooks. TBH I think we can drop support for pytest-2.6 in the next release and don't worry about it, pytest 2.6.4 was released in Oct 24th, 2014.

@RonnyPfannschmidt do you agree?

Other than that, there was minor errors in the flakes environment, could please you fix them @wronglink?

[nicoddemus]

We should also add a new section to the README, as well as a CHANGELOG entry.

[nicoddemus]

I think we can drop numnodes from the hook signature... it can be easily obtained from the config object.

[wronglink]

@nicoddemus I think to add docs as I'm ready to finalise the PR. I'd like to implement something like base scheduler class and clean it interface by getting rid of numnodes parameter. As I can see, we can use something like:

class EachScheduling:
    ...
    @property
    def numnodes(self):
        """Nodes amount in the scheduler."""
        return len(self.nodes)
    ...

But I haven't fixed the failing tests, so didn't include that change.

[nicoddemus]

Sounds good!

Do you think we should also create an abstract class using the abc module so the interface is clear for people extending it?

[RonnyPfannschmidt]

please mark all those apis as provisional, they are supposed to be broken when introducting more formal statemachines

[nicoddemus]

TBH I think we can drop support for pytest-2.6 in the next release and don't worry about it, pytest 2.6.4 was released in Oct 24th, 2014.

@RonnyPfannschmidt do you agree?

Ping. 😁

[RonnyPfannschmidt]

whops,. feel free to drop it

[metasyn]

hi strangers! this is cool. i've been following this and the history of the xdist worker scheduling attempts for a little while. are there actionables for this PR?

[wronglink]

Hi there. I'm really sorry I didn't have enough time to make all requested changes for this PR. I'll try to make one more attempt to fix it, but can't give any deadline for this.

[nicoddemus]

@wronglink no problem at all, we appreciate your efforts here.

Would you mind if any of us maintainers pick up from here if we find the time? The PR is nearly ready as it is.

[wronglink]

@nicoddemus for sure, I'll be grateful in that case. BTW I don't mind either someone starts new PR or give access to my fork to keep changes inside one PR.

[wronglink]

Hello again. I've found some time to continue work. For now, I'm a little bit confused by some part of current scheduler API. I tried to clean up both LoadScheduling and EachScheduling to extract minimal simple and clear API as list of methods and properties required to be implemented by any custom scheduler class. I've also did some renames to make interfaces more consistent.

I ended up with something like this (marked most confusing parts with !):

class Scheduler:
    # Indicates that all tests are already finished
    tests_finished = False
    # List of all nodes available for scheduler
    nodes = []
    # Indicates that tests collection is already finished
    # ! need some explanation the difference between this and tests_finished
    collection_is_completed = False
    # Indicates that scheduller has pending tests. These are tests which have
    # not yet been allocated to a chunk for a node to process.
    has_pending = False
                         

    def add_node(node):
        """ Add node to the pool
        Called when node is set up and ready
        """

    def remove_node(node):
        """ Remove node from the pool
        Called to clean up nodes when tests are finished or error happens
        """

    def add_node_collection(node, ids):
        """ Adds tests collection to node.
        ! don't understand why we add same collection for every node (we also check, that
        ! each node got same collection) and not simply send it to schedule() method
        """

    def mark_test_complete(node, item_index, duration):
        """ Mark particular test item as complete
        Called when slave reported that test with item_index is complete
        """
    
    def schedule():
        """ Initiate schedule of the test collection """

@nicoddemus @RonnyPfannschmidt do you have any comments or suggestions?

[RonnyPfannschmidt]

the current collection addition is purely for sanity

in future we hope to expand, so different slaves can run different sets of tests (like one folder per slave, one configured backend per slave)

it may be necessary to extract a better node concept in future (one of the main reasons why i said to mark those apis provisional is, that we literally have no idea what a good api will be unless we iterate a few time and reflect on what we learn from those iterations)

[RonnyPfannschmidt]

a sidenote, the api looks very imperative, the scheduler should be a manager of state that reacts on events - and it may help if the method names reflect those events

[nicoddemus]

Oh thanks for continuing the work @wronglink!

the current collection addition is purely for sanity

Just to complement: there have been bugs in the past where nodes ended up with different sets of tests collected, which is problematic because the master sends just the index of the test item to a worker to execute next. @RonnyPfannschmidt I seem to recall there was a reason why we don't send the entire test nodeid (as str) other than performance of sending an int over the wire, do you recall what is it?

@wronglink cool that you are cleaning the API, I like the new names so far. I agree that ideally the API would be more reactive like @RonnyPfannschmidt suggested but I understand it's hard for you to find time to work on this feature, so I suggest we try to merge this without too much more change requests. Then we can iterate over it in subsequent PRs, hopefully before publishing it in the next release.

[RonnyPfannschmidt]

@nicoddemus we send indexes instead of nodeids because we allow duplicate test id's

[RonnyPfannschmidt]

since we do have a structural enhancement and havent declared the api public yet, maybe we should just merge it and iterate on top of it

[RonnyPfannschmidt]

after all this pr is good work and puts us in a better spot, even if details may need to cange

[nicoddemus]

Oh yeah, duplicated ids, thanks.

since we do have a structural enhancement and havent declared the api public yet, maybe we should just merge it and iterate on top of it

Agreed, we can iterating on this in other PRs.

[wronglink]

Cool, I'll make another PR to mention this change in CHANGELOG and README.md

[JohnTheodore]

I do -n auto, and one particular test class in my app can't be run in parallel. Does this PR allow me to specify this particular test class to run serially and ignore -n auto?

[wronglink]

@JohnTheodore right, that was one of my purposes. You can look at this gist for inspiration:
https://gist.github.com/wronglink/12e51408431098167b588a64b4e86936