Pre warm raw kernel daemons by DonJayamanne · Pull Request #11487 · microsoft/vscode-python

DonJayamanne · 2020-04-28T23:37:39Z

Currently adding tests and some minor tweaks
Will update Wiki to document how interrupt and daemon work
Will fix functional tests.
Basically the way the daemon works is simple:
- We start a python process and block on stdin
- When we want to use a daemon, we write to the stdin, and thats when the actual ipykernel starts
- Added comments in code (why it cannot be a separate thread - basically interrupt on windows doesn't work as it needs to be main thread).

* ipykernelInstaller: Add missing service Oops Typo Added tests Use separate class to manage kernel depedencies Ensure interrupt button is always enabled (#11461) Add collapsible sections to the bug issue template (#11462) Support ipywidgets in the kernel work (#11444) Ensure isolate script is passed as command arg when installing modules (#11447) Send LS version in static enabled/ready/startup events via lazy properties (#11448)

* master: Use separate class to manage kernel dependencies (#11472) Add ability to pass interpreter into kernel launcher (#11482) Use full path in argv to find interpreter matching a kernel (#11478) make sure we use our resource to get interpreter details (#11474) Handle failure to launch kernel while connecting (#11454) make raw kernel launch respect launching resource environment (#11463)

codecov-io · 2020-04-28T23:57:26Z

Codecov Report

Merging #11487 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11487   +/-   ##
=======================================
  Coverage   60.35%   60.35%           
=======================================
  Files         618      618           
  Lines       33751    33751           
  Branches     4762     4762           
=======================================
  Hits        20372    20372           
  Misses      12368    12368           
  Partials     1011     1011

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 8f6116b...8f6116b. Read the comment docs.

* master: Track notebook/interactive start dates (#11484) Merging back from release to master. (#11488)

* master: Adds missing entries in the unittest ioc container for extract var tests (#11493) Add debugger package root api. (#11464) Replace word "browse" with "find" (#11460) Route python interpreter into kernel launcher (#11486)

rchiodo · 2020-04-29T20:50:27Z

+        await daemonPool.preWarmKernelDaemons();
+
+        // Verify we only created 2 daemons.
+        verify(pythonExecutionFactory.createDaemon(anything())).twice();


This feels like you're understanding implementation details. Would it be better to just check that the daemonPool object has 2 daemons running?

For that I need to expose the list, but i don't want to expose that list.
Its either expose the list for testing puproses or verify we called createDaemon twice.

You don't need the list, just a count

Maybe we need a team chat at some point about handling testing, especially post the testing talk that Graham gave? I'll confess to sometimes being a bit unsure about when I'm using verify in particular.

rchiodo · 2020-04-29T20:51:09Z

+        let daemon = await daemonPool.get(workspace1, kernelSpec, interpreter1);
+        assert.equal(daemon, instance(daemon1));
+        // Verify this daemon was pre-warmed.
+        verify(daemon1.preWarm()).atLeast(1);


preWarm [](start = 23, length = 7)

Same here, the daemon should have something to tell what state it is in. Like 'waiting for prewarm' or something. I think unit tests that know about internal details are brittle.

Can expose a state, but thats only being used for testing purpose, which means we're adding API for testing alone (i.e. exposing internal test for testing purposes).

Isn't that better than exposing the internal implementation details?

FYI - prewarm is a public API.

rchiodo · 2020-04-29T20:52:09Z

+        await daemonPool.preWarmKernelDaemons();
+
+        // Verify we only created 2 daemons.
+        verify(pythonExecutionFactory.createDaemon(anything())).twice();


createDaemon [](start = 38, length = 12)

Yeah I guess it seems bad to me to verify all of these objects by inspecting their internal algorithm.

Same as above, we either expose a list of all created daemons for testing purposes or verify 2 were created.
I'm happy to go with the exposing of list for testing purposes if you're fine with that. However thats similar, as were exposing object internals for testing purposes.

It's the difference between exposing private details and public. If we make the list (or maybe just the count) public, then we're committing to support that. Changes to this code later will know that it needs to keep that up to date. But if we change the internal details, we'll have to fix up a whole bunch of other stuff and these tests might not even work anymore.

Did you get a chance to read Grant's talk yet?

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

Did you get a chance to read Grant's talk yet?

Yes, guess need to put it into practice and be more disciplined.

rchiodo

🕐

* master: Support language servers that don't allow incremental doc updates (#11490)

rchiodo · 2020-04-29T21:53:32Z

        }

        // Finally create if needed
+        let resource: Resource = options.identity;


Or this should use the actual resource that started the interactive window. I believe it used to. We might have messed it up when we created the notebook provider.

Yes, but looking at the code I could'nt find this being provided. I'll file an issue to ensure we pass this into NotebookProvider.
Originally it wasn't required, then we started using identity for the wrong reason.

* master: Do not perform pipenv interpreter discovery on activation (#11369) Remove IJMPConnection and uses of it while maintaining same tests (#11503)

DonJayamanne · 2020-04-29T23:36:47Z

        suppressShutdownErrors(this.realKernel);
-        return this.realKernel.shutdown().then(() => this.kernelProcess.dispose());
+        await this.kernelProcess.dispose();
+        this.socket.dispose();


@rchiodo @IanMatthewHuff Changed here as previous implementation would fall over.
We cannot call shutdown of the realKernel cuz it tries to make http call to jupyter.

Yeah but it just fails silently. I thought it also set state on itself. This would mean we're not setting that state.

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

sonarqubecloud · 2020-04-29T23:37:00Z

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information
0.4% Duplication

rchiodo

DonJayamanne added 11 commits April 28, 2020 08:59

Fix

6a5e644

Use separate class to manage kernel depedencies

1270c08

Added tests

7aee1e8

Typo

380e501

Oops

6f297a6

Add missing service

bcea78c

Fix linter

1720b39

More fixes

2a1ce1f

Fix tests

4bb951b

Fixes

9c60491

DonJayamanne added the no-changelog No news entry required label Apr 29, 2020

DonJayamanne added 8 commits April 29, 2020 08:59

Merge branch 'master' into preWarmKernel

1f0d694

* master: Track notebook/interactive start dates (#11484) Merging back from release to master. (#11488)

Fixes

ceeb9cb

Added tests

84fd9d4

Added tests

7acd719

Tests

20bbc58

Fix tests

fd4fce7

Start adding tests

c4526a7

Fixes

95fe387

DonJayamanne changed the title ~~Pre warm kernel~~ Pre warm raw kernel daemons Apr 29, 2020

DonJayamanne requested review from DavidKutu, IanMatthewHuff, greazer and rchiodo April 29, 2020 18:59

DonJayamanne marked this pull request as ready for review April 29, 2020 19:01

DonJayamanne added 2 commits April 29, 2020 13:38

Fixes

cb7a851

Merge branch 'master' into preWarmKernel

29278c6

* master: Adds missing entries in the unittest ioc container for extract var tests (#11493) Add debugger package root api. (#11464) Replace word "browse" with "find" (#11460) Route python interpreter into kernel launcher (#11486)

rchiodo reviewed Apr 29, 2020

View reviewed changes

Comment thread src/client/datascience/kernel-launcher/kernelDaemonPool.ts

rchiodo reviewed Apr 29, 2020

View reviewed changes

Comment thread src/test/datascience/kernel-launcher/kernelLauncherDaemon.unit.test.ts

rchiodo requested changes Apr 29, 2020

View reviewed changes

DonJayamanne added 3 commits April 29, 2020 14:18

Fixes

aff7634

More fixes

7f3d4f6

Merge branch 'master' into preWarmKernel

5710358

* master: Support language servers that don't allow incremental doc updates (#11490)

IanMatthewHuff reviewed Apr 29, 2020

View reviewed changes

Comment thread src/client/datascience/kernel-launcher/kernelDaemonPool.ts Outdated

DonJayamanne added 2 commits April 29, 2020 14:37

Fixes

6798cf0

Address code review

ec5d77a

DonJayamanne requested review from IanMatthewHuff and rchiodo April 29, 2020 21:50

IanMatthewHuff approved these changes Apr 29, 2020

View reviewed changes

rchiodo reviewed Apr 29, 2020

View reviewed changes

Address code review fixes

336dbaf

DonJayamanne requested a review from rchiodo April 29, 2020 22:30

DonJayamanne added 5 commits April 29, 2020 15:39

Merge branch 'master' into preWarmKernel

6be0c72

* master: Do not perform pipenv interpreter discovery on activation (#11369) Remove IJMPConnection and uses of it while maintaining same tests (#11503)

Fixes

6d374c2

Fixes

d12ff7d

Fix linter

fd46aa7

Fix shutting down of kernel

8f6116b

DonJayamanne commented Apr 29, 2020

View reviewed changes

rchiodo approved these changes Apr 29, 2020

View reviewed changes

DonJayamanne merged commit 4cf109b into microsoft:master Apr 30, 2020

DonJayamanne deleted the preWarmKernel branch April 30, 2020 00:24

lock Bot locked as resolved and limited conversation to collaborators May 20, 2020

Conversation

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

Uh oh!

Uh oh!

codecov-io commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Apr 29, 2020

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DonJayamanne commented Apr 28, 2020 •

edited

Loading

codecov-io commented Apr 28, 2020 •

edited

Loading