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

Add a Scheduling Protocol #1061

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Apr 6, 2024

  • Clearly delineate the required interface of schedulers
  • Useful for typing

dist = config.getvalue("dist")
schedulers = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using the dict because I don't want to include the __init__ in the protocol (if that's even possible...)

return LoadGroupScheduling(config, log)
if dist == "worksteal":
return WorkStealingScheduling(config, log)
return None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a semantic change here -- if dist is not one of the builtin schedulers, this hook now returns None instead of raising a KeyError before. This seems better to me, allowing other hookimpls (e.g. wrappers) to handle this more gracefully.

@@ -103,6 +103,9 @@ def mark_test_complete(self, node, item_index, duration=0):
def mark_test_pending(self, item):
raise NotImplementedError()

def remove_pending_tests_from_node(self, node, indices):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some schedulers were missing this method that is used by DSession, if guess it's not called for them so raising NotImplementedError.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we will not add a changelog for this change because this is not public yet, correct?

from xdist.workermanage import WorkerController


class Scheduling(Protocol):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, up to you: I think a more appropriate name would be Scheduler (a subject), "sheduling" is a verb AFAIK. But I understand also that all the implementations use Scheduling in their name, so on one hand it makes sense to name this Scheduling, on the other hand, "two wrongs don't make a right".

Up to you really. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't know why the existing schedulers are called Scheduling but I don't think we can safely change them at this point. And then I think being consistent is better.

@bluetech
Copy link
Member Author

I assume we will not add a changelog for this change because this is not public yet, correct?

Right, it's purely internal for now. If we want to expose it, would really need proper docs on the methods which I'm not really familiar with yet.

- Clearly delineate the required interface of schedulers
- Useful for typing
@bluetech bluetech merged commit 5dfc590 into pytest-dev:master Apr 15, 2024
21 checks passed
@bluetech bluetech deleted the scheduling-proto branch April 15, 2024 15:20
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.

2 participants