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

bpo-34831: Asyncio tutorial #9748

Closed
wants to merge 31 commits into from
Closed

Conversation

cjrh
Copy link
Contributor

@cjrh cjrh commented Oct 7, 2018

@1st1
Copy link
Member

1st1 commented Oct 7, 2018

cc @ambv -- Łukasz, would be great if you could take a look at this too and share some of your thoughts/experience.

@1st1
Copy link
Member

1st1 commented Oct 7, 2018

also @elprans please take a look.

@cjrh cjrh force-pushed the bpo-34831-asyncio-tutorial branch from a7a9f25 to 4b47460 Compare October 12, 2018 14:34
Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @cjrh. A few comments so far.

Doc/library/asyncio-tutorial/async-functions.rst Outdated Show resolved Hide resolved
Doc/library/asyncio-tutorial/index.rst Show resolved Hide resolved
@cjrh cjrh force-pushed the bpo-34831-asyncio-tutorial branch from 9f21e81 to 30d158d Compare October 21, 2018 04:15
@tirkarthi
Copy link
Member

Docs failure could be due to recent logging related changes to fix Sphinx deprecation warnings in ee171a2

@tirkarthi
Copy link
Member

Upstream issue : https://bugs.python.org/issue35036
PR : #10024

Running make suspicious with the fix locally for reference :

(venv) ➜  Doc git:(0e82bc95f2) ✗ make suspicious
mkdir -p build
Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b suspicious -d build/doctrees -D latex_elements.papersize=  . build/suspicious
Running Sphinx v1.8.1
loading pickled environment... done
loading ignore rules... done, 343 rules loaded
building [mo]: targets for 0 po files that are out of date
building [suspicious]: targets for 484 source files that are out of date
updating environment: [] 0 added, 11 changed, 0 removed
reading sources... [100%] whatsnew/changelog
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [ 30%] library/asyncio-tutorial/case-study-chat-server
/Users/karthikeyansingaravelan/stuff/python/cpython/Doc/venv/lib/python3.7/site-packages/sphinx/application.py:388: RemovedInSphinx20Warning: app.warning() is now deprecated. Use sphinx.util.logging instead.
  RemovedInSphinx20Warning)
writing output... [100%] whatsnew/index
WARNING: [library/asyncio-tutorial/case-study-chat-server:49] ":TODO" found in " (ref:TODO) to create a TCP server very"
build finished with problems, 1 warning.
make[1]: *** [build] Error 1
Suspicious check complete; look for any errors in the above output or in build/suspicious/suspicious.csv.  If all issues are false positives, append that file to tools/susp-ignored.csv.
make: *** [suspicious] Error 1

@cjrh cjrh force-pushed the bpo-34831-asyncio-tutorial branch from a6668dd to 7c95086 Compare November 4, 2018 05:52
@cjrh cjrh force-pushed the bpo-34831-asyncio-tutorial branch from 7c95086 to 8c2f3ff Compare June 15, 2019 03:30
@willingc
Copy link
Contributor

Hi @cjrh, Thanks for updating the PR. I will do my best to review it this week. Hope all is well in your world. ☀️

@1st1
Copy link
Member

1st1 commented Jun 16, 2019

Yeah, it's a huge improvement; I'll also try to find time to review this before 3.8 final.

@JulienPalard
Copy link
Member

The unreadable build failure is due to Sphinx deprecating a function, I'm currently fixing it, but in the meantime here's the error you have to fix (or add to Doc/tools/susp-ignored.csv):

WARNING: [distutils/examples:267] "`" found in "This is the description of the ``foobar`` package."
WARNING: [library/asyncio-tutorial/running-async-functions:131] "`" found in "    task1 = func1()  // In Python: `task1 = create_task(func1())`"
WARNING: [library/asyncio-tutorial/running-async-functions:131] "`" found in "    task2 = func2()  // In Python: `task2 = create_task(func2())`"
WARNING: Found 1/355 unused rules:
distutils/examples,,`,This is the description of the ``foobar`` package.

@JulienPalard
Copy link
Member

@cjrh You can now rebase on top of master, since e1786b5 the suspicious.py script is fixed.

@cjrh
Copy link
Contributor Author

cjrh commented Sep 10, 2019

Thanks for the update, I'll rebase that soon.

@1st1
Copy link
Member

1st1 commented Sep 12, 2019

This is huge, thank you, @cjrh!

I'm a bit busy at the sprints right now, I'll review this soon.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Very impressive.
Please use a new stream API instead of legacy reader/writer.

Async Functions, And Other Syntax Features
==========================================

Regular Python functions are created with the keyword ``def``,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Regular Python functions are created with the keyword ``def``,
Regular Python functions are created with the keyword :keyword:`def`,

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushing :keyword: everywhere for building a link doesn't make sense but the first occurrence in a page can be a link, it's convenient maybe.

asyncio.run(f(1, 2))

The first difference is that the function declaration is spelled
``async def``. The second difference is that async functions cannot be
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``async def``. The second difference is that async functions cannot be
:keyword:`async def`. The second difference is that async functions cannot be


The first difference is that the function declaration is spelled
``async def``. The second difference is that async functions cannot be
executed by simply evaluating them. Instead, we use the ``run()`` function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
executed by simply evaluating them. Instead, we use the ``run()`` function
executed by simply evaluating them. Instead, we use the :func:`~asyncio.run` function

The first difference is that the function declaration is spelled
``async def``. The second difference is that async functions cannot be
executed by simply evaluating them. Instead, we use the ``run()`` function
from the ``asyncio`` module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from the ``asyncio`` module.
from the :mod:`asyncio` module.

-------------------------

The ``run`` function is only good for executing an async function
from "synchronous" code; and this is usually only used to execute
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from "synchronous" code; and this is usually only used to execute
from *synchronous* code; and this is usually only used to execute

Using A Queue To Move Data Between Long-Lived Tasks
---------------------------------------------------

TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

Best Practices For Timeouts
---------------------------

- start with ``asyncio.wait_for()``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- start with ``asyncio.wait_for()``
- start with :func:`asyncio.wait_for()`

---------------------------

- start with ``asyncio.wait_for()``
- also look at ``asyncio.wait()``, and what to do if not all tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- also look at ``asyncio.wait()``, and what to do if not all tasks
- also look at :func:`asyncio.wait()`, and what to do if not all tasks

--------------------------

- app shutdown
- how to handle CancelledError and then close sockets
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- how to handle CancelledError and then close sockets
- how to handle :exc:`~asyncio.CancelledError` and then close sockets

...

async def main():
server = await asyncio.start_server(
Copy link
Contributor

Choose a reason for hiding this comment

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

New API:

async with asyncio.StreamServer(client_connected_cb, 'localhost', 9011) as server:
    await server.serve_forever()

@bedevere-bot
Copy link

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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@asvetlov
Copy link
Contributor

I suggest splitting this combo PR into smaller sections.
For example, async_functions.rst is finished and almost ready for merging after a little polishing.
It makes a value in itself, we can process it fast.

@cjrh cjrh mannequin mentioned this pull request Apr 17, 2022
@kumaraditya303
Copy link
Contributor

I am closing this as this will be split into smaller PRs as discussed in the issue. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir topic-asyncio
Projects
None yet
Development

Successfully merging this pull request may close these issues.