-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
Improve asyncio.loop.call_soon() documentation #20883
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I can definitely see that it may be a bit unclear for anyone unfamiliar with callbacks, I don't think the documentation for loop.call_soon()
is the best place to define the term. It's a fairly general computer science term and is used in several areas of Python, so I think it would make more sense to define it in the glossary, and link to it the first time the term is used in loop.call_soon()
. You can link to a term in the glossary w/ inline Sphinx markup by using the directive :term:
. E.g.
:term:`callback`
Also, the definition "a standard Python function" is not entirely wrong per say, but it would be significantly more descriptive and technically accurate to specify that it's:
"A function which is passed as an argument to be executed at a given point in time. Callbacks can be either synchronously or asynchronously executed, depending on whether they are called before or after the result of the function is available."
This is significantly different from just passing a "Python function", as that can also apply to arguments that are not callbacks such as target for threading.Thread()
or func in functools.partial()
. Those can be potentially used in the process of implementing a callback, but the argument passed to target or func are not considered callbacks in that context.
Would you be interested in working on this @rogerthat94? If not, I should be able to find the time to do it in the relatively near future.
@1st1 As the primary maintainer of the asyncio docs, do you think something like the above would be a reasonable inclusion?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I've updated the title to more specifically refer to the section of the asyncio documentation that an improvement is being suggested for. |
@aeros I would be interested in working on this. However, I think you might be misunderstanding what I was confused about when I first read these documents. It was not the definition of the word "callback." I already knew of a callback as "a function that gets executed after an asynchronous event is finished." However, I don't think understanding what a callback is makes it clear that passing a coroutine to Could you give me an example of a Python function that should not be passed to |
I'm personally not convinced that it's worth mentioning in this case, since a callback is not a coroutine. The reason being that when you call a coroutine function, it returns a coroutine object (instead of the result of the function), which itself is not callable. Instead, it has to be awaited or an using a function like asyncio.run() (or alternatives such as TypeError: 'coroutine' object is not callable If a user tries to pass the coroutine function (without calling it), they'll receive a warning since the coroutine object created is never awaited: /usr/lib/python3.8/asyncio/events.py:81: RuntimeWarning: coroutine 'coro' was never awaited In general in the documentation, we try to avoid explicitly mentioning every non-specific possible error (particularly TypeError: a coroutine was expected, got <Future pending> Ultimately, I think passing a coroutine to Also, this applies even more to the low-level parts of the documentation, as they're really not intended to be used by the most users in the first place, so they tend to be more succinct. In practice, However, that's just my opinion. The final decision to merge or reject would fall upon the primary maintainers of asyncio, Andrew and Yury. I can't say for sure of course, but from working with them in the past, I suspect they would concur with most of the above.
AFAIK, you can pass any arbitrary Python subroutine function to |
@aeros You said a callback is "A function which is passed as an argument to be executed at a given point in time." I don't think this makes it clear that "a callback is not a coroutine." Say I have a function that updates a dict, but does not return anything. I want this to run on the event loop, because other coroutines that run on the event loop also update this dict. It seems reasonable to implement this using a standard python function or a coroutine. This would be a callback either way. Am I missing something here? FWIW passing an async function as a callback in JavaScript works the same way as passing a standard function. The documentation for
Isn't a callback just "any arbitrary function that you want to run after an asynchronous event occurs?" How does defining callback make it clear that a callback shouldn't be defined with "async def?" I understand that |
You're welcome! My hope is to prevent others from getting confused between I hope these new changes will prevent confusion between |
Thanks for explaining your JavaScript background @rogerthat94. I honestly had not considered the difference in semantics between Python's async functions and JS's. I'm somewhat familiar with JS async as well, so I might be able to help clear up some misunderstandings.
Therein lies the difference - an "async def" function specifically returns a coroutine object, which has a difference in semantics from a standard "def" function (which of course just returns the result). In JS, the async function will return a Promise that represents the eventual result (which is more equivalent to a Future in Python), but is otherwise interchangeable with a standard function. The reason for this difference is that in asyncio, we intentionally expose users to the Task-based asynchronous model of programming (similar to C#) rather than the callback model. Most user/high-level APIs in asyncio and async frameworks built ontop of it avoid directly returning futures for that reason. While it's significantly different for those who are more familiar with callbacks, it tends to be much easier to learn for most and avoids the infamous situation of "callback hell" in large codebases that deal with many asynchronous operations. That being said, if you're looking to still use a callback approach because it's more familiar, I would recommend using futures and looking into their API. Although, in my somewhat biased opinion, I think it's worth learning both models so you can choose which one you prefer. Alternatively, there are alternative async frameworks such as Twisted, which revolves around using callbacks in Deferred objects.
If it were specified in the glossary definition that a callback had to specifically be a subroutine function rather than just saying "function" (which could potentially include coroutine functions, e.g.
It's more so "any arbitrary subroutine function that you want to run at a later point in time" rather than necessarily after an asynchronous operation occurs. For calling it after an asynchronous is completed, you'd likely want to use Future.add_done_callback(). This method will end up calling
I would not quite call that section a "beginners" area, that specific section is a bit more intermediate-advanced. The main beginners section is here. That being said, I would definitely approve of linking to the callback definition there as well, which could presumably have "non-coroutine function"/"subroutine function" as part of the definition. I agree that we should aim to be specific about that. |
Doc/library/asyncio-eventloop.rst
Outdated
@@ -191,8 +191,8 @@ Scheduling callbacks | |||
|
|||
.. method:: loop.call_soon(callback, *args, context=None) | |||
|
|||
Schedule a *callback* to be called with *args* arguments at | |||
the next iteration of the event loop. | |||
Schedule a synchronous *callback* to be called with *args* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think this is a substantial improvement over the previous iteration, I would very much prefer to explain that callbacks are subroutine/non-coroutine functions in the Python glossary for "callback", as it universally applies to anything that uses the term in the python docs (including several other areas in the asyncio docs). So rather than repeating it every time we mention callbacks, I think it makes more sense to clarify it in the glossary definition and link to there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to write out the detailed explanation above! I really appreciate it and it was very helpful. Indeed, I think specifying subroutine function, rather than just function would fix the ambiguity for me. Would this pull request be a good place for me to add that to the glossary and link it in these docs? Or should I define coroutine in the glossary in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think that "synchronous callback" might be a bit misleading or cause some confusion, as a callback can definitely be asynchronous depending on how it is used, such as with Future.add_done_callback()
. The function itself passed as a callback is not a coroutine/"async def" function and might be considered "synchronous" in that sense, but the operation is asynchronous since it is happening out of order from the main "flow" of the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to write out the detailed explanation above! I really appreciate it and it was very helpful.
No problem, I'm glad to hear it was helpful. :-)
Indeed, I think specifying subroutine function, rather than just function would fix the ambiguity for me. Would this pull request be a good place for me to add that to the glossary and link it in these docs? Or should I define coroutine in the glossary in a separate PR?
Either way works, but it would probably be easier to do it in the same PR. Just make sure to avoid anything that would require a force-push, since it could overwrite the history of the PR and remove some of the review comments (which loses some context for anyone looking in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I added a glossary definition for callback and referenced it from asyncio-eventloop
and asyncio-dev
.
@@ -191,8 +191,8 @@ Scheduling callbacks | |||
|
|||
.. method:: loop.call_soon(callback, *args, context=None) | |||
|
|||
Schedule a *callback* to be called with *args* arguments at | |||
the next iteration of the event loop. | |||
Schedule the *callback* :term:`callback` to be called with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for asyncio.create_task()
says "Wrap the coro coroutine" instead of "Wrap a coro coroutine."
Closing and re-opening the PR to restart travis-ci. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Yury Selivanov <yury@edgedb.com>
Thanks @rogerthat94 for the PR, and @aeros for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9. |
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit a16d697) Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
GH-21063 is a backport of this pull request to the 3.9 branch. |
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit a16d697) Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
GH-21064 is a backport of this pull request to the 3.8 branch. |
Thanks for the review, Yury. Backporting to 3.9 and 3.8 so it's in the latest versions of the documentation on docs.python.org. |
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit a16d697) Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com> (cherry picked from commit a16d697) Co-authored-by: Roger Iyengar <ri@rogeriyengar.com>
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com>
* Add a glossary entry for the term "callback" * Link to it in loop.call_soon() and in the "Concurrency and Multithreading" section Co-authored-by: Kyle Stanley <aeros167@gmail.com>
I was initially confused about the difference between
loop.call_soon_threadsafe()
andrun_coroutine_threadsafe()
. I believe this change will make this distinction clearer to people who are looking at asyncio for the first time.