Check our saved jupyter interpreters before allowing any of them to be used as active interpreters #10113

IanMatthewHuff · 2020-02-13T20:32:18Z

For #9904

Skip news as this is the second PR for this item.

IanMatthewHuff · 2020-02-13T20:34:25Z

src/client/datascience/activation.ts

@@ -28,6 +28,7 @@ export class Activation implements IExtensionSingleActivationService {
    public async activate(): Promise<void> {
        this.disposables.push(this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebookEditor, this));
        this.disposables.push(this.jupyterInterpreterService.onDidChangeInterpreter(this.onDidChangeInterpreter, this));
+        this.testSavedInterpreter();


Note: not sure on the placement here. Maybe at the same time as PreWarmDaemonPool?

I'd like to just make it part of the getSelectedInterpreter. But if I do that, I'm not sure if we are gaining anything from saving it, since we still would verify the first time that we run it and get the slowdown.

Couldn't you also get into a problem here if auto start is turned on? Auto start would use the saved interpreter before you reset it?

In reply to: 379103857 [](ancestors = 379103857)

Maybe have the auto start stuff wait for the check too?

In reply to: 379116829 [](ancestors = 379116829,379103857)

Yeah, I feel like this currently doesn't guarantee a good interpreter. But making things wait for it would reduce / remove the benefit of saving a good server I would think.

Just the auto start case. Normal startup should not wait.

In reply to: 379118437 [](ancestors = 379118437)

To me it makes sense to have it in activate

@rchiodo . How about this. The saved interpreter check is a promise that we save (like ensureNotebook) and we fire it off on activate right away. Then getSelectedInterpreter always awaits on that promise before returning its result. This way we ensure that getSelectedInterpreter is returning something valid for all the notebook launch cases as well as stuff like preWarmVariables and the auto start server. If I do this, by the time the user is actually starting a notebook it should have completed, since it launched on activate so we still get the benefit of the saved server.

Sounds good to me 👍

IanMatthewHuff · 2020-02-13T20:38:42Z

I was starting to put some work in here on figuring out what to do when the saved server is bogus (aside from the message that I already added). I don't believe this is the full solution, but wanted to discuss it a bit.

Some things that I don't like about this idea.

Timing. Lots of different areas access getSelectedInterpreter could something like prewarm already call getSelectedInterpreter before we check it? Or while we are checking it? If we run it at extension load, even as ignoreErrors is that slowing down our load?
Does this actually ensure valid? I don't think that it does. This did do the trick for somethings that I tried, like blowing away python from an enlistment. But might not catch the kernel spec issue or other possible launch failures.

codecov-io · 2020-02-13T20:48:52Z

Codecov Report

Merging #10113 into master will increase coverage by 0.01%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master   #10113      +/-   ##
==========================================
+ Coverage   61.37%   61.39%   +0.01%     
==========================================
  Files         565      567       +2     
  Lines       30559    30729     +170     
  Branches     4385     4404      +19     
==========================================
+ Hits        18757    18867     +110     
- Misses      10823    10878      +55     
- Partials      979      984       +5

Impacted Files	Coverage Δ
src/client/common/process/types.ts	`100% <ø> (ø)`	⬆️
...upyter/interpreter/jupyterInterpreterStateStore.ts	`73.33% <0%> (ø)`	⬆️
src/client/datascience/activation.ts	`94.28% <100%> (+0.16%)`	⬆️
...e/jupyter/interpreter/jupyterInterpreterService.ts	`89.02% <88.37%> (+38.32%)`	⬆️
.../datascience/jupyter/interpreter/jupyterCommand.ts	`50% <0%> (-5.24%)`	⬇️
src/client/common/installer/productInstaller.ts	`92.79% <0%> (-3.64%)`	⬇️
...eter/jupyterCommandInterpreterDependencyService.ts	`54.76% <0%> (-2.39%)`	⬇️
src/client/datascience/jupyter/notebookStarter.ts	`65.25% <0%> (-2.32%)`	⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts	`75.8% <0%> (-1.78%)`	⬇️
...ient/datascience/interactive-ipynb/nativeEditor.ts	`56.31% <0%> (-0.74%)`	⬇️
... and 15 more

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 c8e7e5e...331e7f7. Read the comment docs.

IanMatthewHuff · 2020-02-13T20:59:43Z

Actually this did check and clear on a kernelspec issue.

src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts

rchiodo · 2020-02-13T22:25:21Z

src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts

@@ -64,7 +73,17 @@ export class JupyterInterpreterService {
            return interpreter;
        }

-        const pythonPath = this._selectedInterpreterPath || this.interpreterSelectionState.selectedPythonPath;
+        let pythonPath = this._selectedInterpreterPath;


_selectedInterpreterPath [](start = 30, length = 24)

Seems like you need to check validate regardless? Or is it because _selectedInterpreterPath always starts out as empty on startup?

It seems weird that this check is here at all because it's checked at the beginning of this function. Unless getInterpreterFromChangeOfOlderVersionOfExtension is setting it, but in that case, then you need to validate the old version.

In reply to: 379153900 [](ancestors = 379153900)

Yeah, I think it only gets set once we pass though this function once. I'll triple check.

"it's checked at the beginning of this function" Are you confusing _selectedInterpreterPath with _selectedInterpreter? Only _selectedInterpreter is checked at the start of this function. But looking at it now it looks like getInterpreterFromChangeOfOlderVersionOfExtension does set the path, so I need to validate both cases here not just the saved one.

rchiodo

🕐

rchiodo · 2020-02-13T22:34:51Z

    if (interpreter) {

I think you need to validate this one too?

Refers to: src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts:72 in 8bc139a. [](commit_id = 8bc139a, deletion_comment = False)

IanMatthewHuff · 2020-02-14T17:06:50Z

src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts

-            return this._selectedInterpreter;
-        }
+        // Before we return _selected interpreter make sure that we have run our initial set interpreter once
+        await this.setInitialInterpreter(token);


I liked the promise at this level better. getSelectedInterpreter is basically a property here, having to swap out the promise results if you set the interpreter later feels like a weird pattern. Instead, just promise wrap the unit of work that we expect to only perform once per extension activation which will always have the same result, and make sure that it happens before we return the property for the first time.

Could you just return the value of the promise?

In reply to: 379544662 [](ancestors = 379544662)

Or is that not true because the promise may finish and then later something else updates the value?

In reply to: 379547528 [](ancestors = 379547528,379544662)

That feels kinda confusing. Maybe a comment explaining how the two are separate. The initial promise must always be run, but the actual selection can be updated at any time.

In reply to: 379548204 [](ancestors = 379548204,379547528,379544662)

rchiodo · 2020-02-14T17:11:17Z

src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts

+
+            if (currentInterpreter) {
+                // Ask and give a chance to install dependencies in current interpreter
+                if (await this.interpreterConfiguration.areDependenciesInstalled(currentInterpreter, token)) {


Should you use validate here instead of checking dependencies? Or is that overkill because you already have th einterpreter?

sonarcloud · 2020-02-14T19:23:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

rchiodo

…e used as active interpreters (#10113)

* Better messaging on notebook fail (#10056) * Install jupyter instead of installing kernel spec (#10080) * Install jupyter instead of installing kernel spec For #10071 * Eliminate variable value when computing data frame info (#10081) * Fix ndarray types to be viewable again (#10093) * Eliminate variable value when computing data frame info * Fix ndarrays to work again Add test to make sure we don't regress this again * Rchiodo/kernel telemetry (#10115) * Add duration to select local/remote kernel * Add notebook language telemetry * Add news entries * #9883 telemetry * News entry * Another spot for kernel spec failure * Add telemetry on product install * Fix install telemetry * Undo launch.json change * Handle other cases * Better way to handle case * Wrong event for jupyter install * Fix unit tests * Clear variables when restarting regardless if visible or not (#10117) * Use different method for checking if kernelspec is available (#10114) * Use different method for checking if kernelspec is available * Fix unit tests * More logging for kernelspec problems (#10132) * More logging for kernelspec problems * Actually capture the exception on the new code * Not actually using output if first exception still there. * Actually only return output on one of the expected calls. * Fix nightly flake * Check our saved jupyter interpreters before allowing any of them to be used as active interpreters (#10113) * Update changelog and package.json * Missing part of changelog * Fix tests and linter problems Co-authored-by: Ian Huff <ian.huff@gmail.com> Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com>

initial idea

5ec1a99

IanMatthewHuff changed the title ~~initial idea~~ DRAFT initial idea Feb 13, 2020

IanMatthewHuff commented Feb 13, 2020

View reviewed changes

IanMatthewHuff requested review from DavidKutu and rchiodo February 13, 2020 21:00

refactor to save check promise

8bc139a

rchiodo reviewed Feb 13, 2020

View reviewed changes

src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts Outdated Show resolved Hide resolved

rchiodo reviewed Feb 13, 2020

View reviewed changes

rchiodo requested changes Feb 13, 2020

View reviewed changes

IanMatthewHuff added 2 commits February 13, 2020 16:28

revamp of initial loading

b25c387

unit tests added

f0eb2be

IanMatthewHuff added the no-changelog No news entry required label Feb 14, 2020

IanMatthewHuff commented Feb 14, 2020

View reviewed changes

IanMatthewHuff marked this pull request as ready for review February 14, 2020 17:08

rchiodo reviewed Feb 14, 2020

View reviewed changes

IanMatthewHuff changed the title ~~DRAFT initial idea~~ Check our saved jupyter interpreters before allowing any of them to be used as active interpreters Feb 14, 2020

slight refactor for setting active jupyter interpreter

331e7f7

rchiodo approved these changes Feb 14, 2020

View reviewed changes

IanMatthewHuff merged commit 8bf0209 into microsoft:master Feb 14, 2020

IanMatthewHuff deleted the dev/ianhu/savedServerFail branch February 14, 2020 21:38

rchiodo pushed a commit that referenced this pull request Feb 15, 2020

Check our saved jupyter interpreters before allowing any of them to b…

0a3f220

…e used as active interpreters (#10113)

lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check our saved jupyter interpreters before allowing any of them to be used as active interpreters #10113

Check our saved jupyter interpreters before allowing any of them to be used as active interpreters #10113

IanMatthewHuff commented Feb 13, 2020 •

edited

Loading

IanMatthewHuff Feb 13, 2020

IanMatthewHuff Feb 13, 2020

rchiodo Feb 13, 2020

rchiodo Feb 13, 2020 •

edited

Loading

IanMatthewHuff Feb 13, 2020

rchiodo Feb 13, 2020

DavidKutu Feb 13, 2020

IanMatthewHuff Feb 13, 2020

rchiodo Feb 13, 2020

IanMatthewHuff commented Feb 13, 2020

codecov-io commented Feb 13, 2020 •

edited

Loading

IanMatthewHuff commented Feb 13, 2020

rchiodo Feb 13, 2020

rchiodo Feb 13, 2020

IanMatthewHuff Feb 13, 2020

IanMatthewHuff Feb 13, 2020

rchiodo left a comment

rchiodo commented Feb 13, 2020

IanMatthewHuff Feb 14, 2020

rchiodo Feb 14, 2020

rchiodo Feb 14, 2020

rchiodo Feb 14, 2020

rchiodo Feb 14, 2020

sonarcloud bot commented Feb 14, 2020

rchiodo left a comment

Check our saved jupyter interpreters before allowing any of them to be used as active interpreters #10113

Check our saved jupyter interpreters before allowing any of them to be used as active interpreters #10113

Conversation

IanMatthewHuff commented Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff commented Feb 13, 2020

codecov-io commented Feb 13, 2020 • edited Loading

Codecov Report

IanMatthewHuff commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo left a comment

Choose a reason for hiding this comment

rchiodo commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 14, 2020

rchiodo left a comment

Choose a reason for hiding this comment

IanMatthewHuff commented Feb 13, 2020 •

edited

Loading

rchiodo Feb 13, 2020 •

edited

Loading

codecov-io commented Feb 13, 2020 •

edited

Loading