-
-
Notifications
You must be signed in to change notification settings - Fork 66
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 configurable options to scheduler #107
Add configurable options to scheduler #107
Conversation
I removed the other planned options for now to make this PR easier to review. |
Are any of these functions performance critical? Should we worry about the overhead of kwargs? |
Should not have any noticeable impact given that they stop propagating once they hit |
From my own experience, sending arbitrary |
If there wasn't the current scheduler plugin architecture built into Dagger (which I will be making good use of), then I would completely agree with you and only thread the kwargs that're needed. But with the plugin architecture being a thing, and different schedulers having the potential for different relevant options (multithreading, GPU support, etc.), we need some way to be generic about scheduler configuration. As an alternative, we could extend the plugin API with a way to create a scheduler-specific struct that can be passed as a single kwarg which will contain the appropriate options. If you'd prefer that sort of API, I will change this PR to implement that approach instead. |
I suspect the latter solution would be simpler to manage down the road while for former is faster to get going right away. I'll let you decide since you'll be the one to actually do the work so if you prefer to stick with the current approach let us know and I'll merge. |
I'd rather do this right the first time, so I'll go with the latter (struct) approach. I'll let you know once I've made the appropriate changes. Thanks for the helpful review :) |
@andreasnoack let me know if the latest changes are to your liking when you get the chance to review. |
Thanks for the review @andreasnoack ❤️ |
This PR allows the scheduler to take options as keyword args. To start with, I'm adding an option called "single" which forces all work onto a single worker (master or a specific worker) by worker id.