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

Spike: Investigate approach to use environment variables for environment activation #20492

Closed
13 tasks done
karrtikr opened this issue Jan 11, 2023 · 9 comments
Closed
13 tasks done
Assignees
Labels
area-debugging area-environments Features relating to handling interpreter environments area-terminal feature-request Request for new features or functionality needs spike Label for issues that need investigation before they can be worked on. verified Verification succeeded
Milestone

Comments

@karrtikr
Copy link

karrtikr commented Jan 11, 2023

For #11039

Getting activated environment variables from conda

Get activated environment variables for each shell.

  • Variables for each shell might be different. For eg. the prompt variable.
  • Shell path to conda for each shell might be different. For eg. cmd might be conda init already but powershell might not be.

Diff is another option. Can we use diff for cmd in powershell, and add the prompt variable ourselves?

Use environment variable collection to activate terminals

Blockers: microsoft/vscode#99878 on macOS

  • Handling the debugger python config scenario
    • Which one takes precedence: environment var collection or env vars run in terminal request or envs supplied when creating terminal
    • Kind of like internal console, except for rc and profile scripts?
    • createTerminal(env) overrides environment variable collection and env in runinterminal request also affects the terminal.
  • Cleanup collection prompt
    image
    Also there are some meaningless changes: like true to 1.
    Instead of this would it make more sense to user to just say: Terminal needed to be restarted to apply results of <conda command>: Allow to provide description to environment collection API vscode#171108
    Some other alternatives:
    • Title of environment variable collection to shown the first time.
    • Either in the prompt UI or when terminal starts up.
      image

What about multiroot workspaces?

Environment variable collection for a workspace?

Clarify what:

Note that an extension can only make a single change to any one variable, so this will overwrite any previous calls to replace, append or prepend.

means.

Relaunch terminal doesn't show up always.

on did change default shell event? microsoft/vscode#160694 (comment)
onDidOpenTerminal and vscode.env.shell: microsoft/vscode#171164

@karrtikr karrtikr added feature-request Request for new features or functionality area-debugging area-terminal area-environments Features relating to handling interpreter environments needs spike Label for issues that need investigation before they can be worked on. labels Jan 11, 2023
@karrtikr karrtikr self-assigned this Jan 11, 2023
@karrtikr
Copy link
Author

karrtikr commented Jan 11, 2023

Multiroot scenarios:

Each workspace can have a different environment selected. How to decide which one applies to the collection? Options:

  1. Active editor decides the Python which appears in the status bar:
    image
    Every time active editor changes, the environment collection is updated to contain fresh environment variables.
    Problems:

    • Prompt shows up asking to relaunch the terminal:
      image
    • Terminal cwd also dictates which environment is to be activated. For eg. in the following case the second workspace is not using julia but the julia is activated in the terminal for second workspace:
      terminal
  2. Active terminal decides which Python to activate using environment collection:
    image
    If cwd of first workspace is active, the environment of first workspace is applied.

    Problems:

    • User try opening the Python file from the second workspace and runs it, it ends up running in the activated environment of the first workspace:
      image

Potential requests from VSCode:

In the second case, if env option in terminal.createTerminal API overrides the environment set by collection, we can use the right environment. The prompt to relaunch terminal is also not expected to show in this case.

To make the first case work would require a more convoluted solution from VSCode, for eg. environment collection only applies to a particular workspace.

For simplicity it would also work if we could disable prompts altogether in multiroot workspaces:
image

@Tyriar
Copy link
Member

Tyriar commented Jan 11, 2023

@karrtikr am I understanding this right that the environment variable collection is applied to the workspace, not the individual folder in the workspace? Would allowing to apply a particular workspace to the EnvironmentVariableMutator solve that issue?

The idea of env overriding seems like a scary breaking change.

@karrtikr
Copy link
Author

am I understanding this right that the environment variable collection is applied to the workspace, not the individual folder in the workspace?

Yes

Would allowing to apply a particular workspace to the EnvironmentVariableMutator solve that issue?

I assume you also mean existing APIs like replace, append etc. will have to be extended to be workspace folder specific as well, to indicate which workspace folder the changes are applied to. If so then yes this would be great.

@karrtikr
Copy link
Author

karrtikr commented Jan 12, 2023

Fish shell scenario:

Fish shell does not have a prompt variable which we could set like cmd to indicate which environment is active:

image

Potential requests from VSCode:

  • One idea is to use the terminal.title property to indicate the environment is activated: Allow to provide description to environment collection API vscode#171108

  • Another idea could be to use shell integration script to insert an environment variable which could influence the prompt, inspiration taken from official activate.fish script:

    code --locate-shell-integration-path fish
    

    If the above script is amended to support something similar to:

    functions -c fish_prompt _old_fish_prompt
    
    # With the original prompt function renamed, we can override with our own.
    function fish_prompt
        # Save the return status of the last command.
        set -l old_status $status
    
        # Output the venv prompt; color taken from the blue of the Python logo.
        printf "%s%s%s" (set_color 4B8BBE) "$VSCODE_PS1 " (set_color normal)
    
        # Restore the return status of the previous command.
        echo "exit $old_status" | .
        # Output the original/"old" prompt.
        _old_fish_prompt
    end

    Variable names would have to unique enough, but this way we can directly edit the VSCODE_PS1 variable to influence the prompt.

@karrtikr
Copy link
Author

Debugger scenario:

We provide a way to debug a python file using its own debugger python that maybe different than what is currently selected:

However the collection is activated using the selected environment, so when debugging the file the incorrect activated shell is used (julia is selected environment):

image

Solution:

Add env property to send environment variables of the debugger python environment:
image

This is passed down using RunInTerminalRequest to VSCode, so the user code is debugged using the expected environment variables. The downside is that if the terminal remains after the debug session completes, user might try to interact with it, and be confused because the shell therein is incorectly activated.

  • Potential VSCode request:

    One solution could be to for VSCode to use the env provided by RunInTerminalRequest to also apply to the Python Debug console terminal that remains after debugging is done, instead of just using it to debug user code. Variables can be set when spawning the shell itself, but it is expected to override environment collection in this case.

  • Treat terminal as single use:

    In this approach, the lifetime of the shell in the terminal is tied to the lifetime of the debug session. We can use “wait on exit” debugger feature, which causes the debug session to present a “Press any key to continue…” prompt at the end, and session is not considered terminated until the prompt is answered.

@Tyriar
Copy link
Member

Tyriar commented Jan 12, 2023

I assume you also mean existing APIs like replace, append etc. will have to be extended to be workspace folder specific as well, to indicate which workspace folder the changes are applied to. If so then yes this would be great.

The trick would be coming up with a way to do it in a backwards compatible way so we don't break the API. Haven't thought about it too much but for example:

export interface EnvironmentVariableMutator {
	readonly type: EnvironmentVariableMutatorType;
	readonly value: string;
	readonly workspace: Workspace | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	replace(variable: string, value: string, workspace?: Workspace): void;
	append(variable: string, value: string, workspace?: Workspace): void;
	prepend(variable: string, value: string, workspace?: Workspace): void;
}

Tracked in microsoft/vscode#171173

@Tyriar
Copy link
Member

Tyriar commented Jan 12, 2023

One idea is to use the terminal.title property to indicate the environment is activated: microsoft/vscode#171108

What if we allowed an extension to control a variable that is fed into both the title and description settings?

image

Tracked in microsoft/vscode#171175

@karrtikr
Copy link
Author

Seems reasonable to me, we can come back to it based on user feedback if need be:

image

@karrtikr
Copy link
Author

karrtikr commented Mar 8, 2023

Created action item based on spike: #20822

@karrtikr karrtikr closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2023
@karrtikr karrtikr added the verified Verification succeeded label Mar 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-debugging area-environments Features relating to handling interpreter environments area-terminal feature-request Request for new features or functionality needs spike Label for issues that need investigation before they can be worked on. verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants