modify parameters passed to asyncio.wait to avoid deprecation warning.

[rosesyrett]

Remove asyncio.wait deprecation warning by only passing tasks to it.

In the master scheduler _do_task method, a coroutine is passed to asyncio.wait. This is deprecated behaviour. I noticed this while working on #153:

File "/workspace/tickit/src/tickit/core/management/schedulers/master.py", line 102, in _do_tick
    which, _ = await asyncio.wait(
  File "/usr/local/lib/python3.10/asyncio/tasks.py", line 377, in wait
    warnings.warn("The explicit passing of coroutine objects to "
DeprecationWarning: The explicit passing of coroutine objects to asyncio.wait() is deprecated since Python 3.8, and scheduled for removal in Python 3.11.

[abbiemery]

Are you certain these are synonymous?

[rosesyrett]

Yes. previously, you had:

current: asyncio.Future = asyncio.sleep(self.sleep_time(when))

which is not a future (as the typing suggests) but a coroutine as per the docs. This change makes it a task, which is an object that runs a coroutine. Basically it adds a layer of wrapping around a coroutine, to comply with python 3.11 standards.

So before, we were setting up a coroutine and then awaiting it. Now, we are setting up a task and awaiting it, at which point we await the coroutine. They are functionally exactly the same.

[tpoliaw]

I'm not entirely sure on this but I think they're not the same in general but in this case it doesn't make any difference. create_task means it starts running immediately where it didn't before but given we immediately wait on the result I think it should be ok. If what we're doing is not supported in 3.11 anyway I don't think this can work any less than it already does.

[tpoliaw]

which is not a future (as the typing suggests)

Is there any advantage to leaving the type annotation there now that it is correct?

[rosesyrett]

is a task a future? Technically we've made some tasks so why not have asyncio.Task typing for both this line and the one below it instead? Leaving them out, mypy seems to understand what types they are also - I'm assuming pyright would be the same.

[garryod]

Yeah, they're really quite different, Coroutines are lazy whilst Tasks are eager meaning that the timer will (potentially) now start ever so slightly earlier. On top of that, the whole reason this API is becoming more restrictive is due to the fact that the automatic promotion of the Coroutine into a Task in the old API left the user without a handle to the task, see this note in the python 3.9 docs

[rosesyrett]

It's a good point. I think the docs aren't very clear about this behaviour:

' Wrap the coro coroutine into a Task and schedule its execution ' is what you get when looking at asyncio.Task(). 'schedule its execution' doesn't sound eager to me.

could we use asyncio.gather instead? That seems to just take awaitables.

[rosesyrett]

Currently I see two solutions to this:

1. use asyncio.gather, which can accept coroutines, and keep the definition as it was (maybe minus the type hint). However, we will possibly get a 'coroutine was not awaited' warning.
2. use asyncio.wait, creating the task in the call, i.e. :

new = asyncio.create_task(self.new_wakeup.wait())
which, _ = await asyncio.wait(
    [asyncio.create_task(asyncio.sleep(self.sleep_time(when))), new], return_when=asyncio.tasks.FIRST_COMPLETED
)

Thoughts? Or am i missing potential solutions to this/ are the solutions above not ideal?

[garryod]

1. use asyncio.gather, which can accept coroutines, and keep the definition as it was (maybe minus the type hint). However, we will possibly get a 'coroutine was not awaited' warning.

This won't work as we need to check the task handle returned is the task which returns on an interrupt:

if new in which:
    return

The correct solution is exactly what you have in the PR 🤣 we're all just bikeshedding

[rosesyrett]

yeah, i guess gather also wouldn't work because I don't see a way to just return when one awaitable completes...

What do you think of the 2nd solution? Surely this minimises how eager 'create_task' is for the sleep?

[garryod]

What do you think of the 2nd solution? Surely this minimises how eager 'create_task' is for the sleep?

I'm not quite willing to jump into the bytecode, but I expect it's functionally equivalent to:

        new = asyncio.create_task(self.new_wakeup.wait())
        current = asyncio.create_task(asyncio.sleep(self.sleep_time(when)))
        which, _ = await asyncio.wait(
            [current, new], return_when=asyncio.tasks.FIRST_COMPLETED
        )

I wouldn't want to sacrifice readability for a few microseconds in this case, also, I'm not sure if late or early is more correct anyway

[codecov]

Codecov Report

Merging #160 (51d27d3) into master (0ff34b1) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #160   +/-   ##
=======================================
  Coverage   94.69%   94.69%           
=======================================
  Files          44       44           
  Lines        1264     1264           
=======================================
  Hits         1197     1197           
  Misses         67       67           

Impacted Files	Coverage Δ
src/tickit/core/management/schedulers/master.py	88.23% <100.00%> (ø)