Add new command to kick off (refresh) interpreter discovery

[juliasilge]

Addresses #4269

This PR adds a new command workbench.action.language.runtime.discoverAllRuntimes to kick off runtime discovery. I just have it accessible from the command palette (no keybindings, no UI), which is a good option for now, I think. I put this in with other commands in languageRuntimeActions.ts, although I do want to highlight that all the other commands in there apply to a single interpreter, not to all of them as a whole.

This command can only add new interpreters. If the user removes an interpreter while Positron is running, this new command does not get rid of it because the runtime is still registered; discovery can only add runtimes. We could totally clear all the registered runtimes but that seems pretty destructive, and removing runtimes hasn't been a big concern so far. Thoughts?

Release Notes

New Features

• Adds new command "Interpreter: Discover All Interpreters" to kick off interpreter discovery, for example if you have just made a new Python environment that Positron doesn't know about yet

Bug Fixes

• N/A

QA Notes

I think the easiest way to see if this is doing what we expect is to watch the output channel for either the R or Python Language Pack and execute the command. You'll see all the logging for interpreter discovery go by each time you do so. Alternatively, you could:

• Look at all your interpreter options in Positron
• Add a new one via the regular terminal (for example, a new rig version)
• Look at all your interpreter choices and see that your new one is not there yet
• Execute the command
• Look at all your interpreter choices and see that now the new one is there

[github-actions]

E2E Tests 🚀
This PR will run tests tagged with: @:critical

^{readme  valid tags}

[juliasilge]

We could use wording here that is more like "Refresh Interpreter Discovery", if we think that is better or more clear?

[jmcphers]

I prefer the wording in the PR!

[jmcphers]

I prefer the wording in the PR!

[jmcphers]

This needs a couple of changes if it is going to be public:

• we need to validate that the startup phase we're currently in is compatible with a transition into Discovering to make sure this can't accidentally get called when a discovery is already in flight, and/or get accidentally invoked before we get to the first discovery phase
• the contents of _discoveryCompleteByExtHostId need to be correct going into this method (i.e. false for all ext hosts since we are about to do discovery) so we should set or enforce that -- otherwise we'll never enter (or will prematurely enter) the complete state

[juliasilge]

I moved the original one back to be private and made a new public rediscoverAllRuntimes() that will only run when we have gotten all the way to Complete.

[jmcphers]

LGTM. A nice future improvement would be for this to show a progress dialog and summarize its work when it's done ("discovering" .... "found 2 new interpreters: interpreter a, interpreter b") since right now it can feel like the command didn't do anything.

[juliasilge]

I was thinking the same thing ("this doesn't seem like it's doing anything") so I added a notification in d00af25. This is still just for adding new runtimes.

I was playing around with this more, and I am realizing it doesn't work very well (at all???) for Python, but I think it's something in the Python extension going wrong. For example, if I:

• Add a new Python version via pyenv while Positron is open
• Run "Python: Select Interpreter" and note that the new pyenv Python is not there, as expected (discovery hasn't run again yet)
• Click the little "refresh" button on the "Python: Select Interpreter" quickpick
• I still do not see the new pyenv, which I do not expect!

If I reload the whole app, then I do see the new pyenv Python show up.

@isabelizimm does this ring a bell for you? I know you were looking at some of this a while back. What might be causing the "Refresh interpreter list" button not to be working? And then subsequently the Positron-specific discovery not to be working?

[isabelizimm]

Was this using the native or js locator? I am seeing the new version show up for both locators when you click the refresh button; the new Python ends up in its own section below Unsupported.

It's possible to have a refresh query only for certain Python paths, although functionally I'm not quite sure how you'd get into that situation on accident.

[juliasilge]

Ah OK, I do see the new interpreters at the end of the list of results when hitting refresh on "Python: Select Interpreter"; they are there but not with the other pyenv interpreters.

I think what we can say is that right now, this PR works for R:

But it does not work for Python. Looks like when we call the discoverAllRuntimes() method on the runtime manager(s) in the extension host(s), it is not kicking off discovery for Python.

[juliasilge]

I think something might be going on around here:

positron/src/vs/workbench/services/runtimeStartup/common/runtimeStartup.ts

Lines 788 to 791 in 0ff52df

	// Ask each extension to provide its language runtime metadata.
	for (const manager of this._runtimeManagers) {
	manager.discoverAllRuntimes(disabledLanguages);
	}

These are actually the runtime managers per extension host, not per extension. How things are turning out for me right now has both R and Python in one extension host.

IIUC that means this is being run one time per extension host (not per extension):

positron/src/vs/workbench/api/browser/positron/mainThreadLanguageRuntime.ts

Lines 1417 to 1419 in 0ff52df

	async discoverAllRuntimes(disabledLanguageIds: string[]): Promise<void> {
	this._proxy.$discoverLanguageRuntimes(disabledLanguageIds);
	}

[juliasilge]

This is what we needed for this to work for Python! 🎉

However, it does change what the Python discoverer does for all uses. Do we think this is a good way to go? The alternative is that discoverAllRuntimes() on the runtime startup service could gain an optional boolean argument that specifies whether this is a refresh or an original discovery; we would then pipe it through the API command through to here:

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

Line 124 in 93ea551

return this.discoverPythonRuntimes();

[isabelizimm]

Refreshing every time could slow down discovery if users open up multiple windows, since it runs on extension activation. However, finding interpreters each new window might be a nice experience (and actually make it less necessary to use a refresh command). I think its fine to do every time.