Skip to content

Create environment using venv or conda - #19848

Merged
Karthik Nadig (karthiknadig) merged 31 commits into
microsoft:mainfrom
karthiknadig:create_environment
Sep 23, 2022
Merged

Create environment using venv or conda#19848
Karthik Nadig (karthiknadig) merged 31 commits into
microsoft:mainfrom
karthiknadig:create_environment

Conversation

@karthiknadig

@karthiknadig Karthik Nadig (karthiknadig) commented Sep 16, 2022

Copy link
Copy Markdown
Member

Closes #19676
Closes #19850

@karthiknadig Karthik Nadig (karthiknadig) added the feature-request Request for new features or functionality label Sep 16, 2022
@karthiknadig Karthik Nadig (karthiknadig) added the skip package*.json package.json and package-lock.json don't both need updating label Sep 16, 2022
);
}

export namespace CreateEnv {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Luciana Abud (@luabud) Strings here need your input.

@karrtikr Kartik Raj (karrtikr) 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.

Neatly done, most comments are similar related to wording.

Btw, are we not doing the "show log" hyperlink? Luciana Abud (@luabud)

image

Comment thread pythonFiles/create_conda.py
Comment thread pythonFiles/create_conda.py

def get_venv_path(name: str) -> str:
if sys.platform == "win32":
return os.fspath(CWD / name / "Scripts" / "python.exe")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any documentation for this assumption?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the same assumption we have in the extension. The docs here describe the "Scripts" or "bin" : https://docs.python.org/3/library/venv.html#creating-virtual-environments

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gotcha.

Maybe still add a one line comment linking the doc? I'm asking because it looks like we actually don't use that assumption in discovery and identification:

function getPyvenvConfigPathsFrom(interpreterPath: string): string[] {
const pyvenvConfigFile = 'pyvenv.cfg';
// Check if the pyvenv.cfg file is in the parent directory relative to the interpreter.
// env
// |__ pyvenv.cfg <--- check if this file exists
// |__ bin or Scripts
// |__ python <--- interpreterPath
const venvPath1 = path.join(path.dirname(path.dirname(interpreterPath)), pyvenvConfigFile);
// Check if the pyvenv.cfg file is in the directory as the interpreter.
// env
// |__ pyvenv.cfg <--- check if this file exists
// |__ python <--- interpreterPath
const venvPath2 = path.join(path.dirname(interpreterPath), pyvenvConfigFile);
// The paths are ordered in the most common to least common
return [venvPath1, venvPath2];
}

and this is the first time.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The purpose of the code here and in the discovery is different. In discovery we are tyring to find the .cfg file. in the create env, we created the env, so we know what type it is and where the python.exe will be. I can add the docs nonetheless.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, I think the comment is misleading:

// envFolder
// |__ pyvenv.cfg  <--- check if this file exists
// |__ python  <--- interpreterPath

It seems to indicate python binary can be directly under the env folder.

Comment thread src/client/common/utils/localize.ts
Comment thread src/client/common/utils/localize.ts Outdated
Comment thread src/client/common/utils/localize.ts Outdated
Comment thread src/client/common/utils/localize.ts
Comment thread src/client/common/utils/localize.ts Outdated
Comment thread src/client/pythonEnvironments/creation/types.ts
Comment thread pythonFiles/create_venv.py

@karrtikr Kartik Raj (karrtikr) 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.

Thanks for addressing, LGTM. I still think we could do the hyperlink to log channel #19848 (review) given it so nicely shows the command progress, otherwise looks good.

@karthiknadig

Copy link
Copy Markdown
Member Author

I still think we could do the hyperlink to log channel

I will definitely add that in an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-request Request for new features or functionality skip package*.json package.json and package-lock.json don't both need updating

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Create Environment command. Investigate the possible flow for a Create Environment command

3 participants