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

[feature request]Support priorities #455

Open
kshpytsya opened this issue Oct 18, 2023 · 6 comments
Open

[feature request]Support priorities #455

kshpytsya opened this issue Oct 18, 2023 · 6 comments

Comments

@kshpytsya
Copy link

Over time, my use case for aiojobs got extended to require support for job priorities, i.e. it should be possible to pass a priority value to Scheduler.spawn method, and if not executed immediately, job should be placed into the pending queue at a position according to the specified priority.

If possible performance impact of replacing deque (which is the structure behind asyncio.Queue used by aiojobs) with heapq+list (used by asyncio.PriorityQueue) could be neglected, then I would suppose that implementing this feature request would be as simple as adding an optional priority parameter to spawn method and switching to PriorityQueue. Otherwise, a slightly more involved implementation would involve creating a Scheduler subclass tentatively named PriorityScheduler.

My use case doesn't require support for priorities in aiojobs.aiohttp but I suspect that it may be desirable to implement that as well for the sake of completeness.

@kshpytsya
Copy link
Author

P.S. Another smartish approach would be having the Scheduler switch from Queue to PriorityQueue at the first spawn call with non-default priority. Existing pending jobs (if any) would be transferred from old to new queue. The overhead for use case without priorities would be a single priority is not None check in spawn. The self._pending.get_nowait would be replaced by something like self._pending_get_nowait which would initially be set to self._pending.get_nowait(), and after switching to PriorityQueue would be set to lambda: self._pending.get_nowait()[1].

@Dreamsorcerer
Copy link
Member

Or just using 2 different class names maybe...

To be honest, I'm not sure if it's really worth maintaining the library going forwards. Most of its use cases should be covered by asyncio in the near future (I think it may be called Supervisor), so I know I probably won't be using it after that point.

If you want to make a PR though, I'll review it.

@kshpytsya
Copy link
Author

I am up to implementing either naive approach (replacing Queue with PriorityQueue and adding non-required keyword argument priority to spawn method), or the "smartish" approach I have described in my comment. I am not up to implementing separate classes like Scheduler and PriorityScheduler. Which one stands a chance of acceptance?

@Dreamsorcerer
Copy link
Member

Hmm, I'm looking at the code, and 2 classes seems like the most logical approach. Clear separation and flexible.
I think we can just subclass the existing scheduler, change the queue type in init, then tweak spawn() and _done() to include a priority.

We'd need to tweak the base class so we can maybe do something like this in the subclass:

async def spawn(..., priority=1):
    return await super().spawn(..., put_queue=lambda j: self._pending.put((priority, j))

While the original class will have put_queue=lambda j: self._pending.put(j). And something similar in _done().

From here, people could subclass and implement their own queue structures the same way.

@kshpytsya
Copy link
Author

If we go with two classes, what (if anything) should be done about aiojobs.aiohttp ? Should we add an optional scheduler_class parameter to aiojobs.aiohttp.setup ?

@Dreamsorcerer
Copy link
Member

Yep, could do that, if it's even going to be useful for aiohttp.

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

No branches or pull requests

2 participants