Skip to content

Some fixes for nightly #11802

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.

Sign up for GitHub

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
Rich Chiodo (rchiodo) merged 10 commits into from
May 14, 2020
Merged

Some fixes for nightly #11802

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3414a6b
Fix debugger tests to exit appropriately
rchiodo May 12, 2020
32f1483
Push pip to test environments
rchiodo May 12, 2020
46f4432
Try another fix for installing
rchiodo May 13, 2020
d523a42
Try again to fix conda install
rchiodo May 13, 2020
3bb2a06
Try to fix connection file problem with raw kernel
rchiodo May 13, 2020
f55faf0
Don't throw on cleanup
rchiodo May 13, 2020
5f20294
Fix multiple interpreters for the interactive window
rchiodo May 13, 2020
c97a5c0
Add comment
rchiodo May 13, 2020
4e4cc34
Fix kernel launcher test to not fail
rchiodo May 14, 2020
3052fcf
Fix unit test failures
rchiodo May 14, 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
Next Next commit
Fix debugger tests to exit appropriately 
Fix functional tests to always use a valid interpreter (instead of just one with jupyter)
  • Loading branch information
@rchiodo
rchiodo committed May 12, 2020
commit 3414a6bd47007637322d23f9b31ce20210e917f5
2 changes: 1 addition & 1 deletion src/client/datascience/jupyter/debuggerVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export class DebuggerVariables implements IConditionalJupyterVariables, DebugAda
// tslint:disable-next-line: no-any
(variable as any).frameId
);
if (results) {
if (results && results.result) {

@rchiodo Rich Chiodo (rchiodo) May 13, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was unnecessarily crashing sometimes (when debugpy doesn't return anything because the value may not evaluate). Checking this here lets the value just exist as is.

// Results should be the updated variable.
return {
...variable,
Expand Down
3 changes: 3 additions & 0 deletions src/client/datascience/jupyterDebugService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,11 @@ export class JupyterDebugService implements IJupyterDebugService, IDisposable {

private sendMessage(command: string, args?: any): Promise<any> {
const response = createDeferred<any>();
const disposable = this.sessionTerminatedEvent.event(() => response.resolve());
this.protocolParser.once(`response_${command}`, (resp: any) => {
this.sendToTrackers(resp);
traceInfo(`Received response from debugger: ${JSON.stringify(args)}`);

@rchiodo Rich Chiodo (rchiodo) May 13, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added this to trace random failures in debugging. Still working on this.

disposable.dispose();
response.resolve(resp.body);
});
this.socket!.on('error', (err) => response.reject(err)); // NOSONAR
Expand Down
50 changes: 33 additions & 17 deletions src/test/datascience/dataScienceIocContainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,18 +1235,24 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
if (!this.mockJupyter) {
const interpreterService = this.serviceManager.get<IInterpreterService>(IInterpreterService);
const activeInterpreter = await interpreterService.getActiveInterpreter();
if (!activeInterpreter || !(await this.hasJupyter(activeInterpreter))) {
const list = await this.getJupyterInterpreters();
this.forceSettingsChanged(undefined, list[0].path);

// Log this all the time. Useful in determining why a test may not pass.
// tslint:disable-next-line: no-console
console.log(`Setting interpreter to ${list[0].displayName || list[0].path}`);

// Also set this as the interpreter to use for jupyter
await this.serviceManager
.get<JupyterInterpreterService>(JupyterInterpreterService)
.setAsSelectedInterpreter(list[0]);
if (!activeInterpreter || !(await this.hasFunctionalDependencies(activeInterpreter))) {
const list = await this.getFunctionalTestInterpreters();
if (list.length) {
this.forceSettingsChanged(undefined, list[0].path);

// Log this all the time. Useful in determining why a test may not pass.
// tslint:disable-next-line: no-console
console.log(`Setting interpreter to ${list[0].displayName || list[0].path} -> ${list[0].path}`);

// Also set this as the interpreter to use for jupyter
await this.serviceManager
.get<JupyterInterpreterService>(JupyterInterpreterService)
.setAsSelectedInterpreter(list[0]);
} else {
throw new Error(
'No jupyter capable interpreter found. Make sure you install all of the functional requirements before running a test'
);
}
}
}
}
Expand Down Expand Up @@ -1315,17 +1321,17 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
}

public async getJupyterCapableInterpreter(): Promise<PythonInterpreter | undefined> {
const list = await this.getJupyterInterpreters();
const list = await this.getFunctionalTestInterpreters();
return list ? list[0] : undefined;
}

public async getJupyterInterpreters(): Promise<PythonInterpreter[]> {
public async getFunctionalTestInterpreters(): Promise<PythonInterpreter[]> {
// This should be cacheable as we don't install new interpreters during tests
if (DataScienceIocContainer.jupyterInterpreters.length > 0) {
return DataScienceIocContainer.jupyterInterpreters;
}
const list = await this.get<IInterpreterService>(IInterpreterService).getInterpreters(undefined);
const promises = list.map((f) => this.hasJupyter(f).then((b) => (b ? f : undefined)));
const promises = list.map((f) => this.hasFunctionalDependencies(f).then((b) => (b ? f : undefined)));
const resolved = await Promise.all(promises);
DataScienceIocContainer.jupyterInterpreters = resolved.filter((r) => r) as PythonInterpreter[];
return DataScienceIocContainer.jupyterInterpreters;
Expand Down Expand Up @@ -1587,12 +1593,22 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
return '';
}

private async hasJupyter(interpreter: PythonInterpreter): Promise<boolean | undefined> {

@rchiodo Rich Chiodo (rchiodo) May 13, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Turns out having jupyter is too lenient. When testing we want only those interpreters that have everything installed that we need. (Seems azure machines can have jupyter in other environments too).

private async hasFunctionalDependencies(interpreter: PythonInterpreter): Promise<boolean | undefined> {
try {
const dependencyChecker = this.serviceManager.get<JupyterInterpreterDependencyService>(
JupyterInterpreterDependencyService
);
return dependencyChecker.areDependenciesInstalled(interpreter);
if (await dependencyChecker.areDependenciesInstalled(interpreter)) {
// Functional tests require livelossplot too. Make sure this interpreter has that value as well
const pythonProcess = await this.serviceContainer
.get<IPythonExecutionFactory>(IPythonExecutionFactory)
.createActivatedEnvironment({
resource: undefined,
interpreter,
allowEnvironmentFetchExceptions: true
});
return pythonProcess.isModuleInstalled('livelossplot'); // Should we check all dependencies?

@IanMatthewHuff Ian Huff (IanMatthewHuff) May 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing the comment here is since livelossplot is standing in for everything in conda-functional-requirements.txt? Seems ok for test code, having to check them all seems like a bit of overkill.

@rchiodo Rich Chiodo (rchiodo) May 13, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah. Livelossplot seemed uncommon for other things to install, so seemed like a good candidate for checking.

This does mean that if you run the functional tests locally, you'll have to install livelossplot into a least one environment for most of the tests to pass with real jupyter.

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

@IanMatthewHuff Ian Huff (IanMatthewHuff) May 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're on the topic, what do you do for setting up your local test env to run all the real python tests? Just this:
conda install --quiet -y --file build/ci/conda_base.yml
python -m pip install --upgrade -r build/conda-functional-requirements.txt

in a Conda env? Wasn't as sure since there were multiple conda envs getting set up as well in the test steps.

@rchiodo Rich Chiodo (rchiodo) May 14, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No I do it more manually.

  1. conda create --name newEnvName python=3.7 jupyter pip
  2. conda activate newEnvName
  3. python -m pip install --upgrade -r build/conda-functional-requirements.txt

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

}
} catch (ex) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/datascience/interactiveWindow.functional.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ Type: builtin_function_or_method`,
if (!ioc.mockJupyter) {
const interactiveWindowProvider = ioc.get<IInteractiveWindowProvider>(IInteractiveWindowProvider);
const interpreterService = ioc.get<IInterpreterService>(IInterpreterService);
const interpreters = await ioc.getJupyterInterpreters();
const interpreters = await ioc.getFunctionalTestInterpreters();
if (interpreters.length < 2) {
// tslint:disable-next-line: no-console
console.log(
Expand Down
3 changes: 2 additions & 1 deletion src/test/datascience/notebook.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,8 @@ suite('DataScience notebook tests', () => {
runTest('Remote Self Certs', async (_this: Mocha.Context) => {
const pythonService = await createPythonService(2);

if (pythonService) {
// Skip test for older python and on windows. Getting E_PROTO on windows.
if (pythonService && os.platform() !== 'win32') {

@rchiodo Rich Chiodo (rchiodo) May 13, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For now self certs seems to always fail on windows.

// We will only connect if we allow for self signed cert connections
ioc.getSettings().datascience.allowUnauthorizedRemoteConnection = true;

Expand Down