Skip to content

Add conda support to nightly flake test #10523

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

Merged
merged 34 commits into from
Mar 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
b260c2a
Add conda environments
rchiodo Mar 4, 2020
885f972
Reenable multiple interpreters
rchiodo Mar 4, 2020
d3f04f6
Change path for conda environments
rchiodo Mar 4, 2020
11c6097
Add pip requirements
rchiodo Mar 4, 2020
db60ded
Get multiple interpreters test to work
rchiodo Mar 4, 2020
fca098e
Partially working conda search
rchiodo Mar 5, 2020
ba0fc56
Get conda working locally
rchiodo Mar 5, 2020
d4f3fee
Make windows supported environments
rchiodo Mar 5, 2020
fb16261
Fix hygiene
rchiodo Mar 5, 2020
bfb4cec
Fix memory leak for daemons
rchiodo Mar 6, 2020
43cf4a1
Fix hang on conda
rchiodo Mar 6, 2020
8e11f39
Actually retry
rchiodo Mar 6, 2020
2573ea0
Two missing setting change updates
rchiodo Mar 6, 2020
c418e11
Rework the failure capture
rchiodo Mar 6, 2020
318b82c
Get rid of memory leak when session dies
rchiodo Mar 6, 2020
cd9112a
Make tests leak less memory
rchiodo Mar 6, 2020
fd76344
Hygiene problem
rchiodo Mar 6, 2020
a83acca
More potential cleanup
rchiodo Mar 6, 2020
be983b2
More changes to clean up memory as soon as possible
rchiodo Mar 9, 2020
39f5e4b
Dont auto start jupyter
rchiodo Mar 9, 2020
dcd6c0f
More leaks found and fixes for breaking tests
rchiodo Mar 9, 2020
21eab4b
Merge remote-tracking branch 'origin/master' into rchiodo/test_with_a…
rchiodo Mar 9, 2020
87fbdbb
Fix missing args when test is shutting down
rchiodo Mar 10, 2020
96b4395
Put back some of the functional requirements
rchiodo Mar 10, 2020
587bafc
Update tests to not use static
rchiodo Mar 10, 2020
94b75fd
Fix linter tests
rchiodo Mar 10, 2020
7712c47
More output for linter tests
rchiodo Mar 10, 2020
b9fc139
Linter only fails with other tests
rchiodo Mar 10, 2020
6f85ffb
Install linters in conda too
rchiodo Mar 10, 2020
130fc50
Make base also have dependencies as it's being picked up anyway
rchiodo Mar 10, 2020
48cc263
Add news entry
rchiodo Mar 11, 2020
bcdfdaf
Review feedback
rchiodo Mar 11, 2020
04d6caa
Fix unit tests and one of the functional tests
rchiodo Mar 11, 2020
1f03b7c
Functional tests and workspace tests at the same time fail
rchiodo Mar 11, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,9 @@
"env": {
// Remove `X` prefix to test with real python (for DS functional tests).
"XVSCODE_PYTHON_ROLLING": "1",
// Remove 'X' to turn on all logging in the debug output
"XVSC_PYTHON_FORCE_LOGGING": "1",
// Remove `X` prefix and update path to test with real python interpreter (for DS functional tests).
// Do not use a conda environment (as it needs to be activated and the like).
"XCI_PYTHON_PATH": "<Python Path>"
},
"outFiles": [
Expand Down
4 changes: 4 additions & 0 deletions build/ci/conda_base.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
pandas
jupyter
numpy
matplotlib
7 changes: 7 additions & 0 deletions build/ci/conda_env_1.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
name: conda_env_1
dependencies:
- python=3.7
- pandas
- jupyter
- numpy
- matplotlib
7 changes: 7 additions & 0 deletions build/ci/conda_env_2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
name: conda_env_2
dependencies:
- python=3.8

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

One of the tests uses two environments now.


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

- pandas
- jupyter
- numpy
- matplotlib
71 changes: 61 additions & 10 deletions build/ci/templates/test_phases.yml
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,7 @@ steps:
python -m pip --disable-pip-version-check install -t ./pythonFiles/lib/python/debugpy/no_wheels --no-cache-dir --implementation py --no-deps --upgrade --pre debugpy
displayName: 'pip install system test requirements'
condition: and(succeeded(), eq(variables['NeedsPythonTestReqs'], 'true'))

# Install the additional sqlite requirements
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
- bash: |
sudo apt-get install libsqlite3-dev
python -m pip install pysqlite
displayName: 'Setup python to run with sqlite on 2.7'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), eq(variables['Agent.Os'], 'Linux'), eq(variables['PythonVersion'], '2.7'))


# Install the requirements for functional tests.
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
Expand All @@ -118,9 +109,69 @@ steps:
- bash: |
python -m pip install numpy
python -m pip install --upgrade -r ./build/functional-test-requirements.txt
python -c "import sys;print(sys.executable)"
displayName: 'pip install functional requirements'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'))

# Add CONDA to the path so anaconda works
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
- bash: |
echo "##vso[task.prependpath]$CONDA/bin"
displayName: 'Add conda to the path'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), ne(variables['Agent.Os'], 'Windows_NT'))

# Add CONDA to the path so anaconda works (windows)
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
- powershell: |
Write-Host "##vso[task.prependpath]$env:CONDA\Scripts"
displayName: 'Add conda to the path'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), eq(variables['Agent.Os'], 'Windows_NT'))

# On MAC let CONDA update install paths
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
- bash: |
sudo chown -R $USER $CONDA
displayName: 'Give CONDA permission to its own files'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), eq(variables['Agent.Os'], 'Darwin'))

# Create the two anaconda environments
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
#
- script: |
conda env create --quiet --force --file build/ci/conda_env_1.yml
conda env create --quiet --force --file build/ci/conda_env_2.yml
displayName: 'Create CONDA Environments'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'))

# Run the pip installs in the 3 environments (darwin linux)
- bash: |
source activate base
conda install --quiet -y --file build/ci/conda_base.yml
python -m pip install --upgrade -r build/conda-functional-requirements.txt
source activate conda_env_1
python -m pip install --upgrade -r build/conda-functional-requirements.txt
source activate conda_env_2
python -m pip install --upgrade -r build/conda-functional-requirements.txt
conda deactivate
displayName: 'Install Pip requirements for CONDA envs'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), ne(variables['Agent.Os'], 'Windows_NT'))

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

Choose a reason for hiding this comment

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

Why install in base environment?

Copy link
Author

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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)

python -m pip install --upgrade -r build/conda-functional-requirements.txt
call activate conda_env_1
python -m pip install --upgrade -r build/conda-functional-requirements.txt
call activate conda_env_2
python -m pip install --upgrade -r build/conda-functional-requirements.txt
displayName: 'Install Pip requirements for CONDA envs'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'), eq(variables['Agent.Os'], 'Windows_NT'))

# Downgrade pywin32 on Windows due to bug https://github.com/jupyter/notebook/issues/4909
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
Expand Down
48 changes: 0 additions & 48 deletions build/ci/vscode-python-nightly-flake-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,6 @@ stages:
steps:
- template: templates/test_phases.yml

- job: 'Py36'

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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


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

dependsOn: []
timeoutInMinutes: 120
strategy:
matrix:
'Functional':
PythonVersion: '3.6'
TestsToRun: 'testfunctional'
NeedsPythonTestReqs: true
NeedsPythonFunctionalReqs: true
VSCODE_PYTHON_ROLLING: true
pool:
vmImage: 'ubuntu-16.04'
steps:
- template: templates/test_phases.yml

- stage: Mac
dependsOn:
- Build
Expand All @@ -85,22 +69,6 @@ stages:
steps:
- template: templates/test_phases.yml

- job: 'Py36'
dependsOn: []
timeoutInMinutes: 120
strategy:
matrix:
'Functional':
PythonVersion: '3.6'
TestsToRun: 'testfunctional'
NeedsPythonTestReqs: true
NeedsPythonFunctionalReqs: true
VSCODE_PYTHON_ROLLING: true
pool:
vmImage: '$(vmImageMacOS)'
steps:
- template: templates/test_phases.yml

- stage: Windows
dependsOn:
- Build
Expand All @@ -119,19 +87,3 @@ stages:
vmImage: 'vs2017-win2016'
steps:
- template: templates/test_phases.yml

- job: 'Py36'
dependsOn: []
timeoutInMinutes: 120
strategy:
matrix:
'Functional':
PythonVersion: '3.6'
TestsToRun: 'testfunctional'
NeedsPythonTestReqs: true
NeedsPythonFunctionalReqs: true
VSCODE_PYTHON_ROLLING: true
pool:
vmImage: 'vs2017-win2016'
steps:
- template: templates/test_phases.yml
19 changes: 19 additions & 0 deletions build/conda-functional-requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# List of requirements for conda environments that cannot be installed using conda
livelossplot
versioneer
flake8
autopep8
bandit
black ; python_version>='3.6'
yapf
pylint
pycodestyle
prospector
pydocstyle
nose
pytest==4.6.9 # Last version of pytest with Python 2.7 support
rope
flask
django
isort
pathlib2>=2.2.0 ; python_version<'3.6' # Python 2.7 compatibility (pytest)
9 changes: 2 additions & 7 deletions build/functional-test-requirements.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,2 @@
# List of requirements for functional tests
versioneer
jupyter
numpy
matplotlib
pandas
livelossplot
# List of requirements for functional tests
versioneer
1 change: 1 addition & 0 deletions news/3 Code Health/10134.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add conda environments to nightly test runs
1 change: 1 addition & 0 deletions src/client/common/asyncDisposableRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export class AsyncDisposableRegistry implements IAsyncDisposableRegistry {
public async dispose(): Promise<void> {
const promises = this._list.map(l => l.dispose());
await Promise.all(promises);
this._list = [];
}

public push(disposable?: IDisposable | IAsyncDisposable) {
Expand Down
3 changes: 2 additions & 1 deletion src/client/common/process/proc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ export class ProcessService extends EventEmitter implements IProcessService {
const proc = spawn(file, args, spawnOptions);
let procExited = false;
const disposable: IDisposable = {
dispose: () => {
// tslint:disable-next-line: no-function-expression
dispose: function() {
if (proc && !proc.killed && !procExited) {
ProcessService.kill(proc.pid);
}
Expand Down
1 change: 1 addition & 0 deletions src/client/common/process/pythonDaemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export class PythonDaemonExecutionService implements IPythonDaemonExecutionServi
try {
// The daemon should die as a result of this.
this.connection.sendNotification(new NotificationType('exit'));
this.proc.kill();
} catch {
noop();
}
Expand Down
6 changes: 4 additions & 2 deletions src/client/common/process/pythonDaemonPool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export class PythonDaemonExecutionServicePool implements IPythonDaemonExecutionS
private readonly observableDaemons: IPythonDaemonExecutionService[] = [];
private readonly envVariables: NodeJS.ProcessEnv;
private readonly pythonPath: string;
private _disposed = false;
constructor(
private readonly logger: IProcessLogger,
private readonly disposables: IDisposableRegistry,
Expand Down Expand Up @@ -67,6 +68,7 @@ export class PythonDaemonExecutionServicePool implements IPythonDaemonExecutionS

// Always ignore warnings as the user should never see the output of the daemon running
this.envVariables[PYTHON_WARNINGS] = 'ignore';
this.disposables.push(this);
}
public async initialize() {
const promises = Promise.all(
Expand All @@ -85,7 +87,7 @@ export class PythonDaemonExecutionServicePool implements IPythonDaemonExecutionS
await Promise.all([promises, promises2]);
}
public dispose() {
noop();
this._disposed = true;
}
public async getInterpreterInformation(): Promise<InterpreterInfomation | undefined> {
const msg = { args: ['GetPythonVersion'] };
Expand Down Expand Up @@ -231,7 +233,7 @@ export class PythonDaemonExecutionServicePool implements IPythonDaemonExecutionS
completed = true;
if (!daemonProc || (!daemonProc.killed && ProcessService.isAlive(daemonProc.pid))) {
this.pushDaemonIntoPool('ObservableDaemon', execService);
} else {
} else if (!this._disposed) {
// Possible daemon is dead (explicitly killed or died due to some error).
this.addDaemonService('ObservableDaemon').ignoreErrors();
}
Expand Down
9 changes: 4 additions & 5 deletions src/client/common/terminal/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class TerminalHelper implements ITerminalHelper {
@inject(IPlatformService) private readonly platform: IPlatformService,
@inject(ITerminalManager) private readonly terminalManager: ITerminalManager,
@inject(ICondaService) private readonly condaService: ICondaService,
@inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
@inject(IInterpreterService) readonly interpreterService: IInterpreterService,
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
@inject(ITerminalActivationCommandProvider)
@named(TerminalActivationProviders.conda)
Expand Down Expand Up @@ -71,9 +71,9 @@ export class TerminalHelper implements ITerminalHelper {
const providers = [this.pipenv, this.pyenv, this.bashCShellFish, this.commandPromptAndPowerShell];
const promise = this.getActivationCommands(resource || undefined, interpreter, terminalShellType, providers);
this.sendTelemetry(
resource,
terminalShellType,
EventName.PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL,
interpreter,
promise
).ignoreErrors();
return promise;
Expand All @@ -89,22 +89,21 @@ export class TerminalHelper implements ITerminalHelper {
const providers = [this.bashCShellFish, this.commandPromptAndPowerShell];
const promise = this.getActivationCommands(resource, interpreter, shell, providers);
this.sendTelemetry(
resource,
shell,
EventName.PYTHON_INTERPRETER_ACTIVATION_FOR_RUNNING_CODE,
interpreter,
promise
).ignoreErrors();
return promise;
}
@traceDecorators.error('Failed to capture telemetry')
protected async sendTelemetry(
resource: Resource,
terminalShellType: TerminalShellType,
eventName: EventName,
interpreter: PythonInterpreter | undefined,
promise: Promise<string[] | undefined>
): Promise<void> {
let hasCommands = false;
const interpreter = await this.interpreterService.getActiveInterpreter(resource);
let failed = false;
try {
const cmds = await promise;
Expand Down
2 changes: 1 addition & 1 deletion src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export interface IOutputChannel extends OutputChannel {}
export const IDocumentSymbolProvider = Symbol('IDocumentSymbolProvider');
export interface IDocumentSymbolProvider extends DocumentSymbolProvider {}
export const IsWindows = Symbol('IS_WINDOWS');
export const IDisposableRegistry = Symbol('IDiposableRegistry');
export const IDisposableRegistry = Symbol('IDisposableRegistry');
export type IDisposableRegistry = { push(disposable: Disposable): void };
export const IMemento = Symbol('IGlobalMemento');
export const GLOBAL_MEMENTO = Symbol('IGlobalMemento');
Expand Down
14 changes: 13 additions & 1 deletion src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,13 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
super.dispose();
this.listeners.forEach(l => l.dispose());
this.updateContexts(undefined);

// When closing an editor, dispose of the notebook associated with it.
// This won't work when we have multiple views of the notebook though. Notebook ownership
// should probably move to whatever owns the backing model.
return this.notebook?.dispose().then(() => {
this._notebook = undefined;
});
}

public startProgress() {
Expand Down Expand Up @@ -1412,7 +1419,12 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
// Request our new list of variables
const response: IJupyterVariablesResponse = this._notebook
? await this.jupyterVariables.getVariables(this._notebook, args)
: { totalCount: 0, pageResponse: [], pageStartIndex: args.startIndex, executionCount: args.executionCount };
: {
totalCount: 0,
pageResponse: [],
pageStartIndex: args?.startIndex,
executionCount: args?.executionCount
};

this.postMessage(InteractiveWindowMessages.GetVariablesResponse, response).ignoreErrors();
sendTelemetryEvent(Telemetry.VariableExplorerVariableCount, undefined, { variableCount: response.totalCount });
Expand Down
12 changes: 0 additions & 12 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -512,18 +512,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
protected async close(): Promise<void> {
// Fire our event
this.closedEvent.fire(this);

// Restart our kernel so that execution counts are reset
let oldAsk: boolean | undefined = false;
const settings = this.configuration.getSettings(await this.getOwningResource());
if (settings && settings.datascience) {
oldAsk = settings.datascience.askForKernelRestart;
settings.datascience.askForKernelRestart = false;
}
await this.restartKernel(true);

Choose a reason for hiding this comment

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

Isn't this is a breaking change?

Copy link
Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (oldAsk && settings && settings.datascience) {
settings.datascience.askForKernelRestart = true;
}
}

protected saveAll() {
Expand Down
Loading