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

No prompt to install Python when open an exist nb without Kernel #5202

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

DonJayamanne
Copy link
Contributor

For #4965
If you have a notebook with metadata that has a language of python, but no kernel information, we still prompt to install Python.
Not sure how users can create such notebooks, in our extension users can create such notebooks because we always default language to python for blank notebooks.
This ensures we don't display prompts if there is no kernel information.

@DonJayamanne DonJayamanne requested a review from a team as a code owner March 18, 2021 17:44
@codecov-io
Copy link

codecov-io commented Mar 18, 2021

Codecov Report

Merging #5202 (e18ba58) into main (d99989e) will decrease coverage by 0%.
The diff coverage is 85%.

@@          Coverage Diff          @@
##            main   #5202   +/-   ##
=====================================
- Coverage     73%     73%   -1%     
=====================================
  Files        402     402           
  Lines      26403   26417   +14     
  Branches    3799    3804    +5     
=====================================
+ Hits       19432   19436    +4     
- Misses      5403    5412    +9     
- Partials    1568    1569    +1     
Impacted Files Coverage Δ
...nt/datascience/notebookStorage/vscNotebookModel.ts 77% <ø> (+2%) ⬆️
src/client/datascience/openNotebookBanner.ts 80% <ø> (ø)
.../client/datascience/notebook/kernelWithMetadata.ts 86% <78%> (+<1%) ⬆️
src/client/datascience/jupyter/kernels/helpers.ts 77% <100%> (+<1%) ⬆️
...datascience/notebook/defaultCellLanguageService.ts 97% <100%> (+<1%) ⬆️
src/client/datascience/notebook/helpers/helpers.ts 75% <100%> (-1%) ⬇️
src/client/datascience/data-viewing/dataViewer.ts 72% <0%> (-4%) ⬇️
...datascience/editor-integration/codelensprovider.ts 85% <0%> (-2%) ⬇️
...lient/datascience/kernel-launcher/kernelProcess.ts 79% <0%> (-1%) ⬇️
...lient/datascience/jupyter/kernels/cellExecution.ts 77% <0%> (-1%) ⬇️
... and 8 more

return translateKernelLanguageToMonaco(jupyterLanguage || PYTHON_LANGUAGE);

// Default to python language only if the Python extension is installed.
const defaultLanguage = this.pythonExtensionChecker.isPythonExtensionInstalled ? PYTHON_LANGUAGE : 'plaintext';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this wasn't right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If user doesn't have a python kernel at all, no point defaulting to Python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Else later if you save these notebooks and open it, then we assume its a python notebook and ask the user to install Python.

// All other notebooks are python notebooks.
return true;
// Valid notebooks will have a language information in the metadata.
return kernelSpec?.language === PYTHON_LANGUAGE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This too wasn't right, better now

}
},
this,
handlerDisposables
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better clean up of handlers.

const saveKernelInfo = () => {
const kernelId = kernelSocket?.options.id;
if (!kernelId) {
return;
}
traceInfo(`Updating preferred kernel for remote notebook ${kernelId}`);
this.preferredRemoteKernelIdProvider.storePreferredRemoteKernelId(doc.uri, kernelId).catch(noop);

disposeAllDisposables(handlerDisposables);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong

@@ -83,15 +83,12 @@ export class VSCodeNotebookModel extends BaseNotebookModel {
return this.document ? this.document.cells.length : this.notebookJson.cells?.length ?? 0;
}
public getNotebookData() {
if (!this.preferredLanguage) {
throw new Error('Preferred Language not initialized');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its possible to not have a preferred langauge, we'll default to plain text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: When you create a blank notebook and we return cells with plain text,
& a kernel is selected, then the language of the cells get updated to match the language of the kernel selected.

Tha'ts existing fucntionality.

I.e. if you create a blank notebook and cells are TypeScript, then select Juali kernel, then language of empty cells in blank notebook will change to Julia.

@DonJayamanne DonJayamanne changed the title Do not prompt to install Python when opening an existing notebook without Kernel No prompt to install Python when open an exist nb without Kernel Mar 19, 2021
@DonJayamanne DonJayamanne merged commit fa71bce into main Mar 19, 2021
@DonJayamanne DonJayamanne deleted the noActiavte branch March 19, 2021 16:56
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.

5 participants