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-38260: Add Docs on asyncio.run #16337

Merged
merged 3 commits into from Sep 25, 2019
Merged

Conversation

eamanu
Copy link
Contributor

@eamanu eamanu commented Sep 23, 2019

Add docs about return and raise exception on asyncio.run

https://bugs.python.org/issue38260

Automerge-Triggered-By: @asvetlov

Add docs about return and raise exception on asyncio.run
@@ -225,6 +225,19 @@ Running an asyncio Program
the end. It should be used as a main entry point for asyncio
programs, and should ideally only be called once.

Return the `base_events.run_until_complete()` that this return the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to mention a future and run_until_complete() here.
asyncio.run() returns a result of coro execution, that's it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @asvetlov Thanks for the review. I make the changes

@brandtbucher brandtbucher added needs backport to 3.7 needs backport to 3.8 only security fixes docs Documentation in the Doc dir labels Sep 23, 2019
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

I left some comments for you.
Please take a look

@@ -225,6 +225,18 @@ Running an asyncio Program
the end. It should be used as a main entry point for asyncio
programs, and should ideally only be called once.

Return a result of *coro* execution, or raise a RuntimeError if
Copy link
Member

Choose a reason for hiding this comment

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

RuntimeError should be

:exc:`RuntimeError`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I change it

@@ -225,6 +225,18 @@ Running an asyncio Program
the end. It should be used as a main entry point for asyncio
programs, and should ideally only be called once.

Return a result of *coro* execution, or raise a RuntimeError if
``asyncio.run()`` is called from a running event loop, or a
ValueError if *coro* is not a courutine.
Copy link
Member

@corona10 corona10 Sep 24, 2019

Choose a reason for hiding this comment

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

This also should be

:exc:`ValueError`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change it too. thanks!

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.

Thanks!

@miss-islington
Copy link
Contributor

@eamanu: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 17deb16 into python:master Sep 25, 2019
@miss-islington
Copy link
Contributor

Thanks @eamanu for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-16379 is a backport of this pull request to the 3.8 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Sep 25, 2019
@bedevere-bot
Copy link

GH-16380 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 25, 2019
Add docs about return and raise exception on asyncio.run

https://bugs.python.org/issue38260

Automerge-Triggered-By: @asvetlov
(cherry picked from commit 17deb16)

Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
miss-islington added a commit that referenced this pull request Sep 25, 2019
Add docs about return and raise exception on asyncio.run

https://bugs.python.org/issue38260

Automerge-Triggered-By: @asvetlov
(cherry picked from commit 17deb16)

Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
miss-islington added a commit that referenced this pull request Sep 25, 2019
Add docs about return and raise exception on asyncio.run

https://bugs.python.org/issue38260

Automerge-Triggered-By: @asvetlov
(cherry picked from commit 17deb16)

Co-authored-by: Emmanuel Arias <emmanuelarias30@gmail.com>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian PGO 3.8 has failed when building commit 4633355.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/220/builds/371) and take a look at the build logs.
  4. Check if the failure is related to this commit (4633355) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/220/builds/371

Failed tests:

  • test_venv

Failed subtests:

  • test_with_pip - test.test_venv.EnsurePipTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

408 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 37 sec
  • test_multiprocessing_spawn: 1 min 51 sec
  • test_multiprocessing_forkserver: 1 min 14 sec
  • test_multiprocessing_fork: 1 min 6 sec
  • test_asyncio: 49 sec 744 ms
  • test_tokenize: 44 sec 417 ms
  • test_io: 36 sec 477 ms
  • test_subprocess: 34 sec 233 ms
  • test_lib2to3: 28 sec 560 ms
  • test_nntplib: 25 sec 21 ms

1 test failed:
test_venv

14 tests skipped:
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_ossaudiodev test_startfile test_tix test_tk test_ttk_guionly
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_venv

Total duration: 10 min 15 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 490, in test_with_pip
    self.do_test_with_pip(False)
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 438, in do_test_with_pip
    self.fail(msg.format(exc, details))
AssertionError: Command '['/tmp/tmpthuhiyk0/bin/python', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 430, in do_test_with_pip
    self.run_with_capture(venv.create, self.env_dir,
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 76, in run_with_capture
    func(*args, **kwargs)
subprocess.CalledProcessError: Command '['/tmp/tmpthuhiyk0/bin/python', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 430, in do_test_with_pip
    self.run_with_capture(venv.create, self.env_dir,
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 76, in run_with_capture
    func(*args, **kwargs)
subprocess.CalledProcessError: Command '['/tmp/tmpkxolwwyz/bin/python', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 490, in test_with_pip
    self.do_test_with_pip(False)
  File "/var/lib/buildbot/slaves/enable-optimizations-bot/3.8.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 438, in do_test_with_pip
    self.fail(msg.format(exc, details))
AssertionError: Command '['/tmp/tmpkxolwwyz/bin/python', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.

@@ -225,6 +225,18 @@ Running an asyncio Program
the end. It should be used as a main entry point for asyncio
programs, and should ideally only be called once.

Return a result of *coro* execution, or raise a :exc:`RuntimeError`
if ``asyncio.run()`` is called from a running event loop, or a
:exc:`ValueError` if *coro* is not a courutine.
Copy link
Contributor

@aeros aeros Sep 25, 2019

Choose a reason for hiding this comment

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

Thanks for the adding the additional information @eamanu, but I noticed a couple of issues with the section added.

The more apparent one was a spelling typo, "courutine" should be "coroutine" (misspelled here and in the docstring). Also, it's helpful to link to the glossary using:

:term:`coroutine`

Also, while this section is helpful (particularly with specifying the RuntimeError and ValueError scenarios), I think it could be phrased significantly better. I would suggest the following changes:

Within the new event loop, *coro* is executed, returning the result. If
``asyncio.run()`` is called within a running event loop, a :exc:`RuntimeError`
is raised. If *coro* is not a coroutine, a :exc:`ValueError` is raised.

Personally, I think it's especially important to phrase this section of the documentation as well as possible, since it's likely one of the most read sections for the entire asyncio module.

Let me know what you think @asvetlov and @1st1. If you approve of the suggestions, I'll open a new PR that makes the changes I mentioned above in the docs for ascynio.run() and in the docstring as well (since this one was already merged and backported).

Grammar tip: When chaining together multiple instances of "or", each of the items should be separated by a comma, but only the last one is supposed to have the "or". For example: "item1, item2, or item3" instead of "item1, or item2, or item3". The section added to the documentation in this PR used the latter.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow "Within the new event loop, coro is executed, returning the result." -- I can't parse it. I'd keep the original one "Return the result of coro execution."

IMO it's not necessary to document RuntimeError and ValueError; "This function cannot be called when another asyncio event loop is running in the same thread." already covers that.

Copy link
Contributor

@aeros aeros Sep 27, 2019

Choose a reason for hiding this comment

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

@1st1 Okay, I'll make the suggested changes in #16403.

Edit: Actually, I think specifying that ValueError is raised when something other than a coroutine is passed might be useful (for basic debugging purposes). Do you think that can remain since it's not covered by the existing section?

@aeros
Copy link
Contributor

aeros commented Sep 25, 2019

Also in case it wasn't obvious, the test_venv failure is completely unrelated to the PR. This failure has been happening intermittently for the buildbots recently. I believe a similar failure with test_venv recently happened in a PR from @corona10 and he created a bpo issue for it.

@asvetlov
Copy link
Contributor

PR with improvement is welcome! @aeros167 your proposals make sense

@aeros
Copy link
Contributor

aeros commented Sep 25, 2019

Sounds good! I'll open one soon.

Edit: The changes are applied in #16403.

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
Add docs about return and raise exception on asyncio.run





https://bugs.python.org/issue38260



Automerge-Triggered-By: @asvetlov
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.

None yet

9 participants