Only trigger auto environment discovery if a user attempts to choose a different interpreter, or when a particular scope is opened for the first time (microsoft/vscode-python#19162)

* Add progress API

* Remove old API in favor of this

* Revert "Remove old API in favor of this"

This reverts commit 9543501576cd063e6aad1ef3ddd7a39cb3354b22.

* Revert "Revert "Remove old API in favor of this""

This reverts commit 5bbea78879f2447c6783d7ad98610d4af36abf29.

* Fix

* Remove old API impl

* Add to proposed API

* Add get refresh promise options

* Change getter to function

* Translate progress events to progress promises

* Fix combining iterators

* Fix tests

* Add test for resolver

* Add test for reducer

* Fixes

* Add tests for environment collection service

* News entry

* Add another test if a query is provided

* Add comments and clarify

* Fix bug

* Use extensionUrl instead of extensionPath (microsoft/vscode-python#19122)

* Only trigger auto environment discovery once in the first session for a particular scope

* Oops

* Ensure old Jupyter APIs still have the old behavior

* Trigger auto-discovery if a user is attempting to choose a different interpreter

* Fix tests

* Temp

* Add an option to trigger refresh for the session

* Fix bugs

* Fix tests

* Remove trigger from telemetry

* Add tests for options

Co-authored-by: Karthik Nadig <[email protected]>

Author: 2 people

extensions/positron-python/news/1 Enhancements/19102.md

@@ -0,0 +1 @@
+Only trigger auto environment discovery if a user attempts to choose a different interpreter, or when a particular scope (a workspace folder or globally) is opened for the first time.

extensions/positron-python/src/client/interpreter/autoSelection/index.ts

@@ -200,7 +200,8 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio
             });
         }
 
-        const interpreters = await this.interpreterService.getAllInterpreters(resource);
+        await this.interpreterService.refreshPromise;
+        const interpreters = this.interpreterService.getInterpreters(resource);
         const workspaceUri = this.interpreterHelper.getActiveWorkspaceUri(resource);
 
         const recommendedInterpreter = this.envTypeComparer.getRecommended(interpreters, workspaceUri?.folderUri);

extensions/positron-python/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts

@@ -111,6 +111,10 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
         // times so that the visible items do not change.
         const preserveOrderWhenFiltering = !!this.interpreterService.refreshPromise;
         const suggestions = this.getItems(state.workspace);
+        // Discovery is no longer guranteed to be auto-triggered on extension load, so trigger it when
+        // user interacts with the interpreter picker but only once per session. Users can rely on the
+        // refresh button if they want to trigger it more than once.
+        this.interpreterService.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors();
         state.path = undefined;
         const currentInterpreterPathDisplay = this.pathUtils.getDisplayName(
             this.configurationService.getSettings(state.workspace).pythonPath,
@@ -134,7 +138,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
                     iconPath: getIcon(REFRESH_BUTTON_ICON),
                     tooltip: InterpreterQuickPickList.refreshInterpreterList,
                 },
-                callback: () => this.interpreterService.triggerRefresh(undefined, 'ui').ignoreErrors(),
+                callback: () => this.interpreterService.triggerRefresh().ignoreErrors(),
             },
             onChangeItem: {
                 event: this.interpreterService.onDidChangeInterpreters,
@@ -375,7 +379,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand {
         if (!targetConfig) {
             return;
         }
-
         const { configTarget } = targetConfig[0];
         const wkspace = targetConfig[0].folderUri;
         const interpreterState: InterpreterStateArgs = { path: undefined, workspace: wkspace };

extensions/positron-python/src/client/interpreter/configuration/types.ts

@@ -29,6 +29,9 @@ export interface IInterpreterSelector extends Disposable {
         suggestions: IInterpreterQuickPickItem[],
         resource: Resource,
     ): IInterpreterQuickPickItem | undefined;
+    /**
+     * @deprecated Only exists for old Jupyter integration.
+     */
     getAllSuggestions(resource: Resource): Promise<IInterpreterQuickPickItem[]>;
     getSuggestions(resource: Resource, useFullDisplayName?: boolean): IInterpreterQuickPickItem[];
     suggestionToQuickPickItem(

extensions/positron-python/src/client/interpreter/contracts.ts

@@ -3,7 +3,11 @@ import { CodeLensProvider, ConfigurationTarget, Disposable, Event, TextDocument,
 import { FileChangeType } from '../common/platform/fileSystemWatcher';
 import { Resource } from '../common/types';
 import { PythonEnvSource } from '../pythonEnvironments/base/info';
-import { ProgressNotificationEvent, PythonLocatorQuery } from '../pythonEnvironments/base/locator';
+import {
+    ProgressNotificationEvent,
+    PythonLocatorQuery,
+    TriggerRefreshOptions,
+} from '../pythonEnvironments/base/locator';
 import { CondaEnvironmentInfo } from '../pythonEnvironments/common/environmentManagers/conda';
 import { EnvironmentType, PythonEnvironment } from '../pythonEnvironments/info';
 
@@ -17,7 +21,7 @@ export type PythonEnvironmentsChangedEvent = {
 export const IComponentAdapter = Symbol('IComponentAdapter');
 export interface IComponentAdapter {
     readonly onProgress: Event<ProgressNotificationEvent>;
-    triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise<void>;
+    triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise<void>;
     getRefreshPromise(): Promise<void> | undefined;
     readonly onChanged: Event<PythonEnvironmentsChangedEvent>;
     // VirtualEnvPrompt
@@ -63,14 +67,17 @@ export interface ICondaService {
 
 export const IInterpreterService = Symbol('IInterpreterService');
 export interface IInterpreterService {
-    triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise<void>;
+    triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise<void>;
     readonly refreshPromise: Promise<void> | undefined;
     readonly onDidChangeInterpreters: Event<PythonEnvironmentsChangedEvent>;
     onDidChangeInterpreterConfiguration: Event<Uri | undefined>;
     onDidChangeInterpreter: Event<void>;
     onDidChangeInterpreterInformation: Event<PythonEnvironment>;
     hasInterpreters(filter?: (e: PythonEnvironment) => Promise<boolean>): Promise<boolean>;
     getInterpreters(resource?: Uri): PythonEnvironment[];
+    /**
+     * @deprecated Only exists for old Jupyter integration.
+     */
     getAllInterpreters(resource?: Uri): Promise<PythonEnvironment[]>;
     getActiveInterpreter(resource?: Uri): Promise<PythonEnvironment | undefined>;
     getInterpreterDetails(pythonPath: string, resoure?: Uri): Promise<undefined | PythonEnvironment>;

extensions/positron-python/src/client/interpreter/interpreterService.ts

@@ -20,7 +20,6 @@ import {
     IInterpreterStatusbarVisibilityFilter,
     PythonEnvironmentsChangedEvent,
 } from './contracts';
-import { PythonLocatorQuery } from '../pythonEnvironments/base/locator';
 import { traceError, traceLog } from '../logging';
 import { Commands, PYTHON_LANGUAGE } from '../common/constants';
 import { reportActiveInterpreterChanged } from '../proposedApi';
@@ -29,6 +28,7 @@ import { Interpreters } from '../common/utils/localize';
 import { sendTelemetryEvent } from '../telemetry';
 import { EventName } from '../telemetry/constants';
 import { cache } from '../common/utils/decorators';
+import { PythonLocatorQuery, TriggerRefreshOptions } from '../pythonEnvironments/base/locator';
 
 type StoredPythonEnvironment = PythonEnvironment & { store?: boolean };
 
@@ -40,11 +40,8 @@ export class InterpreterService implements Disposable, IInterpreterService {
         return this.pyenvs.hasInterpreters(filter);
     }
 
-    public triggerRefresh(
-        query?: PythonLocatorQuery & { clearCache?: boolean },
-        trigger?: 'auto' | 'ui',
-    ): Promise<void> {
-        return this.pyenvs.triggerRefresh(query, trigger);
+    public triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise<void> {
+        return this.pyenvs.triggerRefresh(query, options);
     }
 
     public get refreshPromise(): Promise<void> | undefined {
@@ -140,6 +137,10 @@ export class InterpreterService implements Disposable, IInterpreterService {
     }
 
     public async getAllInterpreters(resource?: Uri): Promise<PythonEnvironment[]> {
+        // For backwards compatibility with old Jupyter APIs, ensure a
+        // fresh refresh is always triggered when using the API. As it is
+        // no longer auto-triggered by the extension.
+        this.triggerRefresh(undefined, { ifNotTriggerredAlready: true }).ignoreErrors();
         await this.refreshPromise;
         return this.getInterpreters(resource);
     }
@@ -211,7 +212,7 @@ export class InterpreterService implements Disposable, IInterpreterService {
             traceLog('Conda envs without Python are known to not work well; fixing conda environment...');
             const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath));
             shell.withProgress(progressOptions, () => promise);
-            promise.then(() => this.triggerRefresh({ clearCache: true }).ignoreErrors());
+            promise.then(() => this.triggerRefresh(undefined, { clearCache: true }).ignoreErrors());
         }
     }
 }

extensions/positron-python/src/client/proposedApi.ts

@@ -102,7 +102,7 @@ export function buildProposedApi(
                 return interpreterPathService.update(resource, ConfigurationTarget.WorkspaceFolder, path);
             },
             async refreshEnvironment(options?: RefreshEnvironmentsOptions) {
-                await discoveryApi.triggerRefresh(options ? { clearCache: options.clearCache } : undefined);
+                await discoveryApi.triggerRefresh(undefined, options ? { clearCache: options.clearCache } : undefined);
                 const paths = discoveryApi.getEnvs().map((e) => getEnvPath(e.executable.filename, e.location));
                 return Promise.resolve(paths);
             },

extensions/positron-python/src/client/pythonEnvironments/api.ts

@@ -2,15 +2,12 @@
 // Licensed under the MIT License.
 
 import { Event } from 'vscode';
-import { StopWatch } from '../common/utils/stopWatch';
-import { sendTelemetryEvent } from '../telemetry';
-import { EventName } from '../telemetry/constants';
-import { getEnvPath } from './base/info/env';
 import {
     GetRefreshEnvironmentsOptions,
     IDiscoveryAPI,
     ProgressNotificationEvent,
     PythonLocatorQuery,
+    TriggerRefreshOptions,
 } from './base/locator';
 
 export type GetLocatorFunc = () => Promise<IDiscoveryAPI>;
@@ -52,20 +49,8 @@ class PythonEnvironments implements IDiscoveryAPI {
         return this.locator.resolveEnv(env);
     }
 
-    public async triggerRefresh(query?: PythonLocatorQuery, trigger?: 'auto' | 'ui') {
-        const stopWatch = new StopWatch();
-        await this.locator.triggerRefresh(query);
-        if (!query) {
-            // Intent is to capture time taken for all of discovery to complete, so make sure
-            // all interpreters are queried for.
-            sendTelemetryEvent(EventName.PYTHON_INTERPRETER_DISCOVERY, stopWatch.elapsedTime, {
-                interpreters: this.getEnvs().length,
-                environmentsWithoutPython: this.getEnvs().filter(
-                    (e) => getEnvPath(e.executable.filename, e.location).pathType === 'envFolderPath',
-                ).length,
-                trigger: trigger ?? 'auto',
-            });
-        }
+    public async triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions) {
+        return this.locator.triggerRefresh(query, options);
     }
 }
 

extensions/positron-python/src/client/pythonEnvironments/base/locator.ts

@@ -193,6 +193,17 @@ export interface GetRefreshEnvironmentsOptions {
     stage?: ProgressReportStage;
 }
 
+export type TriggerRefreshOptions = {
+    /**
+     * Trigger a fresh refresh.
+     */
+    clearCache?: boolean;
+    /**
+     * Only trigger a refresh if it hasn't already been triggered for this session.
+     */
+    ifNotTriggerredAlready?: boolean;
+};
+
 export interface IDiscoveryAPI {
     /**
      * Tracks discovery progress for current list of known environments, i.e when it starts, finishes or any other relevant
@@ -212,7 +223,7 @@ export interface IDiscoveryAPI {
     /**
      * Triggers a new refresh for query if there isn't any already running.
      */
-    triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }, trigger?: 'auto' | 'ui'): Promise<void>;
+    triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise<void>;
     /**
      * Get current list of known environments.
      */

extensions/positron-python/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts

@@ -4,9 +4,13 @@
 import { Event, EventEmitter } from 'vscode';
 import '../../../../common/extensions';
 import { createDeferred, Deferred } from '../../../../common/utils/async';
+import { StopWatch } from '../../../../common/utils/stopWatch';
 import { traceError } from '../../../../logging';
+import { sendTelemetryEvent } from '../../../../telemetry';
+import { EventName } from '../../../../telemetry/constants';
 import { normalizePath } from '../../../common/externalDependencies';
 import { PythonEnvInfo } from '../../info';
+import { getEnvPath } from '../../info/env';
 import {
     GetRefreshEnvironmentsOptions,
     IDiscoveryAPI,
@@ -15,6 +19,7 @@ import {
     ProgressNotificationEvent,
     ProgressReportStage,
     PythonLocatorQuery,
+    TriggerRefreshOptions,
 } from '../../locator';
 import { getQueryFilter } from '../../locatorUtils';
 import { PythonEnvCollectionChangedEvent, PythonEnvsWatcher } from '../../watcher';
@@ -33,6 +38,9 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
     /** Keeps track of promises which resolves when a stage has been reached */
     private progressPromises = new Map<ProgressReportStage, Deferred<void>>();
 
+    /** Keeps track of whether a refresh has been triggered for various queries. */
+    private wasRefreshTriggeredForQuery = new Map<PythonLocatorQuery | undefined, boolean>();
+
     private readonly progress = new EventEmitter<ProgressNotificationEvent>();
 
     public get onProgress(): Event<ProgressNotificationEvent> {
@@ -89,24 +97,25 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
 
     public getEnvs(query?: PythonLocatorQuery): PythonEnvInfo[] {
         const cachedEnvs = this.cache.getAllEnvs();
-        if (cachedEnvs.length === 0 && this.refreshesPerQuery.size === 0) {
-            // We expect a refresh to already be triggered when activating discovery component.
-            traceError('No python is installed or a refresh has not already been triggered');
-            this.triggerRefresh().ignoreErrors();
-        }
         return query ? cachedEnvs.filter(getQueryFilter(query)) : cachedEnvs;
     }
 
-    public triggerRefresh(query?: PythonLocatorQuery & { clearCache?: boolean }): Promise<void> {
+    public triggerRefresh(query?: PythonLocatorQuery, options?: TriggerRefreshOptions): Promise<void> {
+        const stopWatch = new StopWatch();
+        if (options?.ifNotTriggerredAlready) {
+            if (this.wasRefreshTriggered(query)) {
+                return Promise.resolve(); // Refresh was already triggered, return.
+            }
+        }
         let refreshPromise = this.getRefreshPromiseForQuery(query);
         if (!refreshPromise) {
-            refreshPromise = this.startRefresh(query);
+            refreshPromise = this.startRefresh(query, options);
         }
-        return refreshPromise;
+        return refreshPromise.then(() => this.sendTelemetry(query, stopWatch));
     }
 
-    private startRefresh(query: (PythonLocatorQuery & { clearCache?: boolean }) | undefined): Promise<void> {
-        if (query?.clearCache) {
+    private startRefresh(query: PythonLocatorQuery | undefined, options?: TriggerRefreshOptions): Promise<void> {
+        if (options?.clearCache) {
             this.cache.clearCache();
         }
         this.createProgressStates(query);
@@ -182,6 +191,10 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
         return this.refreshesPerQuery.get(query)?.promise ?? this.refreshesPerQuery.get(undefined)?.promise;
     }
 
+    private wasRefreshTriggered(query?: PythonLocatorQuery) {
+        return this.wasRefreshTriggeredForQuery.get(query) ?? this.wasRefreshTriggeredForQuery.get(undefined);
+    }
+
     /**
      * Ensure we trigger a fresh refresh for the query after the current refresh (if any) is done.
      */
@@ -203,6 +216,7 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
 
     private createProgressStates(query: PythonLocatorQuery | undefined) {
         this.refreshesPerQuery.set(query, createDeferred<void>());
+        this.wasRefreshTriggeredForQuery.set(query, true);
         Object.values(ProgressReportStage).forEach((stage) => {
             this.progressPromises.set(stage, createDeferred<void>());
         });
@@ -230,4 +244,16 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
             this.progress.fire({ stage: ProgressReportStage.discoveryFinished });
         }
     }
+
+    private sendTelemetry(query: PythonLocatorQuery | undefined, stopWatch: StopWatch) {
+        if (!query && !this.wasRefreshTriggered(query)) {
+            // Intent is to capture time taken for discovery of all envs to complete the first time.
+            sendTelemetryEvent(EventName.PYTHON_INTERPRETER_DISCOVERY, stopWatch.elapsedTime, {
+                interpreters: this.cache.getAllEnvs().length,
+                environmentsWithoutPython: this.cache
+                    .getAllEnvs()
+                    .filter((e) => getEnvPath(e.executable.filename, e.location).pathType === 'envFolderPath').length,
+            });
+        }
+    }
 }

extensions/positron-python/src/client/pythonEnvironments/index.ts

@@ -12,7 +12,10 @@ import { PythonEnvsReducer } from './base/locators/composite/envsReducer';
 import { PythonEnvsResolver } from './base/locators/composite/envsResolver';
 import { WindowsPathEnvVarLocator } from './base/locators/lowLevel/windowsKnownPathsLocator';
 import { WorkspaceVirtualEnvironmentLocator } from './base/locators/lowLevel/workspaceVirtualEnvLocator';
-import { initializeExternalDependencies as initializeLegacyExternalDependencies } from './common/externalDependencies';
+import {
+    initializeExternalDependencies as initializeLegacyExternalDependencies,
+    normCasePath,
+} from './common/externalDependencies';
 import { ExtensionLocators, WatchRootsArgs, WorkspaceLocators } from './base/locators/wrappers';
 import { CustomVirtualEnvironmentLocator } from './base/locators/lowLevel/customVirtualEnvLocator';
 import { CondaEnvironmentLocator } from './base/locators/lowLevel/condaLocator';
@@ -52,20 +55,44 @@ export async function initialize(ext: ExtensionState): Promise<IDiscoveryAPI> {
 /**
  * Make use of the component (e.g. register with VS Code).
  */
-export async function activate(api: IDiscoveryAPI, _ext: ExtensionState): Promise<ActivationResult> {
+export async function activate(api: IDiscoveryAPI, ext: ExtensionState): Promise<ActivationResult> {
     /**
      * Force an initial background refresh of the environments.
      *
-     * Note API is ready to be queried only after a refresh has been triggered, and extension activation is blocked on API. So,
-     * * If discovery was never triggered, we need to block extension activation on the refresh trigger.
-     * * If discovery was already triggered, it maybe the case that this is a new workspace for which it hasn't been triggered yet.
-     * So always trigger discovery as part of extension activation for now.
-     *
-     * TODO: https://github.com/microsoft/vscode-python/issues/17498
-     * Once `onInterpretersChanged` event is exposed via API, we can probably expect extensions to rely on that and
-     * discovery can be triggered after activation, especially in the second case.
+     * Note API is ready to be queried only after a refresh has been triggered, and extension activation is
+     * blocked on API being ready. So if discovery was never triggered for a scope, we need to block
+     * extension activation on the "refresh trigger".
      */
-    api.triggerRefresh().ignoreErrors();
+    const folders = vscode.workspace.workspaceFolders;
+    const wasTriggered = getGlobalStorage<boolean>(ext.context, 'PYTHON_WAS_DISCOVERY_TRIGGERED', false);
+    if (!wasTriggered.get()) {
+        api.triggerRefresh().ignoreErrors();
+        wasTriggered.set(true).then(() => {
+            folders?.forEach(async (folder) => {
+                const wasTriggeredForFolder = getGlobalStorage<boolean>(
+                    ext.context,
+                    `PYTHON_WAS_DISCOVERY_TRIGGERED_${normCasePath(folder.uri.fsPath)}`,
+                    false,
+                );
+                await wasTriggeredForFolder.set(true);
+            });
+        });
+    } else {
+        // Figure out which workspace folders need to be activated.
+        folders?.forEach(async (folder) => {
+            const wasTriggeredForFolder = getGlobalStorage<boolean>(
+                ext.context,
+                `PYTHON_WAS_DISCOVERY_TRIGGERED_${normCasePath(folder.uri.fsPath)}`,
+                false,
+            );
+            if (!wasTriggeredForFolder.get()) {
+                api.triggerRefresh({
+                    searchLocations: { roots: [folder.uri], doNotIncludeNonRooted: true },
+                }).ignoreErrors();
+                await wasTriggeredForFolder.set(true);
+            }
+        });
+    }
 
     return {
         fullyReady: Promise.resolve(),