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

Display kernel failure msg in last executed cell #9587

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Apr 4, 2022

Fixes #9375

// Also when kernels die, we don't restart automatically with raw kernels,
// We should'nt do the same with jupyter (else startup code will not run).
promisedArgs.push(Promise.resolve('--KernelManager.autorestart=False'));
if (this.context.extensionMode === ExtensionMode.Test) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting this change, else this breaks jupyter.
I.e. if a kernel dies, then we never get a response back for anything, we don't know whether the died or not.
With auto start enabled, jupyter will at a minimum re-start the kernel and we know that it was auto-restarted as the kernel died. Note: Jupyter auto restarts when kernels die.

Hence this change is required (& it was wrong to add)

// & this is very slow (we dont want users to wait because of kernel failures).
// Also when kernels die, we don't restart automatically with raw kernels,
// We should'nt do the same with jupyter (else startup code will not run).
promisedArgs.push(Promise.resolve('--KernelManager.autorestart=False'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting this change, else this breaks jupyter.
I.e. if a kernel dies, then we never get a response back for anything, we don't know whether the died or not.
With auto start enabled, jupyter will at a minimum re-start the kernel and we know that it was auto-restarted as the kernel died. Note: Jupyter auto restarts when kernels die.

Hence this change is required (& it was wrong to add)

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #9587 (991131e) into main (4f138fc) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           main   #9587    +/-   ##
=====================================
  Coverage    72%     73%            
=====================================
  Files       189     193     +4     
  Lines      8563    8451   -112     
  Branches   1261    1238    -23     
=====================================
- Hits       6232    6171    -61     
+ Misses     1848    1807    -41     
+ Partials    483     473    -10     
Impacted Files Coverage Δ
src/platform/common/application/notebook.node.ts 87% <ø> (ø)
src/platform/common/application/types.ts 100% <ø> (ø)
src/platform/common/utils/localize.ts 98% <100%> (+1%) ⬆️
.../platform/common/variables/systemVariables.node.ts 39% <0%> (-11%) ⬇️
src/platform/common/application/workspace.ts 75% <0%> (-4%) ⬇️
src/platform/common/utils/decorators.ts 83% <0%> (-1%) ⬇️
...ommon/process/environmentActivationService.node.ts 69% <0%> (-1%) ⬇️
src/platform/api/types.ts 100% <0%> (ø)
src/platform/common/types.ts 100% <0%> (ø)
src/platform/activation/types.ts 100% <0%> (ø)
... and 60 more

@DonJayamanne DonJayamanne marked this pull request as ready for review April 5, 2022 00:27
@DonJayamanne DonJayamanne requested a review from a team as a code owner April 5, 2022 00:27
private onDidStartKernel(kernel: IKernel) {
this.kernelsStartedSuccessfully.add(kernel);
}
private onKernelStatusChanged({ kernel }: { status: KernelMessage.Status; kernel: IKernel }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remvoed code form here as:

  • progress for restarting kernels has nothing to do with command handler
  • DIsplaying warnings about dead kernels has nothing to do with command handler

As both of the above are two distinct features, i've moved into 2 separate files.

@DonJayamanne DonJayamanne requested review from amunger, IanMatthewHuff and rchiodo and removed request for a team, IanMatthewHuff, amunger and rchiodo April 5, 2022 01:33
@DonJayamanne DonJayamanne marked this pull request as draft April 5, 2022 06:22
@DonJayamanne DonJayamanne marked this pull request as ready for review April 5, 2022 19:26

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we deal with user code (packages) causing kernel failures
3 participants