Skip to content

Use resource for starting interactive window#10139

Merged
rchiodo merged 17 commits into
masterfrom
rchiodo/use_resource
Feb 19, 2020
Merged

Use resource for starting interactive window#10139
rchiodo merged 17 commits into
masterfrom
rchiodo/use_resource

Conversation

@rchiodo

@rchiodo rchiodo commented Feb 14, 2020

Copy link
Copy Markdown

For #3123

Use the file associated with starting the interactive window to determine what interpreter/kernelspec should be the default.

@rchiodo rchiodo self-assigned this Feb 14, 2020
@rchiodo rchiodo changed the title Rchiodo/use resource Use resource for starting interactive window Feb 14, 2020
@codecov-io

codecov-io commented Feb 14, 2020

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@9f420a8). Click here to learn what that means.
The diff coverage is 35.48%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10139   +/-   ##
=========================================
  Coverage          ?   61.27%           
=========================================
  Files             ?      567           
  Lines             ?    30832           
  Branches          ?     4424           
=========================================
  Hits              ?    18893           
  Misses            ?    10956           
  Partials          ?      983
Impacted Files Coverage Δ
...ient/datascience/jupyter/kernels/kernelSwitcher.ts 86.95% <ø> (ø)
src/client/datascience/jupyter/jupyterExecution.ts 49.68% <ø> (ø)
...lient/datascience/jupyter/kernels/kernelService.ts 52.54% <ø> (ø)
src/client/telemetry/index.ts 87.38% <ø> (ø)
...reter/jupyterCommandInterpreterExecutionService.ts 68.88% <ø> (ø)
src/client/datascience/datascience.ts 67.6% <100%> (ø)
...cience/jupyter/interpreter/jupyterCommandFinder.ts 71.87% <100%> (ø)
...lient/datascience/jupyter/liveshare/serverCache.ts 66.66% <100%> (ø)
...rc/client/datascience/jupyter/jupyterConnection.ts 85.96% <100%> (ø)
.../datascience/interactive-common/interactiveBase.ts 16.74% <14.28%> (ø)
... and 3 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 9f420a8...3fd4a21. Read the comment docs.

);

runMountedTest(
'Multiple Interpreters',

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.

'Multiple Interpreters' [](start = 8, length = 23)

I added a note on the linked issue with the name of this test and the file.

@IanMatthewHuff IanMatthewHuff left a comment

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.

:shipit:

@DonJayamanne DonJayamanne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we need more changes, e.g. when calling IConfigurationService.getSettings we'll need to pass in the resource and the like.
Also, I'm now confused with the use of getNotebookIdentity. I.e. what's the difference between notebook identity and notebook resource.
How can a notebook resource change? (apart from a rename/save as).

E.g. what if we were to run a python file from two different workspaces with different python interpreters?
Does this mean we change kernels in the interactive window.
I.e. a multi root workspace with two folders, each with interpreter1 and interpreter2.
Now run file1 in interactive window, next run file2 in interactive window. What's the expected behavior?

Also, what if a user has jupyterServerURI in one of the workspace folders and not the other.

Do we need both notebook identity and a resource?

@rchiodo

rchiodo commented Feb 18, 2020

Copy link
Copy Markdown
Author

notebook identity is per window, notebook resource is the file that started the notebook. Because other things need to find the interactive notebook, it's a different value. Maybe the naming is wrong.

Your example with running two different files for the interactive window, the first one would win (as there's only 1 interactive window). It would pick the kernel from the first. Although I think you're likely right about the getConfig values. It would likely use the wrong jupyterServerUri.


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

@DonJayamanne

DonJayamanne commented Feb 18, 2020

Copy link
Copy Markdown

I think this needs to be turned into its own epic. After all, this issue is about supporting mult-root workspaces.
Considering the fact that we cannot have two interactive windows, this kinda complicates the solution that' we'd implement in this PR... i.e. how are things supposed to work

@rchiodo

rchiodo commented Feb 18, 2020

Copy link
Copy Markdown
Author

This issue is just about getting the interactive window to use the matching interpreter. We can enter another issue (and I believe we have one) for supporting multiple interactive windows.

@DonJayamanne

Copy link
Copy Markdown

This issue is just about getting the interactive window to use the matching interpreter.

Unfortunately I didn't word the issue properly. THa'ts the title. But if you look at my comments, I've stated that we don't pass resource to most methods, e.g. getSettings.

We can enter another issue (and I believe we have one) for supporting multiple interactive windows.

But that still doesn't solve the issue of getSettings not getting the right resource.

I.e. the issue is stuff will not work in multi-root workspaces.
Basically, I don't see any benefit with this PR if we're not going to support multi-root workspaces. Its only a partial fix, hence the request to create an epic.

It would pick the kernel from the first.

Also, this doesn't seem to be correct, if we're using resource. I.e. the question is, what does this PR solve?

@rchiodo

rchiodo commented Feb 18, 2020

Copy link
Copy Markdown
Author

This PR makes it so that the interactive window will pick the kernel based on the first file you run it from. That's what the issue is about. That's the only thing this fix does.

Comment thread src/client/datascience/jupyter/interpreter/jupyterCommandFinder.ts
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rchiodo rchiodo merged commit ba951e4 into master Feb 19, 2020
@rchiodo rchiodo deleted the rchiodo/use_resource branch February 19, 2020 21:18
@lock lock Bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants