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

gh-96377: Update asyncio policy doc intro paras to be clear and accurate #97603

Merged
merged 3 commits into from
Sep 27, 2022

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Sep 27, 2022

As reported in #96377 , the introductory paragraphs of the Asyncio Policies doc are confusing and not particularly accurate. Therefore, as discussed there, I've rewritten them to better reflect and explain the current reality, per the guidance of @gvanrossum . I've also added refs to the various relevant sections as they are mentioned, and also linked the doc explaining what an event loop is to begin with when mentioned.

To note, the previous asyncio-event-loop ref target label actually pointed to the table of low-level event loop methods, instead of pointing to the top-level summary of what an event loop actually is as you'd expect and also what most actual usages treated it as, which was in fact missing a ref target label entirely. Therefore, I moved it up a bit to point to that top-level document heading so I could use such, and added a new asyncio-event-loop-methods ref pointing specifically to the section with the table, updating the few instances that actually did treat the previous as the methods table to point to that instead.

Preview:

image

that can override these behaviors.

The :ref:`policy object <asyncio-policy-objects>`
gets and sets a separate event loop per *context*.
Copy link
Member

Choose a reason for hiding this comment

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

Should/could context link to anything?
It's only mentioned here and in a couple of methods below, but it's otherwise not defined in this page.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same question and asked Guido that in #96377 and it seems there isn't really anything more to link it to; see his reply for more details.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add -- in a separate PR -- a concise list of asyncio-related terms/concepts (coroutines, tasks, futures, event loops, transports, contexts, policies, ...).

Copy link
Member Author

Choose a reason for hiding this comment

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

If we did so as a proper Sphinx glossary, they could be easily referenced with, e.g. :term:`coroutine` from anywhere (including via intersphinx).

Copy link
Member

Choose a reason for hiding this comment

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

Good luck defining context. It has at least four unrelated uses/meanings:

  • context manager, e.g. TaskGroup and timeout; see also contextlib
  • a collection of context variables (see contextvars) used to pass contextual state along to child tasks (e.g. create_task(..., context=...))
  • a dict with assorted things passed to exception handlers
  • The context referred to by the docs for event loop policies

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I think this is ready to land. We should probably backport it generously.

@gvanrossum gvanrossum added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 27, 2022
@gvanrossum gvanrossum merged commit cc0f3a1 into python:main Sep 27, 2022
@miss-islington
Copy link
Contributor

Thanks @CAM-Gerlach for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-97604 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 27, 2022
@bedevere-bot
Copy link

GH-97605 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2022
… accurate (pythonGH-97603)

Also fix up some cross-references in the asyncio docs.
(cherry picked from commit cc0f3a1)

Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2022
… accurate (pythonGH-97603)

Also fix up some cross-references in the asyncio docs.
(cherry picked from commit cc0f3a1)

Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington added a commit that referenced this pull request Sep 28, 2022
…ate (GH-97603)

Also fix up some cross-references in the asyncio docs.
(cherry picked from commit cc0f3a1)

Co-authored-by: C.A.M. Gerlach <[email protected]>
miss-islington added a commit that referenced this pull request Sep 28, 2022
…ate (GH-97603)

Also fix up some cross-references in the asyncio docs.
(cherry picked from commit cc0f3a1)

Co-authored-by: C.A.M. Gerlach <[email protected]>
@CAM-Gerlach
Copy link
Member Author

We should probably backport it generously.

Oops, I forgot to add the backport tags, sorry. Thanks for taking care of that.

By using a custom event loop policy, the behavior of
:func:`get_event_loop`, :func:`set_event_loop`, and
:func:`new_event_loop` functions can be customized.
An event loop policy is a global (per-interpreter) object
Copy link
Contributor

Choose a reason for hiding this comment

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

An event loop policy is a global (per-interpreter)

I know the details here are hard to understand but "per-interpreter" can give false sense that asyncio can work under multiple interpreters. It doesn't since it caches the thread id and does not checks the interp id. It would be better to remove per-interpreter altogether here.

See #91375

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're right, I remember now. We should add a note to that issue to add "per-interpreter" back here once that's been fixed. (It's not rocket science to fix it, IIRC, just work?)

Copy link
Member

Choose a reason for hiding this comment

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

Oh and since this is already merged we'd need a new PR. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay done #97618

pablogsal pushed a commit that referenced this pull request Oct 22, 2022
…ate (GH-97603)

Also fix up some cross-references in the asyncio docs.
(cherry picked from commit cc0f3a1)

Co-authored-by: C.A.M. Gerlach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants