Restore UseTerminalToGetActivatedEnvVars experiment

[karrtikr]

Original issue https://github.com/microsoft/vscode-python/issues/8928

Today we use IEnvironmentActivationService to generate environment variables for activated environments. E.g. when running python code on behalf of user as a background process.

We do this today by activating the environment and capturing the environment variables.
For this we get the activation commands (using a default shell) and run it as a process.

A better approach is:

• Create a hidden terminal
• Get activation command and send to terminal
• Dump variables to text file (instead of stdout).

Solves

• unicode issues (today scraping stdout unicode)
• Activates in terminal (thats what some activation command expect, i.e. to be run within a shell).

The experiment was temporarily disabled #9967 and old behavior was restored #9967 until we update to use the latest VSC API which allows us to hide terminal.

That API is now available, so this should be restored. This solves various issues like #12739 and some datascience issues like https://github.com/microsoft/vscode-python/issues/8823 where getting environment variables within the activated environment using process fails.

cc @DonJayamanne @greazer @rchiodo

[karrtikr]

Concerns with hidden terminal approach

There are been some concerns with using terminals if we don't absolutely need them. For eg.

 // Sometimes the terminal takes some time to start up before it can start accepting input.
 await new Promise(resolve => setTimeout(resolve, 100));

protected async waitForCommandToProcess(_shell: TerminalShellType) {
    // Give the command some time to complete.
    // Its been observed that sending commands too early will strip some text off in VS Code Terminal.
    await sleep(500);
}

Handling temporary terminals, disposing them correctly, disposing the temporary files created to extract output, are also a few additional things to lookout for.

Spike to decide whether to go with spawning processes or use hidden terminals to get the info

One main issue with using spawning processes is that we only do it in the default shells for now - not the one that user has selected. IOW it's not running under the same shell that the actual terminal will be, which can be confusing for users.

// The shell under which we'll execute activation scripts.
const defaultShells = {
    [OSType.Windows]: { shell: 'cmd', shellType: TerminalShellType.commandPrompt },
    [OSType.OSX]: { shell: 'bash', shellType: TerminalShellType.bash },
    [OSType.Linux]: { shell: 'bash', shellType: TerminalShellType.bash },
    [OSType.Unknown]: undefined
};

The following needs further investigation:

• NodeJS child_process.exec which is used to spawn the commands is known to work for cmd. Not sure if it has reliable support for powershell.

• Collect all issues similar to https://github.com/microsoft/vscode-python/issues/8823 with similar error messages with Error: Activating Python 3.7.5 64-bit ('torch': conda) to run Jupyter failed with Error: Command failed. Even the closed ones. Make a list of errors due to which the command failed.
  Some of them I can recall until now,

  • Unicode issues (today scraping stdout unicode) https://github.com/microsoft/vscode-python/issues/8823
  • Activation failing in process shell but not when using the terminal user has. This could be because process shells are not initialized to use the environment, meanwhile the configured terminal was? (If everything planned with conda init works, we may not have to worry about it)
  • Because user didn't have permissions to execute scripts within cmd due to group policy restrictions Jupyter doesn't use the terminal configuration to start its server #12739 Restore UseTerminalToGetActivatedEnvVars experiment #13071 Unable to select the python interpreter if Command prompt is not supported #17848 (comment)
  • A tty interface is required by either pipenv or pyenv. Can't remember which one it was. I also remember seeing a similar error message with conda. We don't have this issue with using hidden terminals.
  • The shell itself requires a tty interface due to a possibly corrupt setup Activating to run Jupyter failed #11862 (comment)

Using hidden terminals will make sure the activation scripts will run in the exact same environment that would other wise run when users run their code. So users can debug failing commands easily by just using the active terminal. Right now they can't debug why a command failed in the process.

cc @int19h

[int19h]

For Unicode issues, we should be able to control the output encoding, since we control the environment, and can set LC_CTYPE, PYTHONIOENCODING etc as we see fit. We can also author the Python script itself to explicitly use the desired encoding - e.g. on Python 3, using encode("utf-8") and then writing the resulting bytes to sys.stdout.buffer, rather than letting sys.stdout do its thing. Or we can use the same approach with temp files instead of stdout - it's orthogonal to whether the process is spawned in a terminal or not.

Spawning the shell directly should be no different from it being run in the terminal wrt initialization/activation, if we do everything correctly. This means respecting "shell" and "shellArgs" settings, in particular (e.g. -l can make a world of difference for bash!), and using the same defaults as VSC does for "shellArgs".

The issue with group policy restrictions, or really any other configuration problems with a particular shell, mean that we can't hardcode that one shell, even for automated tools. There are other issues with this, anyway - moreso on non-Windows platforms where shells handle stuff like per-user environment variables (and so us using a different one means a different environment). But this, again, basically just means respecting "shell".

So the biggest sticking point is that some activation scripts appear to require a tty. I don't believe that to be the case for conda - definitely not for conda shell.activate, because conda itself runs that command with stdout redirected. So if we use that approach there - which we wanted to do in any case, because it lets us avoid a dependency on conda init - we should be good on conda.

But if pyenv and/or pipenv require a tty in principle (i.e. not just because of the way we are using them), there would be no way to avoid a terminal for those. And if we have it in the picture, we might as well use it uniformly for conda etc as well. Still, there ought to be a better way to reliably use that, which doesn't require introducing artificial delays to avoid synchronization issues. Maybe there's something in the Pseudoterminal VSCode API to that effect, although I'm not seeing it.

[int19h]

pipenv shell does indeed require a tty, because it spawns a subshell (unlike conda activate, which changes the environment of the shell it runs on).

However, there's also pipenv run, which is more appropriate for this use case - and unlike conda run, it works correctly wrt stdin/out. So we should just use that to run the Python script that dumps the environment.

[int19h]

For pyenv, we can use the recipe from pyenv/pyenv#1028:

PYENV_VERSION=2.7.14 pyenv exec python ...

[karrtikr]

A tty interface is required by either pipenv or pyenv. Can't remember which one it was. I also remember seeing a similar error message with conda.

@int19h Found issue #11862, where the environment required tty. The user wrote "Python3 installed and working 3.6.8, conda installed", which is why I thought the environment maybe conda. Hard to tell what environment it was as user didn't provide further details.

[jpdehollain]

Just wondering if this issue is still in the pipeline to implement? I'm still interested! :)

[karrtikr]

Another possible stty issue: #14938

[jpdehollain]

FYI, after the latest update, I ran a Jupyter Notebook on VSC and it started the kernel and ran cells with no problems!
There have been no admin changes on my side, so it must be that VSC is now properly using the terminal configuration to run its kernel scripts.

[luabud]

@karrtikr could this be closed?

[karrtikr]

Yep, closing as conda run is working quite well for now so we do not run into this.