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

Better error handling when kernel crashes due to overridden built in modules #8237

Merged
merged 10 commits into from
Nov 11, 2021

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Nov 10, 2021

Fixes #8195
Fixes #5896
Fixes #6791

  • Display message prompting user to rename file if kernel fails to start due to some funky file in workspace
  • Better handling of cancellation (if user stops kernel startup, then cancel all of the kernel startup code early)
    • Came across this when testing kernel failures (constantly running & causing cells to crash/fail)
    • Now cells stop running immediately when you decide to switch kernels & things are killed much earlier/quicker (previously we'd wait to start the kernel & then kill it)

@@ -11,9 +11,6 @@
# See comment at the point of our use of Popen
from subprocess import Popen, PIPE # nosec

from ipython_genutils.encoding import getdefaultencoding
from ipython_genutils.py3compat import cast_bytes_py2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer required, was only required for python 2.7
Also ipython_genutils is no longer shipping in conda

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #8237 (6660e65) into main (cfa54b2) will increase coverage by 0%.
The diff coverage is 51%.

❗ Current head 6660e65 differs from pull request most recent head 489bdd4. Consider uploading reports for the commit 489bdd4 to get more accurate results

@@          Coverage Diff           @@
##            main   #8237    +/-   ##
======================================
  Coverage     71%     71%            
======================================
  Files        368     369     +1     
  Lines      22613   22751   +138     
  Branches    3423    3460    +37     
======================================
+ Hits       16229   16373   +144     
+ Misses      4986    4952    -34     
- Partials    1398    1426    +28     
Impacted Files Coverage Δ
src/client/common/errors/types.ts 43% <ø> (+17%) ⬆️
...e/jupyter/interpreter/jupyterInterpreterService.ts 89% <ø> (ø)
...tascience/jupyter/kernels/kernelCommandListener.ts 66% <0%> (ø)
...datascience/jupyter/liveshare/hostJupyterServer.ts 62% <ø> (-2%) ⬇️
src/client/datascience/jupyter/notebookStarter.ts 69% <0%> (ø)
src/client/datascience/types.ts 100% <ø> (ø)
src/client/datascience/baseJupyterSession.ts 75% <22%> (+<1%) ⬆️
...lient/datascience/jupyter/kernels/cellExecution.ts 74% <25%> (-1%) ⬇️
...lient/datascience/jupyter/jupyterSessionManager.ts 61% <33%> (+<1%) ⬆️
...client/datascience/raw-kernel/rawJupyterSession.ts 71% <33%> (-2%) ⬇️
... and 42 more

@DonJayamanne DonJayamanne marked this pull request as ready for review November 11, 2021 21:21
@DonJayamanne DonJayamanne requested a review from a team as a code owner November 11, 2021 21:21
@DonJayamanne
Copy link
Contributor Author

Did these all move here from somewhere else?

Moved from errors.ts, but some of the code is new

Is this another goofy spelling difference? Analyze is with a 'z' afaik.

Damn it, sorry, will change.

Probably dont want to submit this. Although I think the test just overwrites it each time.

Oops.

@DonJayamanne DonJayamanne requested a review from rchiodo November 11, 2021 22:24
@DonJayamanne DonJayamanne merged commit 64bb04b into main Nov 11, 2021
@DonJayamanne DonJayamanne deleted the issue8195 branch November 11, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants