Fix interpreter tests #7410

DonJayamanne · 2021-09-03T14:29:51Z

Looks like we have two interpreter paths that end up pointing to the exact same interpreter:

/opt/hostedtoolcache/Python/3.8.11/x64/python
/opt/hostedtoolcache/Python/3.8.11/x64/bin/python

This PR ensures we have a common way to compare interpreter paths and normalize them so that they are treated the same.

codecov-commenter · 2021-09-03T14:50:50Z

Codecov Report

Merging #7410 (34295a2) into main (cce419f) will decrease coverage by 0%.
The diff coverage is 81%.

❗ Current head 34295a2 differs from pull request most recent head 3429ffc. Consider uploading reports for the commit 3429ffc to get more accurate results

@@          Coverage Diff          @@
##            main   #7410   +/-   ##
=====================================
- Coverage     62%     62%   -1%     
=====================================
  Files        361     361           
  Lines      22563   22623   +60     
  Branches    3416    3435   +19     
=====================================
+ Hits       14141   14147    +6     
- Misses      7223    7264   +41     
- Partials    1199    1212   +13

Impacted Files	Coverage Δ
...ient/datascience/export/exportInterpreterFinder.ts	`70% <ø> (ø)`
...e/jupyter/interpreter/jupyterInterpreterService.ts	`89% <ø> (ø)`
src/client/datascience/jupyter/kernels/helpers.ts	`69% <75%> (+<1%)`	⬆️
src/client/api/pythonApi.ts	`60% <83%> (-3%)`	⬇️
.../localPythonAndRelatedNonPythonKernelSpecFinder.ts	`79% <100%> (+<1%)`	⬆️
src/client/pythonEnvironments/info/interpreter.ts	`85% <100%> (+7%)`	⬆️
...ence/notebook/noPythonKernelsNotebookController.ts	`14% <0%> (-26%)`	⬇️
src/client/logging/logger.ts	`54% <0%> (-16%)`	⬇️
...ient/datascience/kernel-launcher/kernelLauncher.ts	`84% <0%> (-8%)`	⬇️
src/client/logging/transports.ts	`76% <0%> (-5%)`	⬇️
... and 9 more

DonJayamanne · 2021-09-03T22:13:00Z

.github/actions/create-venv-for-tests/action.yml

@@ -21,13 +21,17 @@ runs:

        python -m venv .venvkernel
        source .venvkernel/bin/activate
+        python --version
+        python -c "import sys;print(sys.executable)"


Useful, this is how i figured some of the paths were different.

No harm in leaving this.

DonJayamanne · 2021-09-03T22:13:43Z

src/client/api/pythonApi.ts

+                    ) {
+                        return;
+                    }
+                    interpreterCacheForCI.set(resource?.toString() || '', itemToStore);


Looks like Python extension stuff doesn't get logged, this was crucial to figuring out what was going on.
I've enabled this just for CI.
We can remove this, i'm not fussed. I thought it would be useful

I've used the logs from Python extension a lot in the past, hence feel this is useful, but NOT necessary.

Can remove if anyone things we don't need this,

Don't see an issue with leaving it in myself.

DonJayamanne · 2021-09-03T22:15:16Z

src/client/datascience/jupyter/kernels/helpers.ts

@@ -451,6 +461,10 @@ export function findPreferredKernel(
            let subScore = 0;

            if (spec) {
+                traceInfoIf(isCI, `Checking kernel Spec ${JSON.stringify(spec)}`);
+                traceInfoIf(isCI, `isPythonKernelSpec(spec) = ${isPythonKernelSpec(spec)}`);
+                traceInfoIf(isCI, `isKernelRegisteredByUs(spec) = ${isKernelRegisteredByUs(spec)}`);


More logging just for CI, found that if the version numbers were 3.8.10 and 3.8.11, we'd end up returuning 3.8.10 as the correct one when 3.8.11 is whats exected.
Hence this logging was very useful.

Again, this only gets logged in CI

DonJayamanne · 2021-09-03T22:15:36Z

src/client/datascience/jupyter/kernels/helpers.ts

-                            score += 4;
-                        }
-                    }
-                }


Moved this code down to where we compare major versions (keep both together)

DonJayamanne · 2021-09-03T22:16:22Z

src/client/datascience/jupyter/kernels/helpers.ts

+                        metadata.interpreter?.version?.raw &&
+                        metadata.interpreter?.version?.raw === preferredInterpreter?.version?.raw
+                    ) {
+                        traceInfoIf(isCI, 'Increased score by +3 for matching raw version in preferred interpreter');


Cam across a scenario where 3.8.10 was picked over 3.8.11 (beacuse both have 3 and 3.8.10 was on the top of the list hence that got picked when both had the same score).

DonJayamanne · 2021-09-03T22:16:50Z

src/client/pythonEnvironments/info/interpreter.ts

+ *  They are both the same.
+ * This function will take that into account.
+ */
+export function areInterpreterPathsSame(path1: string = '', path2:string = '', ostype = getOSType(), fs?: IFileSystem){


Added new helpers to compare paths, ensured the hash also takes this into account.

DonJayamanne · 2021-09-07T19:42:59Z

src/test/common.ts

+export async function disableExperimentsInPythonExtension(): Promise<void> {
+    const vscode = require('vscode') as typeof import('vscode');
+    const pythonConfig = vscode.workspace.getConfiguration('python', (null as any) as Uri);
+    await pythonConfig.update('experiments.enabled', false, ConfigurationTarget.Global);


This should fix the regressions in Python extension.

rchiodo · 2021-09-07T19:45:35Z

src/client/pythonEnvironments/info/interpreter.ts

+}
+/**
+ * Sometimes on CI, we have paths such as (this could happen on user machines as well)
+ *  - /opt/hostedtoolcache/Python/3.8.11/x64/python


I'm not sure why these are the same? They're the same on your machine, but why everywhere this pattern is found? I would assume on the CI machine these paths are symlinked to each other?

same? They're the same on your machine, but why everywhere this pattern is found? I would assume on the CI machine these paths are symlinked to each other?

Yes, if its /bin/xyz and you have xyz then they are the same. I.e. python doesn't hvae multiple different interpreter executables in the same directory. They have python3.5.exe and python.exe` in the same directory and both are the same.

& /bin/ is only used on linux, on windows its always scripts/....
Also we're looking at python file names, hence windows will always be excluded as well.

found? I would assume on the CI machine these paths are symlinked to each other?

I think so, i didn't try to figure that out. This is very new, we didn't see this in the past.
Looking back at older runs i couldn't find this. Either ther'es a change in some python package or CI or the python extension is not finding these symlinked items.

I guess I'm just nervous this will break something that used to work.

Somebody has say

~/mystuff/python ~/mystuff/bin/python

And those aren't the same. I think you're saying this isn't normal. You wouldn't have bin/python and python at the same level.

ou wouldn't have bin/python and python at the same level.

Yes.
Because if mystuff/python is the python environment, then you won't have a different version interpreter under mystuff/bin/python (thats very very unlikely, from what i know, python doesn't setup interpreters that way).

DonJayamanne requested a review from a team as a code owner September 3, 2021 14:29

DonJayamanne force-pushed the issueFixInterpreterTest branch from 805a57d to 4f9c76b Compare September 3, 2021 15:12

Fix interpreter tests

3d0e80b

DonJayamanne force-pushed the issueFixInterpreterTest branch from 4f9c76b to 3d0e80b Compare September 3, 2021 15:43

DonJayamanne added 19 commits September 3, 2021 08:45

Fixes

7f934e9

Better logging

00c498a

Better logging

5903973

Fixes

22978c0

Fixes

18e6df1

Better fixes

96f13b4

Added some logging for CI

65b185c

More fixes

9574014

Fix tests

69e79dc

Oops

611a58f

oops

4711e19

Fix tests

85fd924

Fix tests

95d3394

Oops

b9fd41b

Fixes

2a1109e

Fixes

62ff3db

Oops

1ed7b40

Fix tests

310521f

Skip test in remote & non-raw

cfc7144

DonJayamanne commented Sep 3, 2021

View reviewed changes

DonJayamanne added 12 commits September 3, 2021 18:03

Added logging

4947189

More fixes

66ca21f

More logging

1030254

Ignore files

9f87900

Add more logging

8703957

Logging

f73ab80

oops

6b8eb52

Some more logging

ed2354d

Add logging

49462a2

Fixes

3a8e2a3

Some more logging

25bbee2

More logging

2040893

DonJayamanne mentioned this pull request Sep 7, 2021

Extension API getActiveInterperter and getInterpreters return undefined and [] microsoft/vscode-python#17303

Closed

Activate python

4252041

DavidKutu approved these changes Sep 7, 2021

View reviewed changes

DonJayamanne added 5 commits September 7, 2021 11:35

More logging

47ef368

Remove logging

dbba5f3

More fixes

04ff288

Oops

002c4f0

Disable python extension experiments

5e16ec2

DonJayamanne commented Sep 7, 2021

View reviewed changes

rchiodo reviewed Sep 7, 2021

View reviewed changes

DonJayamanne added 4 commits September 7, 2021 12:58

Fix tests

34295a2

Oops

982e074

Fix tests

6d940d5

Fixes

3429ffc

rchiodo approved these changes Sep 7, 2021

View reviewed changes

Fix functional tests

80eca1a

DonJayamanne merged commit f08a71f into main Sep 7, 2021

DonJayamanne deleted the issueFixInterpreterTest branch September 7, 2021 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interpreter tests #7410

Fix interpreter tests #7410

DonJayamanne commented Sep 3, 2021 •

edited

Loading

codecov-commenter commented Sep 3, 2021 •

edited

Loading

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

IanMatthewHuff Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 3, 2021

DonJayamanne Sep 7, 2021

rchiodo Sep 7, 2021

DonJayamanne Sep 7, 2021

DonJayamanne Sep 7, 2021

rchiodo Sep 7, 2021

DonJayamanne Sep 7, 2021

Fix interpreter tests #7410

Fix interpreter tests #7410

Conversation

DonJayamanne commented Sep 3, 2021 • edited Loading

codecov-commenter commented Sep 3, 2021 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Sep 3, 2021 •

edited

Loading

codecov-commenter commented Sep 3, 2021 •

edited

Loading