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

await _finish_kernel_start #617

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Nov 26, 2021

This is the result of investigating the failing js-services tests in JupyterLab: jupyterlab/jupyterlab#11548

Looking at the code, it looks like this future is ensured with asyncio.ensure_future but never awaited.

@jtpio jtpio added the bug label Nov 26, 2021
@jtpio jtpio added this to the 1.12 milestone Nov 26, 2021
@jtpio
Copy link
Member Author

jtpio commented Nov 26, 2021

cc @blink1073 probably this one needs the await and not be behind the use_pending_kernels check?

@jtpio jtpio marked this pull request as ready for review November 26, 2021 18:25
Copy link
Collaborator

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

We need to add a guard on the use of pending kernels here, otherwise we will block and defeat the purpose of the feature.

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #617 (005e783) into master (e3eec2a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   77.58%   77.62%   +0.04%     
==========================================
  Files         110      110              
  Lines       10402    10404       +2     
  Branches     1287     1288       +1     
==========================================
+ Hits         8070     8076       +6     
+ Misses       1933     1926       -7     
- Partials      399      402       +3     
Impacted Files Coverage Δ
jupyter_server/services/kernels/kernelmanager.py 83.17% <100.00%> (+2.02%) ⬆️
jupyter_server/services/kernels/handlers.py 60.90% <0.00%> (-0.51%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3eec2a...005e783. Read the comment docs.

Copy link
Collaborator

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 309375c into jupyter-server:master Nov 26, 2021
@jtpio jtpio deleted the await-future branch November 26, 2021 23:22
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

3 participants