Revision to displaying cached remote kernel specs and live kernels #9985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DonJayamanne
changed the title
DonJayamanne
force-pushed
the
issue9617Followup
branch
from
e688b28 to
a408ff7
Compare
Codecov Report
@@ Coverage Diff @@
## main #9985 +/- ##
====================================
- Coverage 63% 63% -1%
====================================
Files 209 210 +1
Lines 9948 9970 +22
Branches 1591 1594 +3
====================================
+ Hits 6326 6331 +5
- Misses 3111 3121 +10
- Partials 511 518 +7
|
DonJayamanne
force-pushed
the
issue9617Followup
branch
from
0c939e8 to
c28708f
Compare
rchiodo
reviewed
May 11, 2022
DonJayamanne
commented
May 11, 2022
| @@ -236,11 +234,6 @@ export class VSCodeNotebookController implements Disposable { | |||
| if (cells.length < 1) { | |||
| return; | |||
| } | |||
| if (this.isConnectionOutdated && !this.outdatedMessageDisplayed) { | |||
| void this.appShell.showErrorMessage(DataScience.jupyterRemoteConnectFailedModalMessage(), { modal: true }); | |||
There was a problem hiding this comment.
This is no longer required, we have better ways to handle this now.
There was a problem hiding this comment.
I.e. we let the connection crash and handle the errors in the usual central location errorHandler.ts
DonJayamanne
commented
May 12, 2022
| @@ -137,8 +140,6 @@ export class NotebookControllerManager implements INotebookControllerManager, IE | |||
| @inject(ICommandManager) private readonly commandManager: ICommandManager, | |||
| @inject(IExtensionContext) private readonly context: IExtensionContext, | |||
| @inject(IKernelProvider) private readonly kernelProvider: IKernelProvider, | |||
| @inject(PreferredRemoteKernelIdProvider) | |||
| private readonly preferredRemoteKernelIdProvider: PreferredRemoteKernelIdProvider, | |||
There was a problem hiding this comment.
Removed this as well, as this too could be handled the same way as the new code.
rchiodo
reviewed
May 12, 2022
| @@ -632,9 +600,6 @@ export class VSCodeNotebookController implements Disposable { | |||
| // Before we start the notebook, make sure the metadata is set to this new kernel. | |||
| await updateNotebookDocumentMetadata(document, this.documentManager, selectedKernelConnectionMetadata); | |||
|
|
|||
| if (isLocalConnection(this.connection)) { | |||
There was a problem hiding this comment.
Nice, love the removal of the knowledge of remote.
There was a problem hiding this comment.
Agreed, one less dependency less complex
rchiodo
approved these changes
May 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #9167
The previous implementation was incorrect and as follows:
However the requirement is:
To make this possible we decided to only display the kernels they had used.
The implementation of this logic was incorrect:
Problems
Revised solution