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

extmod/asyncio: Add Task methods from CPython #13000

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

imnotjames
Copy link

@imnotjames imnotjames commented Nov 17, 2023

This updates the way that Task is defined in the C module for asyncio which implements tasks & task queues. The goal is to bring asyncio in MicroPython slightly closer to CPython.

This adds the following methods to Task:

  • get_coro() - how CPython exposes the coroutine backing the task (CPython Docs)
  • result() - a helper method from CPython for returning the successful response (CPython Docs)
  • exception() - a helper method from CPython for returning the raised values (CPython Docs)
  • cancelled() - helper for true / false if the task has been cancelled (CPython Docs)
  • add_done_callback() - adds a callback to the task for completion or if already complete executes it (CPython docs)
  • remove_done_callback() - removes a specific "done" callback from the task (CPython docs)

Adds a hash unary operation so Tasks can be added to sets like in CPython.

Also adds set_result and set_exception for consistency but neither of these do anything - they raise a RuntimeError because they aren't supported on Tasks. 🤷

Copy link

github-actions bot commented Nov 17, 2023

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64: +1760 +0.220% standard[incl +224(data)]
      stm32:  +620 +0.158% PYBV10
     mimxrt:  +672 +0.185% TEENSY40
        rp2:  +688 +0.210% RPI_PICO
       samd:  +664 +0.253% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@imnotjames imnotjames force-pushed the feat/upython/async-tasks branch 2 times, most recently from 964a9a9 to f029578 Compare November 17, 2023 06:24
@imnotjames imnotjames changed the title feat: add Task methods to make _asyncio more similar to cpython extmod/asyncio: add Task methods to make _asyncio more similar to cpython Nov 17, 2023
@imnotjames imnotjames changed the title extmod/asyncio: add Task methods to make _asyncio more similar to cpython extmod/asyncio: Add Task methods from CPython Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a968888) 98.43% compared to head (8d260d8) 98.43%.
Report is 13 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13000   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files         158      158           
  Lines       20978    21027   +49     
=======================================
+ Hits        20649    20698   +49     
  Misses        329      329           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@imnotjames imnotjames force-pushed the feat/upython/async-tasks branch 7 times, most recently from da05a15 to fb170d9 Compare November 18, 2023 18:30
extmod/modasyncio.c Outdated Show resolved Hide resolved
@imnotjames imnotjames marked this pull request as ready for review November 18, 2023 18:33
@imnotjames imnotjames force-pushed the feat/upython/async-tasks branch 2 times, most recently from 921984b to ae0905c Compare November 18, 2023 19:14
Adds methods that are in CPython, such as `exception`, `result`,
`get_coro`, `cancelled`, `add_done_callback`, and
`remove_done_callback`.

Also adds support for the unary hash so tasks may be collected in
a python set.

Signed-off-by: James Ward <james@notjam.es>
Signed-off-by: James Ward <james@notjam.es>
Signed-off-by: James Ward <james@notjam.es>
@andrewleech
Copy link
Sponsor Contributor

I personally wouldn't worry about adding:

Also adds set_result and set_exception for consistency but neither of these do anything - they raise a RuntimeError because they aren't supported on Tasks.

Adding even function names for these costs space, and not adding them will just raise an exception anyway, it's really not much different!

@imnotjames
Copy link
Author

I personally wouldn't worry about adding:

Also adds set_result and set_exception for consistency but neither of these do anything - they raise a RuntimeError because they aren't supported on Tasks.

Adding even function names for these costs space, and not adding them will just raise an exception anyway, it's really not much different!

I guess so. My concern was that the exception would be a different kind of error but that's probably ok.

@imnotjames imnotjames force-pushed the feat/upython/async-tasks branch 2 times, most recently from d65d495 to a97ef0d Compare November 20, 2023 00:11
Signed-off-by: James Ward <james@notjam.es>
Signed-off-by: James Ward <james@notjam.es>
@imnotjames imnotjames force-pushed the feat/upython/async-tasks branch 2 times, most recently from 0d98d3b to db9db03 Compare November 20, 2023 00:48
Signed-off-by: James Ward <james@notjam.es>
@imnotjames
Copy link
Author

Okay, dropped those functions and fixed the commit messages.

@dpgeorge
Copy link
Member

Thanks for the contribution.

MicroPython aims to be compact so it can run on devices without many resources. As such, these kinds of methods/functions were left out of the asyncio implementation on purpose (among many others).

That said, we can add them if they are important and useful.

What motivated you to add these particular features? Do they add anything that you can't already do?

I don't think the done-callback functions are really needed. MicroPython doesn't support any kind of callback in asyncio. Instead you can wait on the task to complete then run the required code.


def add_done_callback(self, callback):
if not self.state:
callback(self, self.data)
Copy link
Member

Choose a reason for hiding this comment

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

This signature of the callback does not match CPython. In CPython the callback takes only one argument.

Copy link
Author

Choose a reason for hiding this comment

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

That's right - I'm hoping to follow up to bring this in line with CPython in cases where only one argument is accepted in the function in a follow up PR.

However, changing the callback signature here would mean it's different from where it gets called elsewhere.

Copy link
Author

Choose a reason for hiding this comment

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

I could add a comment to clarify that if it helps?

@@ -255,6 +387,15 @@ STATIC void task_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
}
}

STATIC mp_obj_t task_unary_op(mp_unary_op_t op, mp_obj_t o_in) {
Copy link
Member

Choose a reason for hiding this comment

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

Hashing of Task should actually work, since commit eaccaa3. So I think this bit of code can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

I'll confirm it without this, it's very likely that's the case!

Copy link
Author

@imnotjames imnotjames Nov 23, 2023

Choose a reason for hiding this comment

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

Yup, seems fine with that patch, awesome. Removed that change from this PR.

@peterhinch
Copy link
Contributor

MicroPython doesn't support any kind of callback in asyncio. Instead you can wait on the task to complete then run the required code.

Definitely +1.

@imnotjames
Copy link
Author

imnotjames commented Nov 22, 2023

Thanks for the contribution.

Of course! :)

MicroPython aims to be compact so it can run on devices without many resources. As such, these kinds of methods/functions were left out of the asyncio implementation on purpose (among many others).

Compact is a great goal, and I do recognize that a lot of effort was put in to make asyncio as small as possible -- and also a lot of effort to make the compatibility with the CPython asyncio implementation as high as possible within that constraint.

That said, we can add them if they are important and useful.

What motivated you to add these particular features? Do they add anything that you can't already do?

All of these features expose existing "private" APIs (plumbing?) via the CPython public API (porcelain?). So everything can technically be done today - but in a way that's more brittle.

In particular, this PR heavily simplifies understanding if a task is cancelled or not, and if it's "done" what the result of it running was. It's not documented anywhere how the state or data properties works and I think that's intentional - it's internal aspects of the implementation that work well but shouldn't be touched by other parties.

The use case I had for this was to implement the wait and other waiting primitives in CPython as well as Task groups via these kinds of features.

The example CPython gives for tasks groups is as such:

async with asyncio.TaskGroup() as tg:
  task1 = tg.create_task(some_coro(...))
  task2 = tg.create_task(another_coro(...))
print(f"Both tasks have completed now: {task1.result()}, {task2.result()}")

Where the result is retrieved via task.result()

I don't think the done-callback functions are really needed. MicroPython doesn't support any kind of callback in asyncio.

I had added them because micropython kind of supports them via state (if you squint hard enough) but it's not clear without looking at the internals that they work that way, nor the rules you would need to follow so as not to break the standard async/await functionality. This encodes some of those rules into the add_done_callback and remove_done_callback methods.

Instead you can wait on the task to complete then run the required code.

If you await the task you'll get them serially rather than the result in parallel. The example case I have is supporting the waiting primitives on multiple tasks with the FIRST_COMPLETED or FIRST_EXCEPTION option.

Signed-off-by: James Ward <james@notjam.es>
@peterhinch
Copy link
Contributor

For task groups see #8791. A very useful enhancement.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants