Adopt new Python extensions non-blocking API for interpreter disocvery

[karrtikr]

Three discovery APIs are being added for Jupyter which is meant to replace existing getInterpreters() API: microsoft/vscode-python#17452

type PythonApiForJupyterExtension = {
     ...
    /**
     * IInterpreterService
     */
    readonly refreshPromise: Promise<void> | undefined;
    /**
     * IInterpreterService
     */
    readonly onDidChangeInterpreters: Event<PythonEnvironmentsChangedEvent>;
    /**
     * Equivalent to getInterpreters() in IInterpreterService
     */
    getKnownInterpreters(resource?: Uri): PythonEnvironment[];
    /**
     * Equivalent to getAllInterpreters() in IInterpreterService
     */
    getInterpreters(resource?: Uri): Promise<PythonEnvironment[]>;
    ...

Previous behavior:

Existing getInterpreters() is an asynchronus blocking API, so it waits until all of discovery is completed the first time it is called. Subsequent calls return instantaneously from cache if the same resource is queried again.

New behavior:

• New getKnownInterpreters() simply returns whatever is currently in cache. It is useful when we do not need all of the interpreters at once, which is usually the case. In the Python extension we are able to replace all instances of getInterpreters() with getKnownInterpreters(). Do note that the kind info returned by this API may change as refresh progresses.
• onDidChangeInterpreters fires when an environment gets added/removed/updated in cache. Event properties tracks the old and new environments accordingly.
• Python extension triggers a refresh in background when the extension loads for the first time, which is when refreshPromise is set. It can be used to implement now deprecated getInterpreters() in the following way:

    /** Gets all interpreters **/
    public async getInterpreters(resource?: Uri): Promise<PythonEnvironment[]> {
        await this.refreshPromise; // Wait for the refresh to finish so that the cache contains all environments.
        return getKnownInterpreters(resource); // Return from cache.
    }

• getInterpreterDetails() is still available as before, and can be used to get complete and reliable information on environments. As it is a blocking call, it's recommend to only use it if complete information is needed, which is generally only the case for selected interpreters.

You can the see the new API in action here in our dynamic quickpick microsoft/vscode-python#17043 (comment).

Note: Any internal interpreter list caching that was recently added should go away once onDidChangeInterpreters events are available.

Changes regarding quickpick API

We also used to expose getSuggestions API for the quickpick, which has the same problem as getInterpreters(). The following non-blocking API is being added to replace it:

getKnownSuggestions(resource: Resource): Promise<IInterpreterQuickPickItem[]>;

It can be used in combination of refreshPromise to get all suggestions. Let me know if you have any questions! 😄

[rchiodo]

@karrtikr just want to double check, this new API won't break existing users, right? If a user is on python insiders, but not updating the jupyter extension, they won't be broken because the old extension is still using the old API?

[karrtikr]

Yes, the old APIs are still available, the new APIs are added in addition.

[karrtikr]

FYI we plan to expose discovery API generally for all extensions and not just Jupyter, and it's a little different from this API. Proposed design: https://gist.github.com/karthiknadig/4b7bc9a9420e9071105007b5cc096f65

I'll leave it up-to you guys on whether you want to wait for the final general API. This issue can be closed in that case.

[DonJayamanne]

@karrtikr Was looking at this API today, some questions:

• old & new
  • When we get the first event old = undefined, and new = A
  • Next lets assume somethign changes, then old = A and new = B
  • How do I know whether the old value from the second event === new value from first event.
    • Are the object refs the same, (looking at the code, they are not)
    • Hence its impossible for me to update the objects at my end, i.e. i cannot update the interpreter information as i don't have the original object (all i'm getting from python extension is some new object that says its the original, but its a whole new refernce).
    • I.e.there's no identity

Here's the code that shows the objets are re-created everytime for every event

            this.changed.fire({
                type: event.type,
                new: event.new ? convertEnvInfo(event.new) : undefined,
                old: event.old ? convertEnvInfo(event.old) : undefined,
                resource: event.searchLocation,
            });

Internally things are ok for Python extension as the original object refs are stored and updated in place

            } else if (seen[event.index] !== undefined) {
                const oldEnv = seen[event.index];
                seen[event.index] = event.update;
                didUpdate.fire({ index: event.index, old: oldEnv, update: event.update });

Solution

• We should pass the same objects around instead of creating them again & again
• I think python extension can easily maintain a weakmap of these ojbects (key = python object & mapped object is the one sent to external users) or other ways...(e.g. regular map, but more expensive)

[karrtikr]

How do I know whether the old value from the second event === new value from first event.

You could compare the .path property directly. I think the other properties in old won't be used anyways, so maybe we should only send the path as an id instead of the previous environment (denoted by old).

Internally things are ok for Python extension as the original object refs are stored and updated in place

We maybe doing this but we do not rely on this information and do path check directly, see here: https://github.com/microsoft/vscode-python/blob/ec628195910d7b67fba12ece521d60b75a81fb0f/src/client/pythonEnvironments/base/info/env.ts#L245-L247

[DonJayamanne]

You could compare the .path

That wouldn't work as some conda environments. If you create conda environments on unix, the python path is the same as the base conda env.( Unless you provide the python argument in the clip).

[karrtikr]

Interesting, we're not handling such environments at the moment in the discovery component and completely rely on 'path', but weirdly haven't received any issue reports.

cc @karthiknadig

[DonJayamanne]

Closing this as completed, the new API has been completely adopted.

[karrtikr]

Can we consider microsoft/vscode-python#19989 as unblocked from Jupyter's end, if there's no feedback/concerns?

[DonJayamanne]

Can we leave the Python issue opeen at least for one iteration, in case we run into any issues, this was we have a chance to test the API in production.
In other words, I think we should consider leaving it open untill the API is used in production environment (by Jupyter) before considering it to be stable enough.

Note: That doesn't mean that there are any issues, but would prefer to leave the Python issue open for now.

Next month if we dont get back, then please feel free to close it.

[karrtikr]

Makes sense, not a problem.