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 startup Errors in cell output for more visibility #8304

Merged
merged 4 commits into from
Nov 19, 2021

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Nov 18, 2021

For #7902
For #8351
image

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #8304 (b348eea) into main (35709dc) will increase coverage by 0%.
The diff coverage is 53%.

@@          Coverage Diff           @@
##            main   #8304    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        371     372     +1     
  Lines      23075   23193   +118     
  Branches    3512    3542    +30     
======================================
+ Hits       16649   16743    +94     
- Misses      4985    4996    +11     
- Partials    1441    1454    +13     
Impacted Files Coverage Δ
...ent/datascience/interactive-common/linkProvider.ts 100% <ø> (ø)
...atascience/interactive-window/interactiveWindow.ts 69% <ø> (+6%) ⬆️
...ce/interactive-window/interactiveWindowProvider.ts 75% <ø> (ø)
src/client/datascience/jupyter/kernels/types.ts 100% <ø> (ø)
src/client/datascience/types.ts 100% <ø> (ø)
...rc/client/datascience/errors/errorRendererComms.ts 34% <34%> (ø)
...ent/datascience/jupyter/kernels/kernelExecution.ts 66% <50%> (-1%) ⬇️
...lient/datascience/kernel-launcher/kernelProcess.ts 71% <50%> (-2%) ⬇️
src/client/datascience/errors/errorHandler.ts 66% <61%> (+1%) ⬆️
.../datascience/jupyter/kernels/cellExecutionQueue.ts 94% <100%> (+<1%) ⬆️
... and 25 more

@inject(IApplicationShell) private readonly applicationShell: IApplicationShell
) {}

activate(): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved out of interactive window class, this is not specific to just interactive window anymore.

@DonJayamanne DonJayamanne marked this pull request as ready for review November 18, 2021 21:46
@DonJayamanne DonJayamanne requested a review from a team as a code owner November 18, 2021 21:46
errorMessage = errorMessage.replace(matches[0], `<a href='${matches[2]}'>${matches[1]}</a>`);
}
}
const execution = controller.controller.createNotebookCellExecution(cellToDisplayErrors);
Copy link
Member

Choose a reason for hiding this comment

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

I know this is how the API works, just feels odd that we have to do all this just to change the output. Though I guess it does ensure that everything runs through the execution control path.

@DonJayamanne DonJayamanne merged commit f3f77cf into main Nov 19, 2021
@DonJayamanne DonJayamanne deleted the errorsInOutput branch November 19, 2021 00:09
// Then display the error in the cell.
const stopWatch = new StopWatch();
while (stopWatch.elapsedTime <= 1_000 && associatedkernel.hasPendingCells) {
await sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to interrupt the current execution instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it will stop. The existing code will still the execution.
The problem is this code gets called just when kernel dies, hence the cells are stored after that, within a few Ms.
Hence the max delay of 1s.

I wish the api allowed us to update cell output without all of this api. But these are a few exceptions

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.

4 participants