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

Select kernel based on metadata in notebook #14217

Merged
merged 3 commits into from
Oct 2, 2020

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Oct 2, 2020

For #14213
Need to fix the tests

@@ -25,8 +25,6 @@ import {

// Helper functions for dealing with kernels and kernelspecs

export const defaultKernelSpecName = 'python_defaultSpec_';
Copy link
Author

Choose a reason for hiding this comment

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

Removed this, not required.

@@ -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)) {
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

@@ -69,7 +69,22 @@ export function updateNotebookMetadata(
kernelConnection && kernelConnectionMetadataHasKernelModel(kernelConnection)
? kernelConnection.kernelModel
: kernelConnection?.kernelSpec;
if (kernelSpecOrModel && !metadata.kernelspec) {
if (kernelConnection?.kind === 'startUsingPythonInterpreter') {
Copy link
Author

Choose a reason for hiding this comment

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

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

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

sonarcloud bot commented Oct 2, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@DonJayamanne DonJayamanne changed the title Fix picking kernel based on metadata Select kernel based on metadata in notebook Oct 2, 2020
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2020

Codecov Report

Merging #14217 into main will decrease coverage by 0.00%.
The diff coverage is 34.61%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
...rc/client/datascience/notebookStorage/baseModel.ts 62.16% <0.00%> (-0.86%) ⬇️
...client/datascience/kernel-launcher/kernelFinder.ts 74.86% <40.00%> (-0.28%) ⬇️
src/client/datascience/jupyter/kernels/helpers.ts 33.63% <100.00%> (-0.60%) ⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts 62.38% <100.00%> (ø)
...ient/datascience/jupyter/kernels/kernelSwitcher.ts 76.59% <100.00%> (+2.12%) ⬆️
src/client/common/utils/platform.ts 68.00% <0.00%> (-4.00%) ⬇️
src/client/pythonEnvironments/index.ts 17.07% <0.00%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a83c93d...3a6cbb0. Read the comment docs.

@DonJayamanne DonJayamanne merged commit 74a4d99 into microsoft:main Oct 2, 2020
@DonJayamanne DonJayamanne deleted the fixKernelSave branch October 2, 2020 17:48
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants