Add conda support to nightly flake test by rchiodo · Pull Request #10523 · microsoft/vscode-python

rchiodo · 2020-03-11T16:01:27Z

For #10134 - Conda support for tests

Last pipeline run is here. Mostly working so going to submit for now. I'll work on the last couple of failures later
https://dev.azure.com/ms/vscode-python/_build/results?buildId=67498&view=results

Essentially this change enables all of the interpreter services inside the test container. As a result of this there seems to be a memory leak caused by promises never finishing. So part of this work was to cleanup a bunch of stuff during dispose.

…naconda

codecov-io · 2020-03-11T16:22:44Z

Codecov Report

Merging #10523 into master will decrease coverage by 0.11%.
The diff coverage is 37%.

@@            Coverage Diff             @@
##           master   #10523      +/-   ##
==========================================
- Coverage   60.86%   60.74%   -0.12%     
==========================================
  Files         579      578       -1     
  Lines       31294    31355      +61     
  Branches     4451     4463      +12     
==========================================
+ Hits        19046    19047       +1     
- Misses      11285    11339      +54     
- Partials      963      969       +6

Impacted Files	Coverage Δ
...ient/datascience/interactive-ipynb/nativeEditor.ts	`11.76% <ø> (+0.35%)`	⬆️
src/client/ioc/types.ts	`100% <ø> (ø)`	⬆️
...reter/locators/services/cacheableLocatorService.ts	`78.7% <0%> (-1.49%)`	⬇️
src/client/datascience/jupyter/jupyterExecution.ts	`49.36% <0%> (-0.32%)`	⬇️
...atascience/jupyter/liveshare/guestJupyterServer.ts	`11.76% <0%> (ø)`	⬆️
src/client/common/process/proc.ts	`14.49% <0%> (-0.73%)`	⬇️
src/client/datascience/jupyter/notebookStarter.ts	`65.25% <0%> (ø)`	⬆️
src/client/common/process/pythonDaemon.ts	`19.68% <0%> (-0.11%)`	⬇️
src/client/datascience/jupyter/jupyterServer.ts	`7.36% <0%> (-0.16%)`	⬇️
.../datascience/interactive-common/interactiveBase.ts	`5.52% <0%> (-0.02%)`	⬇️
... and 23 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 cd90761...1f03b7c. Read the comment docs.

DonJayamanne · 2020-03-11T16:47:56Z

build/ci/conda_env_2.yml

@@ -0,0 +1,7 @@
+name: conda_env_2
+dependencies:
+  - python=3.8


Trying to understand the purpose of creating two environments.
We have P37 and P38. Which will be used in tests?
I think P38 will always be used. If that's the case, then why install pandas, jupyter, matplotlib in P37?

One of the tests uses two environments now.

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

DonJayamanne · 2020-03-11T16:51:29Z

build/ci/vscode-python-nightly-flake-ci.yaml

    steps:
      - template: templates/test_phases.yml

-  - job: 'Py36'


Why are we removing 3.6?
At a minimum I would have expected to test against 37 as thats the most popular version of Python used, sure soon it will be 38.

This didn't actually test against 36. The conda environment determines what version of python is used now.

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

DonJayamanne · 2020-03-11T16:52:39Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

-            oldAsk = settings.datascience.askForKernelRestart;
-            settings.datascience.askForKernelRestart = false;
-        }
-        await this.restartKernel(true);


Isn't this is a breaking change?

Breaking how? We talked it over and decided it was a mistake to restart the kernel. Instead will just close the kernel.

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

DonJayamanne · 2020-03-11T17:02:37Z

src/client/datascience/jupyter/jupyterSessionManager.ts

+
+            // The session manager can actually be stuck in the context of a timer. Clear out the specs inside of
+            // it so the memory for the session is minimized. Otherwise functional tests can run out of memory
+            if (sessionManager._specs) {


Looking at the code calling this.sessionManager.shutdownAll() and this.sessionManager.dispose should be sufficient (does the same thing).
I'd prefer that, as that's a public api (instead of this approach).

I.e.

try { await this.sessionManager.shutdownAll(); } catch { // noop. } finally { this.sessionManager.dispose(); }

No that doesn't do the same thing, hence why I added this code. There's bugs in the jupyterlab services implementation. At least with respect to shutdown

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

They never stop their polling.

In reply to: 391135964 [](ancestors = 391135964,391123563)

shutdownAll would probably be good though. I'll add that.

In reply to: 391136172 [](ancestors = 391136172,391135964,391123563)

DonJayamanne · 2020-03-11T17:04:49Z

src/client/interpreter/activation/service.ts

+];
+
+export class EnvironmentActivationServiceCache {
+    private static useStatic = false;


Adding some comments explaining the purpose of this class would be useful.
Specially for core extension devs.

Okay

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

DonJayamanne · 2020-03-11T17:07:18Z

src/client/interpreter/activation/service.ts

+                } catch (exc) {
+                    // Special case. Conda for some versions will state a file is in use. If
+                    // that's the case, wait and try again. This happens especially on AzDo
+                    const excString = exc.toString();


Might want to add comments about the issue this is trying to solve (probably link to issue number). Right now its not obvious that we're trying to work around some conda issue.

I.e. its not obvious as to why we even need a retry.

I'll link to the issue and explain here that conda sucks.

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

DonJayamanne · 2020-03-11T17:10:27Z

src/client/interpreter/activation/service.ts

-            if (result.stderr && result.stderr.length > 0) {
-                throw new Error(`StdErr from ShellExec, ${result.stderr}`);
+            let result: ExecutionResult<string> | undefined;
+            while (!result) {


Feels dangerous to keep trying indefinitely. I'd gate this to 10-20 or other retries. I.e. max out at 5-10-20 seconds.

Okay sure. I suppose it's possible it could get stuck indefinitely.

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

DonJayamanne · 2020-03-11T17:13:20Z

src/test/datascience/nativeEditor.functional.test.tsx

                        await addCell(wrapper, ioc, 'a', false);
                    }

+                    async function updateFileConfig(key: string, value: any) {


DonJayamanne · 2020-03-11T17:14:10Z

build/ci/templates/test_phases.yml

+  # Run the pip installs in the 3 environments (windows)
+  - script: |
+      call activate base
+      conda install --quiet -y --file build/ci/conda_base.yml      


Why install in base environment?

Because the functional-requirements.txt seems to install jupyter into the base conda after conda is added to the path. So we need the pandas/matplotlib stuff in the base environment then too. It ends up being the first environment.

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

I think I'm going to leave this as is for now (it's just another environment). the original intent was to have base have nothing in it. For some reason we're detecting jupyter in it though. Maybe the anaconda install does this without our help.

In reply to: 391138171 [](ancestors = 391138171,391131125)

DonJayamanne · 2020-03-11T17:14:59Z

src/client/datascience/jupyter/liveshare/guestJupyterServer.ts

        );
        this.notebooks.set(identity.toString(), result);
-        const oldDispose = result.dispose;
+        const oldDispose = result.dispose.bind(result);


Would have been one reason why things weren't shutting down..

Possibly. This would only be on the live share tests though

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

rchiodo · 2020-03-11T17:16:47Z

src/test/datascience/dataScienceIocContainer.ts

-            this.serviceManager.addSingleton<IInterpreterService>(IInterpreterService, InterpreterService);
+
+            // Make sure full interpreter services are available.
+            registerInterpreterTypes(this.serviceManager);


registerInterpreterTypes [](start = 12, length = 24)

This line here makes all of the conda environments be found.

IanMatthewHuff · 2020-03-11T18:08:09Z

src/client/datascience/jupyter/jupyterSessionManager.ts

 import { Agent as HttpsAgent } from 'https';
 import { CancellationToken } from 'vscode-jsonrpc';

+import { noop } from 'jquery';


This looks like a mistaken autoimport. I mean a noop is noop, but we have our own one in utils that we use everywhere else. Don't think we want jquery here.

Good catch. I'll fix it.

IanMatthewHuff · 2020-03-11T18:09:38Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

-            oldAsk = settings.datascience.askForKernelRestart;
-            settings.datascience.askForKernelRestart = false;
-        }
-        await this.restartKernel(true);


Just a day ago I changed restartKernel to pass a bool to not log telemetry just for this case (as it was not user triggered restart). We could remove that now as it's not needed.

IanMatthewHuff · 2020-03-11T18:19:13Z

src/test/unittests.ts

-// tslint:disable:no-any no-require-imports no-var-requires
+// Not sure why but on windows, if you execute a process from the System32 directory, it will just crash Node.
+// Not throw an exception, just make node exit.
+// However if a system32 process is run first, everything works.


Fine I guess, but that's super weird.

sonarqubecloud · 2020-03-11T21:12:40Z

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information
0.0% Duplication

* master: Fix flakey file system tests (#10541) Tweaks for gather (#10532) Fix #10437: Update launch.json handling to support "listen" and "connect" (#10517) Add conda support to nightly flake test (#10523) Rename datascience to datascience_modules (#10525) Clean up the extension activation code. (#10455) Allow escape and ctrl+U to clear the interactive window (#10513) Fix builtins so they don't interfere with our execution (#10511) Jupyter autocompletion will only show up on empty lines, (#10420) notify on missing kernel and interpreter with kernel (#10508)

rchiodo added 30 commits March 3, 2020 16:28

Add conda environments

b260c2a

Reenable multiple interpreters

885f972

Change path for conda environments

d3f04f6

Add pip requirements

11c6097

Get multiple interpreters test to work

db60ded

Partially working conda search

fca098e

Get conda working locally

ba0fc56

Make windows supported environments

d4f3fee

Fix hygiene

fb16261

Fix memory leak for daemons

bfb4cec

Fix hang on conda

43cf4a1

Actually retry

8e11f39

Two missing setting change updates

2573ea0

Rework the failure capture

c418e11

Get rid of memory leak when session dies

318b82c

Make tests leak less memory

cd9112a

Hygiene problem

fd76344

More potential cleanup

a83acca

More changes to clean up memory as soon as possible

be983b2

Dont auto start jupyter

39f5e4b

More leaks found and fixes for breaking tests

dcd6c0f

Merge remote-tracking branch 'origin/master' into rchiodo/test_with_a…

21eab4b

…naconda

Fix missing args when test is shutting down

87fbdbb

Put back some of the functional requirements

96b4395

Update tests to not use static

587bafc

Fix linter tests

94b75fd

More output for linter tests

7712c47

Linter only fails with other tests

b9fc139

Install linters in conda too

6f85ffb

Make base also have dependencies as it's being picked up anyway

130fc50

rchiodo requested review from DavidKutu, DonJayamanne and IanMatthewHuff March 11, 2020 16:02

rchiodo self-assigned this Mar 11, 2020

Add news entry

48cc263

DonJayamanne requested changes Mar 11, 2020

View reviewed changes

rchiodo commented Mar 11, 2020

View reviewed changes

Review feedback

bcdfdaf

DonJayamanne approved these changes Mar 11, 2020

View reviewed changes

IanMatthewHuff reviewed Mar 11, 2020

View reviewed changes

IanMatthewHuff approved these changes Mar 11, 2020

View reviewed changes

rchiodo added 2 commits March 11, 2020 13:32

Fix unit tests and one of the functional tests

04d6caa

Functional tests and workspace tests at the same time fail

1f03b7c

rchiodo merged commit 1b04ed7 into master Mar 11, 2020

rchiodo deleted the rchiodo/test_with_anaconda branch March 11, 2020 23:39

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

Conversation

rchiodo commented Mar 11, 2020

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

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 Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 11, 2020 •

edited

Loading