-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add python.interpreters.include
and python.interpreters.exclude
settings
#6398
base: main
Are you sure you want to change the base?
Conversation
This option allows user-specified Python search paths to be included as part of Python installation discovery. This is useful when the Python interpreter is not installed in a standard location. The option is a list of unique paths to include in the search for Python interpreters. The paths can be absolute or relative to the user's home directory. The default value is an empty list.
…uite working yet
E2E Tests 🚀 |
this is needed because the config api is now used to check for included/excluded runtimes as part of registering runtimes.
Opening this up for review, but I am working on adding a JS locator test for the new QUESTION: For the native locator, shall I just leave a note in |
* Gets the list of interpreters that the user has explicitly included in the settings. Converts | ||
* relative and aliased paths to absolute paths. | ||
* @returns List of interpreters that the user has explicitly included in the settings. |
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.
Where are we converting relative and aliases paths? It looks like we're just ignoring them.
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.
oops good catch! I meant to resolve aliased paths and not include relative paths. I just pushed a commit to resolve aliased paths.
For relative paths, my thinking is that it's ambiguous where the paths are relative to. Are they relative to the home directory or to the project directory? Does it make sense for an admin to set a relative path as a default? I figured we can document that only absolute or absolute-but-aliased paths are valid here.
|
||
/** | ||
* Check whether an interpreter should be included in the list of discovered interpreters. | ||
* If an interpreter is both explicitly included and excluded, it will be included. |
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.
Are you confident this is the right approach? I'm wondering if we'd run into a situation where a Workbench admin has explicitly excluded an interpreter, but then a confused user tries to include it. We might want to consider implementing a way to log this use for admins, but we don't have monitoring implemented yet in Workbench for VS Code or Positron.
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 language pack output will have a bit of info about why certain interpreters were included/excluded, but longer term, I'm hoping to better surface this information in a dedicated pane or something similar.
I think this approach makes sense for the Desktop user case, but let me think on the admin situation a bit! I feel like we will want a separate flag/option elsewhere, like an "admin policy" to enforce certain options
added a comment to the native python finder to add a test in the future
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.
Code changes LGTM! I tested locally and it seems to work well, except that the interpreters are still shown in the "Python: Select Interpreter" command. Is that expected?
extensions/positron-python/src/client/positron/interpreterSettings.ts
Outdated
Show resolved
Hide resolved
|
||
// --- Start Positron --- | ||
/** | ||
* Wraps the original filter function to include additional filtering logic. | ||
* If no filter is provided, the returned function will only include the additional filtering logic. | ||
* @param filter The original filter function | ||
* @returns A new filter function that includes the original filter function and the additional filtering logic | ||
*/ | ||
function filterWrapper(filter: ((i: PythonEnvironment) => boolean) | undefined) { | ||
return (i: PythonEnvironment) => (filter ? filter(i) : true) && shouldIncludeInterpreter(i.path); | ||
} | ||
// --- End Positron --- |
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 interpreters are still shown in the "Python: Select Interpreter" command. Is that expected?
Thank you for catching this! I checked the "include" case, but missed the "exclude" case for the quickpick. This should be fixed now!
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.
Question: If I have started an interpreter, say, a pyenv interpreter, then I exclude ~/.pyenv
, should I expect to see the interpreter on restart?
It will not show if I have shut down the interpreter, but if I leave it running and then quit/reopen Positron, the interpreter persists. This feels like the right choice, but just making sure it is intentional!
++ to @seeM comment, this does not affect the Python: Select Interpreter
list. With the upcoming changes to our interpreter, we might want to make sure include/exclude works there as well
...ons/positron-python/src/client/pythonEnvironments/base/locators/common/nativePythonFinder.ts
Outdated
Show resolved
Hide resolved
@@ -17,6 +17,7 @@ import * as workspaceApis from '../../client/common/vscodeApis/workspaceApis'; | |||
|
|||
// --- Start Positron --- | |||
// re-enable when Native Finder is used for discovery | |||
// TODO: add test for python.interpreters.include here once we switch to Native Finder |
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.
this is fine for now, I'll log an issue to turn these back on (probably for the next milestone)
oh gosh, I reviewed just a few mins too late! I see the |
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.
good with me once ci is green ✅
Yes this feels right to me as well 👍 Thanks for calling this out! I added a note to the QA Notes section to mention this case |
### Release Notes <!-- Optionally, replace `N/A` with text to be included in the next release notes. The `N/A` bullets are ignored. If you refer to one or more Positron issues, these issues are used to collect information about the feature or bugfix, such as the relevant language pack as determined by Github labels of type `lang: `. The note will automatically be tagged with the language. These notes are typically filled by the Positron team. If you are an external contributor, you may ignore this section. --> #### New Features - N/A #### Bug Fixes - #3740 filters out unsupported (<3.8) Python versions from the `Python: Select Interpreter` dropdown ### QA Notes 1. create a Python interpreter <3.8 (if you are a pyenv user, you can do `pyenv install 3.6`) 2. go to `Python: Select Interpreter` 3. should not appear in dropdown
Summary
/opt/python
to discovery paths #6254 for the JS locator (previously only implemented for the native locator)python.interpreters.include
: List of folders to search for Python installations. These folders are searched in addition to the default folders for your operating system.python.interpreters.exclude
: List of interpreter paths or folders to exclude from the available Python installations. These interpreters will not be displayed in the Positron UI.Settings UI
Settings JSON
Python Language Pack Output
Some log prefixes/messages to search for:
pythonRuntimeDiscoverer
: to find out if any runtimes were filtered out during discoveryshouldIncludeInterpreter
: whether or not the interpreter will be included based on the user settingsNot registering runtime ${extraData.pythonPath} as it is excluded via user settings.
: if the runtime is not getting registered because the user excluded itgetAdditionalEnvDirs
: native python finder -- list all the additional directories being searched, such as the user-included dirsRelease Notes
New Features
Bug Fixes
QA Notes
python.locator
.python.interpreters.include
/python.interpreters.exclude
settings so that discovery can re-run with the settings applied/1/2/3/4/5/3.10.4/bin/python
, they should use/1/2/3/4/5
as the included directory.