Select kernel based on metadata in notebook #14217
Merged
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
requested review from
IanMatthewHuff,
greazer,
rchiodo,
DavidKutu and
joyceerhl
DonJayamanne
commented
Oct 2, 2020
| @@ -25,8 +25,6 @@ import { | |||
|
|
|||
| // Helper functions for dealing with kernels and kernelspecs | |||
|
|
|||
| export const defaultKernelSpecName = 'python_defaultSpec_'; | |||
There was a problem hiding this comment.
Removed this, not required.
DonJayamanne
commented
Oct 2, 2020
| @@ -70,35 +69,32 @@ export class KernelFinder implements IKernelFinder { | |||
| const kernelName = kernelSpecMetadata?.name; | |||
|
|
|||
| if (kernelSpecMetadata && kernelName) { | |||
| // For a non default kernelspec search for it | |||
| if (!kernelName.includes(defaultKernelSpecName)) { | |||
There was a problem hiding this comment.
Not required, its not possible for us to get to this state.
Having the condition doesn't hurt, but its invalid code.
There was a problem hiding this comment.
Why is it not possible for the kernel spec to not start off as the default one? What if it comes from an external notebook?
There was a problem hiding this comment.
This method gets executed only in KernelSelector and kernel selector first searches for an interpreter based on metadata in notebook, if not found then it comes in here.
At that point, its impossible for the metadata to contain the name of a default kernel spec.
DefaultKernelSpec is something we generate, and that happens well after searching for a kernel.
Hence this code was not necessary.
DonJayamanne
commented
Oct 2, 2020
| @@ -69,7 +69,22 @@ export function updateNotebookMetadata( | |||
| kernelConnection && kernelConnectionMetadataHasKernelModel(kernelConnection) | |||
| ? kernelConnection.kernelModel | |||
| : kernelConnection?.kernelSpec; | |||
| if (kernelSpecOrModel && !metadata.kernelspec) { | |||
| if (kernelConnection?.kind === 'startUsingPythonInterpreter') { | |||
There was a problem hiding this comment.
This is the fix.
We were doing this too late. This check needs to be done first.
DonJayamanne
added
no-changelog
|
Kudos, SonarCloud Quality Gate passed!
|
DonJayamanne
changed the title
Codecov Report
@@ Coverage Diff @@
## main #14217 +/- ##
==========================================
- Coverage 59.96% 59.95% -0.01%
==========================================
Files 706 706
Lines 39182 39184 +2
Branches 5683 5681 -2
==========================================
- Hits 23494 23492 -2
- Misses 14451 14455 +4
Partials 1237 1237
Continue to review full report at Codecov.
|
DavidKutu
approved these changes
Oct 2, 2020
rchiodo
approved these changes
Oct 2, 2020
DonJayamanne
added a commit
that referenced
this pull request
Oct 2, 2020
* Fix picking kernel based on metadata
DonJayamanne
added a commit
that referenced
this pull request
Oct 2, 2020
rchiodo
added a commit
that referenced
this pull request
Oct 7, 2020
* Port ipykernel install fix to release (#13975) * Fix installing ipykernel into interpreters for raw kernel (#13959) * update news Co-authored-by: Ian Huff <ianhuff@CEIDCCEVIPSVC01.redmond.corp.microsoft.com> * Merge in changes to release (#13976) * Up release version for new release (#13928) * Up release version * Update changelog * Update changelog * Workaround test issue (#13930) * Try different version of VS code in release * Change to make it use the actual variable * Use a real version * More tests failing with gpu error (#13935) * Try different version of VS code in release * Change to make it use the actual variable * Use a real version * Two more version changes * Fix kernel and server name missing in certain situations (#13974) * Fix kernel name and server name * Fixup server name for remote situations * Add some functional tests * Add news entry * Delete news file * Port two fixes to the release branch (#13995) * Disable split views of custom editors (#13985) * Fix backup storage by looking at the options correctly (#13983) * Fix backup storage by looking at the options correctly * Fix backup by being more explicit * Only linux tests are failing. Hopefully fix them * Fixup changelog Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com> * add jedi-language-server to 3rd party notices (#13977) * add jedi-language-server to 3rd party notices * move license from distribution to repository file * disable test_discover_complex_default and (#14024) test_discover_complex_doctest * Upgrade isort to 5.5.3 (#14035) (#14037) * prepare release (#14042) * fixed annoying warnings (#14049) * Cherry pick to address path issues. (#14125) * Do not quote isolated in exec module (#14108) * Do not quote isolated in exec module * Revert "Do not quote isolated in exec module" This reverts commit b9fa04c. * Revert "IPyKernel install issue with windows paths (#13667)" This reverts commit 23725ab. * Fix unit test broken by recent revert (#14122) * Port escape fix to release branch (#14133) * Fix HTML escaping to match what Jupyter does (#14038) * Basic idea * add some functional tests * Add news entry * Fix functional tests * Update changelog * update version and changelog (#14139) * Escaping fix broke a number of things (#14145) (#14154) * Fixes for escaping * Push a comment ot start PR again * Cache task is failing * Remove cache task * Not fixing so just put back cache task * Port NB Convert Fix to point release branch (#14200) * Port escape fix to release branch (#14202) * A different way of fixing escaping (#14186) * Move escaping to just output * Add some tests to verify round tripping * Fixup test for rountrip and make roundtripping actually work * Add news entry * Add to manual test file * Fix streaming problem and add more to the test * Fix traceback unit test * Fix problem caught by functional tests :) * Another functional test catch * Update changelog * Port interactive window export fix (#14232) * Select kernel based on metadata in notebook (#14217) (#14234) * Fix picking kernel based on metadata * Port more escape fixes to point release (#14242) * Fix two problems with escaping (#14228) * Fixup changelog * Port prune fix from main to release (#14243) * Remove unneeded cell keys when exporting (#14241) * Remove transient output when exporting from the interactive window * Add news entry * Update changelog * Merge fixes related to remembering interpreter (#14270) * update version, changelog and thrid party notices (#14280) * remove VSC_PYTHON_CI_TEST_VSC_CHANNEL and test skips * delete ipynb file * delete solved news files * delete more news Co-authored-by: Ian Huff <ianhu@microsoft.com> Co-authored-by: Ian Huff <ianhuff@CEIDCCEVIPSVC01.redmond.corp.microsoft.com> Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com> Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com> Co-authored-by: Kartik Raj <karraj@microsoft.com> Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
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.
For #14213
Need to fix the tests