Select kernel based on metadata in notebook#14217

DonJayamanne · 2020-10-02T00:52:48Z

For #14213
Need to fix the tests

DonJayamanne · 2020-10-02T00:53:08Z

src/client/datascience/jupyter/kernels/helpers.ts


 // Helper functions for dealing with kernels and kernelspecs

-export const defaultKernelSpecName = 'python_defaultSpec_';


Removed this, not required.

DonJayamanne · 2020-10-02T00:53:53Z

src/client/datascience/kernel-launcher/kernelFinder.ts


        if (kernelSpecMetadata && kernelName) {
-            // For a non default kernelspec search for it
-            if (!kernelName.includes(defaultKernelSpecName)) {


Not required, its not possible for us to get to this state.
Having the condition doesn't hurt, but its invalid code.

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?

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 · 2020-10-02T00:54:21Z

src/client/datascience/notebookStorage/baseModel.ts

            ? kernelConnection.kernelModel
            : kernelConnection?.kernelSpec;
-    if (kernelSpecOrModel && !metadata.kernelspec) {
+    if (kernelConnection?.kind === 'startUsingPythonInterpreter') {


This is the fix.
We were doing this too late. This check needs to be done first.

sonarqubecloud · 2020-10-02T16:06:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

No Coverage information
0.0% Duplication

codecov-commenter · 2020-10-02T16:12:43Z

Codecov Report

❌ Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 59%. Comparing base (a83c93d) to head (3a6cbb0).
⚠️ Report is 2884 commits behind head on main.

Files with missing lines	Patch %	Lines
...client/datascience/kernel-launcher/kernelFinder.ts	40%	8 Missing and 1 partial ⚠️
...rc/client/datascience/notebookStorage/baseModel.ts	0%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #14217   +/-   ##
======================================
- Coverage     59%      59%   -1%     
======================================
  Files        706      706           
  Lines      39182    39184    +2     
  Branches    5683     5681    -2     
======================================
- Hits       23494    23492    -2     
- Misses     14451    14455    +4     
  Partials    1237     1237

Files with missing lines	Coverage Δ
src/client/datascience/jupyter/kernels/helpers.ts	`33% <100%> (-1%)`	⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts	`62% <100%> (ø)`
...ient/datascience/jupyter/kernels/kernelSwitcher.ts	`76% <100%> (+2%)`	⬆️
...rc/client/datascience/notebookStorage/baseModel.ts	`62% <0%> (-1%)`	⬇️
...client/datascience/kernel-launcher/kernelFinder.ts	`74% <40%> (-1%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

* Fix picking kernel based on metadata

* 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>

Fix picking kernel based on metadata

e3c578e

DonJayamanne requested review from DavidKutu, IanMatthewHuff, greazer, joyceerhl and rchiodo October 2, 2020 00:52

DonJayamanne commented Oct 2, 2020

View reviewed changes

Fix tests

7bd1177

DonJayamanne added no-changelog No news entry required and removed no-changelog No news entry required labels Oct 2, 2020

news

3a6cbb0

DonJayamanne changed the title ~~Fix picking kernel based on metadata~~ Select kernel based on metadata in notebook Oct 2, 2020

DavidKutu approved these changes Oct 2, 2020

View reviewed changes

rchiodo approved these changes Oct 2, 2020

View reviewed changes

DonJayamanne merged commit 74a4d99 into microsoft:main Oct 2, 2020

DonJayamanne deleted the fixKernelSave branch October 2, 2020 17:48

DonJayamanne added a commit that referenced this pull request Oct 2, 2020

Select kernel based on metadata in notebook (#14217)

dbbfbfc

* Fix picking kernel based on metadata

DonJayamanne added a commit that referenced this pull request Oct 2, 2020

Select kernel based on metadata in notebook (#14217) (#14234)

8022bfe

* Fix picking kernel based on metadata

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select kernel based on metadata in notebook#14217

Select kernel based on metadata in notebook#14217
DonJayamanne merged 3 commits intomicrosoft:mainfrom
DonJayamanne:fixKernelSave

DonJayamanne commented Oct 2, 2020 •

edited

Loading

Uh oh!

DonJayamanne Oct 2, 2020

Uh oh!

DonJayamanne Oct 2, 2020

Uh oh!

rchiodo Oct 2, 2020

Uh oh!

DonJayamanne Oct 2, 2020

Uh oh!

DonJayamanne Oct 2, 2020

Uh oh!

sonarqubecloud bot commented Oct 2, 2020

Uh oh!

codecov-commenter commented Oct 2, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		// Helper functions for dealing with kernels and kernelspecs

		export const defaultKernelSpecName = 'python_defaultSpec_';

Conversation

DonJayamanne commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DonJayamanne Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

rchiodo Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 2, 2020

Uh oh!

codecov-commenter commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DonJayamanne commented Oct 2, 2020 •

edited

Loading

codecov-commenter commented Oct 2, 2020 •

edited

Loading