Skip to content
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

Create new Venv for Python-based Project #3289

Merged
merged 9 commits into from
May 29, 2024

Conversation

sharon-wang
Copy link
Member

@sharon-wang sharon-wang commented May 28, 2024

Intent

Demo

new_venv_project.mp4

Approach

python extension

  • add interpreterPath to the CreateEnvironmentOptionsInternal options so we can skip user input via quickpick, as we'll already have the interpreterPath for a global python installation
  • modify VenvCreationProvider.createEnvironment() to use interpreterPath if provided
  • modify validateMetadata to return a corresponding registered runtimeMetadata based on the pythonPath -- this allows us to pass in a runtimeMetadata object which only contains a pythonPath and receive an actual filled-out runtimeMetadata object

new project service

  • add support for creating a new Python environment using Venv
  • install ipykernel in the created environment
  • autostart the interpreter initialized in the created environment

project wizard dialog

  • leverage the pythonEnvProviderName to omit Conda envs from the project config, since we don't currently support Conda (support will be added via Project Wizard: Python project environment step - Conda #2839)
  • only install ipykernel for existing interpreters, since new environments will be handled by the new project service

QA Notes

Suggested steps

  1. Open the New Project Wizard
  2. Select Python Project or Jupyter Notebook
  3. Project name/location should be inconsequential (defaults are fine)
  4. Select New Environment and use Venv as the provider
  5. Select any version of Python in the interpreter dropdown
  6. Create the Project

Expected outputs

  • once the new project window is loaded, a .venv directory should be created after a few moments, with ipykernel installed
  • the Venv interpreter (based on the selected version of Python in the project wizard) should be selected as the active interpreter
  • a blank Python/Jupyter file should be open
  • the Console should be functional and reflect the Venv interpreter

@sharon-wang sharon-wang marked this pull request as ready for review May 28, 2024 21:41
@sharon-wang sharon-wang requested a review from jmcphers May 28, 2024 21:41
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

LGTM modulo a couple of questions.

this._contextService.getWorkspace().folders[0];
if (!workspaceFolder) {
this._logService.error(
'Could not determine workspace folder for new project. Cannot create Python environment.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

If an error occurs that prevents us from creating the project's Python environment, I think we should show it to the user in a warning-level toast, instead of (or in addition to) logging it here, along with whatever context we can gather. Same with the errors below.

Copy link
Member Author

@sharon-wang sharon-wang May 29, 2024

Choose a reason for hiding this comment

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

Ah yes, we should surface the reasoning to users 👍

Some sample toasts that have been added:
image

In practice, only one of these would show at a time since the env creation would fail and exit once we hit one of these errors.

@@ -220,8 +220,11 @@ export class PythonRuntimeManager implements IPythonRuntimeManager {
throw new Error(`Python interpreter path is missing: ${extraData.pythonPath}`);
}

// Replace the metadata if we can find the runtime in the registered runtimes,
const registeredMetadata = this.registeredPythonRuntimes.get(extraData.pythonPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this rely on discovery happening before we validate? (seems like it might be necessary for us to hydrate the full metadata from the path to avoid this but maybe I'm missing something)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we won't have done discovery yet, but since we call validateMetadata in doAutoStartRuntime, we'll receive the full metadata at that point.

The full metadata will be registered (in the extension and in positron) upon the creation of the Venv through the python extension. We won't have the runtime manager info in positron, but positron will ask for the manager via the extension host proxy.

I think this means we won't need to rely on discover to occur first!

I added a commit to log when we hydrate the metadata in runtimeSession.ts doAutoStartRuntime()

Env tasks can take a while, but creating the blank files is quick. This way, the blank files are interact-able while the env is set up in the background.
@sharon-wang
Copy link
Member Author

A note from Wasim to double-check the new Venv interpreter startup behaviour here, once #3298 is resolved. The changes there will auto-start the newly created Venv interpreter, which may result in some weirdness when we try to start the affiliated runtime here.

I'll rebase this PR once the new project git init PR is merged, then merge this if no further comments!

@sharon-wang sharon-wang force-pushed the feature/project-wizard-new-venv branch from 47baee1 to f0774e0 Compare May 29, 2024 19:33
@sharon-wang sharon-wang merged commit ff35c6a into main May 29, 2024
21 checks passed
@sharon-wang sharon-wang deleted the feature/project-wizard-new-venv branch May 29, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants