-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: persist conda environment state across execute_command calls #6629
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { inspect } from "util" | |||||||||||||||||||||||||
| import type { ExitCodeDetails } from "./types" | ||||||||||||||||||||||||||
| import { BaseTerminalProcess } from "./BaseTerminalProcess" | ||||||||||||||||||||||||||
| import { Terminal } from "./Terminal" | ||||||||||||||||||||||||||
| import { TerminalRegistry } from "./TerminalRegistry" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export class TerminalProcess extends BaseTerminalProcess { | ||||||||||||||||||||||||||
| private terminalRef: WeakRef<Terminal> | ||||||||||||||||||||||||||
|
|
@@ -47,6 +48,10 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||||||||||||
| public override async run(command: string) { | ||||||||||||||||||||||||||
| this.command = command | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Check if this is a conda activate/deactivate command | ||||||||||||||||||||||||||
| const condaActivateMatch = command.match(/^\s*conda\s+activate\s+(.+)$/i) | ||||||||||||||||||||||||||
| const condaDeactivateMatch = command.match(/^\s*conda\s+deactivate\s*$/i) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const terminal = this.terminal.terminal | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const isShellIntegrationAvailable = terminal.shellIntegration && terminal.shellIntegration.executeCommand | ||||||||||||||||||||||||||
|
|
@@ -248,6 +253,24 @@ export class TerminalProcess extends BaseTerminalProcess { | |||||||||||||||||||||||||
| this.fullOutput = match | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Update the terminal's active environment based on the command and output | ||||||||||||||||||||||||||
| if (condaActivateMatch) { | ||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentional? The conda activate/deactivate detection logic is duplicated here and in ExecaTerminalProcess.ts. Could we extract this into a shared utility function to maintain DRY principles?
Suggested change
|
||||||||||||||||||||||||||
| // Extract the environment name from the command | ||||||||||||||||||||||||||
| const envName = condaActivateMatch[1].trim() | ||||||||||||||||||||||||||
| // Check if activation was successful by looking for common success indicators in output | ||||||||||||||||||||||||||
| const cleanOutput = this.removeEscapeSequences(this.fullOutput).toLowerCase() | ||||||||||||||||||||||||||
| if (!cleanOutput.includes("error") && !cleanOutput.includes("not found")) { | ||||||||||||||||||||||||||
| this.terminal.activeEnvironment = envName | ||||||||||||||||||||||||||
| TerminalRegistry.setLastActiveEnvironment(envName) | ||||||||||||||||||||||||||
| console.log(`[TerminalProcess] Conda environment activated: ${envName}`) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } else if (condaDeactivateMatch) { | ||||||||||||||||||||||||||
| // Clear the active environment on deactivation | ||||||||||||||||||||||||||
| this.terminal.activeEnvironment = undefined | ||||||||||||||||||||||||||
| TerminalRegistry.setLastActiveEnvironment(undefined) | ||||||||||||||||||||||||||
| console.log("[TerminalProcess] Conda environment deactivated") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // For now we don't want this delaying requests since we don't send | ||||||||||||||||||||||||||
| // diagnostics automatically anymore (previous: "even though the | ||||||||||||||||||||||||||
| // command is finished, we still want to consider it 'hot' in case | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ export class TerminalRegistry { | |
| private static nextTerminalId = 1 | ||
| private static disposables: vscode.Disposable[] = [] | ||
| private static isInitialized = false | ||
| private static lastActiveEnvironment?: string | ||
|
|
||
| public static initialize() { | ||
| if (this.isInitialized) { | ||
|
|
@@ -160,7 +161,7 @@ export class TerminalRegistry { | |
| let terminal: RooTerminal | undefined | ||
|
|
||
| // First priority: Find a terminal already assigned to this task with | ||
| // matching directory. | ||
| // matching directory and environment. | ||
| if (taskId) { | ||
| terminal = terminals.find((t) => { | ||
| if (t.busy || t.taskId !== taskId || t.provider !== provider) { | ||
|
|
@@ -173,11 +174,34 @@ export class TerminalRegistry { | |
| return false | ||
| } | ||
|
|
||
| return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd) | ||
| // Prefer terminals with matching environment | ||
| const envMatch = this.lastActiveEnvironment ? t.activeEnvironment === this.lastActiveEnvironment : true | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a potential race condition here? If multiple commands are executed simultaneously, the terminal selection logic might pick a terminal that's about to become busy. Should we consider adding some form of locking or reservation mechanism to prevent this? |
||
|
|
||
| return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd) && envMatch | ||
| }) | ||
| } | ||
|
|
||
| // Second priority: Find any available terminal with matching directory. | ||
| // Second priority: Find any available terminal with matching directory and environment. | ||
| if (!terminal) { | ||
| terminal = terminals.find((t) => { | ||
| if (t.busy || t.provider !== provider) { | ||
| return false | ||
| } | ||
|
|
||
| const terminalCwd = t.getCurrentWorkingDirectory() | ||
|
|
||
| if (!terminalCwd) { | ||
| return false | ||
| } | ||
|
|
||
| // Prefer terminals with matching environment | ||
| const envMatch = this.lastActiveEnvironment ? t.activeEnvironment === this.lastActiveEnvironment : true | ||
|
|
||
| return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd) && envMatch | ||
| }) | ||
| } | ||
|
|
||
| // Third priority: Find any available terminal with matching directory (ignore environment). | ||
| if (!terminal) { | ||
| terminal = terminals.find((t) => { | ||
| if (t.busy || t.provider !== provider) { | ||
|
|
@@ -194,8 +218,14 @@ export class TerminalRegistry { | |
| }) | ||
| } | ||
|
|
||
| // Third priority: Find any non-busy terminal (only if directory is not | ||
| // required). | ||
| // Fourth priority: Find any non-busy terminal with matching environment (only if directory is not required). | ||
| if (!terminal && !requiredCwd && this.lastActiveEnvironment) { | ||
| terminal = terminals.find( | ||
| (t) => !t.busy && t.provider === provider && t.activeEnvironment === this.lastActiveEnvironment, | ||
| ) | ||
| } | ||
|
|
||
| // Fifth priority: Find any non-busy terminal (only if directory is not required). | ||
| if (!terminal && !requiredCwd) { | ||
| terminal = terminals.find((t) => !t.busy && t.provider === provider) | ||
| } | ||
|
|
@@ -333,4 +363,21 @@ export class TerminalRegistry { | |
| ShellIntegrationManager.zshCleanupTmpDir(id) | ||
| this.terminals = this.terminals.filter((t) => t.id !== id) | ||
| } | ||
|
|
||
| /** | ||
| * Updates the last active conda environment | ||
| * @param environment The environment name or undefined to clear | ||
| */ | ||
| public static setLastActiveEnvironment(environment?: string): void { | ||
| this.lastActiveEnvironment = environment | ||
| console.log(`[TerminalRegistry] Last active environment updated: ${environment || "none"}`) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these console.log statements be behind a debug flag or use a proper logging framework? They might clutter production logs. |
||
| } | ||
|
|
||
| /** | ||
| * Gets the last active conda environment | ||
| * @returns The last active environment name or undefined | ||
| */ | ||
| public static getLastActiveEnvironment(): string | undefined { | ||
| return this.lastActiveEnvironment | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error detection only checks for "error" and "not found" in the output. What about other failure scenarios like permission issues ("Permission denied") or conda not being in PATH ("command not found")? Should we consider a more robust approach, perhaps checking for success indicators instead?