Skip to content
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

installed_check script fails in mulit-root workspace with virtual environment #22460

Closed
tboddyspargo opened this issue Nov 10, 2023 · 14 comments
Closed
Assignees
Labels
triage-needed Needs assignment to the proper sub-team

Comments

@tboddyspargo
Copy link

tboddyspargo commented Nov 10, 2023

Type: Bug

Behaviour

Expected vs. Actual

I expect no exceptions to be raised when provided wth a valid python interpreter path and requirements file path.

Instead, I see an error (see logs below)

Steps to reproduce:

  1. Create a multi-root workspace
  2. In a folder, set the python.defaultInterpreterPath to .venv/bin/python
  3. Create the venv with python -m venv .venv (with that folder as the working directory)
  4. Create a requirements.txt in the folder
  5. Open your Python output and you should show the error below

Repro multi-root workspace:
workspace_root.zip

Diagnostic data

  • Python version (& distribution if applicable, e.g. Anaconda): 3.9.11
  • Type of virtual environment used (e.g. conda, venv, virtualenv, etc.): Pyenv
  • Value of the python.languageServer setting: Pylance
Output for Python in the Output panel (ViewOutput, change the drop-down the upper-right of the Output panel to Python)

2023-11-10 17:11:22.654 [info] Running installed packages checker:  .venv/bin/python /Users/username/.vscode/extensions/ms-python.python-2023.20.0/pythonFiles/installed_check.py /tmp/workspace_root/one/requirements.txt
2023-11-10 17:11:22.655 [info] > .venv/bin/python ~/.vscode/extensions/ms-python.python-2023.20.0/pythonFiles/installed_check.py /tmp/workspace_root/one/requirements.txt
2023-11-10 17:11:22.656 [error] Error while getting installed packages check result:
 Error: spawn .venv/bin/python ENOENT
    at Process.ChildProcess._handle.onexit (node:internal/child_process:283:19)
    at onErrorNT (node:internal/child_process:476:16)
    at processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn .venv/bin/python',
  path: '.venv/bin/python',
  spawnargs: [
    '/Users/username/.vscode/extensions/ms-python.python-2023.20.0/pythonFiles/installed_check.py',
    '/tmp/workspace_root/one/requirements.txt'
  ]
}

User Settings

Multiroot scenario, following user settings may not apply:

envFile: "<placeholder>"

languageServer: "Pylance"

testing
• pytestArgs: "<placeholder>"

Extension version: 2023.20.0
VS Code version: Code 1.84.2 (Universal) (1a5daa3a0231a0fbba4f14db7ec463cf99d7768e, 2023-11-09T10:52:33.687Z)
OS version: Darwin arm64 23.0.0
Modes:

System Info
Item Value
CPUs Apple M1 Max (10 x 24)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: enabled
Load (avg) 6, 5, 5
Memory (System) 32.00GB (0.05GB free)
Process Argv --crash-reporter-id 9c8b74e5-efe2-4756-b68a-91b9fde0fa03
Screen Reader no
VM 0%
A/B Experiments
vsliv368:30146709
vsreu685:30147344
python383:30185418
vspor879:30202332
vspor708:30202333
vspor363:30204092
vslsvsres303:30308271
vserr242:30382549
pythontb:30283811
vsjup518:30340749
pythonptprofiler:30281270
vshan820:30294714
vstes263:30335439
vscoreces:30445986
vscod805:30301674
binariesv615:30325510
bridge0708:30335490
bridge0723:30353136
vsaa593cf:30376535
pythonvs932:30410667
py29gd2263cf:30880073
vsclangdf:30486550
c4g48928:30535728
dsvsc012cf:30540253
pynewext54:30695312
azure-dev_surveyone:30548225
2e4cg342:30602488
f6dab269:30613381
a9j8j154:30646983
showlangstatbar:30737416
pythonfmttext:30731395
fixshowwlkth:30771522
showindicator:30805244
pythongtdpath:30769146
i26e3531:30792625
welcomedialog:30882109
pythonnosmt12:30797651
pythonidxpt:30866567
pythonnoceb:30805159
synctok:30869157
dsvsc013:30795093
dsvsc014:30804076
dsvsc015:30845448
pythontestfixtcf:30871695
pythonregdiag2:30871582
pyreplss2:30886141
pythonmypyd1:30879173
pythoncet0:30885854
h48ei257:30885898
pythontbext0:30879054
dsvsc016cf:30886111
dsvsc017cf:30886113
dsvsc018:30886114
aa_t_chat:30882232

@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Nov 10, 2023
@karthiknadig
Copy link
Member

@karrtikr it looks like we are getting the wrong python path here?

@github-actions github-actions bot added the info-needed Issue requires more information from poster label Nov 10, 2023
@karrtikr
Copy link

karrtikr commented Nov 11, 2023

It does not look like the path to interpreter is valid. Either use

'./.venv' or use ${workspaceFolder}/.venv notation to configure relative interpreter paths, the latter is preferred especially in a multiroot scenario.

@tboddyspargo
Copy link
Author

Thanks for your help with this!

In my experience, .venv/bin/python has been valid for test discovery, test execution, test debugging, run file, debug file, pylint, and intellisense. I believe this is because they all assume a cdw of the “nearest” current workspace folder.

Does installed_check.py follow a different standard or have different requirements?

I’ve struggled a bit to understand what this script does and what it’s requirements/expectations are.

Would it be reasonable to have installed_check.py use the same contextual details as the other extension tools to resolve a relative interpreter path? I worry about different pieces of the extension having different requirements for that kind of thing.

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Nov 11, 2023
@tboddyspargo
Copy link
Author

I was reassured to see that I can copy the command that is run and execute it from the workspace folder (as the working directory) and it is successful:

one$ .venv/bin/python ~/.vscode/extensions/ms-python.python-2023.20.0/pythonFiles/installed_check.py /private/tmp/workspace_root/one/requirements.txt
[{"line": 0, "character": 0, "endLine": 0, "endCharacter": 6, "package": "pytest", "code": "not-installed", "severity": 3}]

My conclusion is that the only missing piece is ensuring that the spawned process is appropriately using the workspace folder as the working directory (similar to how the language server, test discover/execution, etc. do).

@karrtikr
Copy link

karrtikr commented Nov 11, 2023

Unfortunately due to technical reasons and simplicity of implementation, that is not possible for now, especially when there're reliable ways to correctly specify relative paths. The thing which makes it difficult it's up to the each tool how that is interpreted, which can be unpredictable (imagine a tool using Python extension which we do not control).

If there's an issue with supported functionality do let us know and we'll happy to have a look again, closing for now.

@karrtikr karrtikr closed this as not planned Won't fix, can't repro, duplicate, stale Nov 11, 2023
@github-actions github-actions bot added the info-needed Issue requires more information from poster label Nov 11, 2023
@tboddyspargo
Copy link
Author

tboddyspargo commented Nov 11, 2023

Thanks for your message @karrtikr. I have a different interpretation of this issue, the configuration options of the extension, and the documented expectations for defaultInterpreterPath. Respectfully, I ask you to reconsder.

The thing which makes it difficult it's up to the each tool how that is interpreted, which can be unpredictable (imagine a tool using Python extension which we do not control).

I don't see "multiple tools" at play here. I believe that getInstalledPackagesDiagnostics is using concepts and utilities that are purely internal to this extension and/or part of the public extensions API.

As a matter of fact, the vscode documentation (the extension's documentation?) provides an example (emphasis is mine) that's identical to the one described in this issue (the documentation does say "full path", but it's clear that does not mean "absolute path" in the context of the provided example):

You can then either enter the full path of the Python interpreter directly in the text box (for example, ".venv/Scripts/python.exe") or [...]
--https://code.visualstudio.com/docs/python/environments#_manually-specify-an-interpreter

I believe I was actually inspired by this example when I configured my workspace and my setup has been generally well supported by the extension (other than in the case of getInstalledPackagesDiagnostics).

Unfortunately due to technical reasons and simplicity of implementation

I'm sorry, but it's not clear to me what "technical reasons" would prevent two parts of the same extension (vscode-python) from being able to share common interpreter path resolution... I also don't see much complexity in the requested fix.

Other parts of the same codebase seem quite capable of doing resolving the interpreter path relative to the workspace folder:

this.defaultInterpreterPath = getAbsolutePath(this.defaultInterpreterPath, workspaceRoot);

return getAbsolutePath(this.pythonPath, workspaceRoot);

this.languageStatus.detail = this.pathUtils.getDisplayName(interpreter.path, workspaceFolder?.fsPath);

It seems to me that installedCheckScript could easily be updated to be consistent with the rest of the extension:

const interpreter = interpreterPathService.get(doc.uri);


My general concern is that as a user of vscode-python, I expect the extensions settings have fixed, consistent requirements throughout the extension - they should mean and express one (and only one) thing. I can certainly see why a fix would not be a high priority, given the availability of a workaround. However, I see no evidence that this is "not possible now."

It's clear that "python.defaultInterpreterPath": ".venv/bin/python" is resolved relative to the workspace folder by most areas of the extension, so I propose that aligning getInstalledPackagesDiagnostics with the majority of the rest of the extension is the best and easiest path to a consistent behavior.

Thanks anyway for your consideration. I hope you may reconsider a fix here to improve the consistency of handling defaultInterpreterPath throughout the extension.

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Nov 11, 2023
@karrtikr karrtikr reopened this Nov 11, 2023
@karrtikr
Copy link

Reopening as we should correct the docs if we don't support this scenario. Is there a reason why the other notations do not work for you?

@github-actions github-actions bot added the info-needed Issue requires more information from poster label Nov 11, 2023
@tboddyspargo
Copy link
Author

tboddyspargo commented Nov 12, 2023

Reopening as we should correct the docs if we don't support this scenario.

Thanks, @karrtikr! However, I think it would be more accurate to say that the extension does support this scenario in every area I've found exception this one. That's why I suspect fixing getInstalledPackagesDiagnostics is both the easiest path and probably most accurately reflects the intent of the extension as a whole.

Is there a reason why the other notations do not work for you?

I honestly wasn't worried about them when you first made the alternative suggestions, however I've tested them and they resulted in the same failures as my original syntax:

  • ./.venv/bin/python also does not work at the moment.
  • Additionally, though not surprisingly, it seems that ${workspaceFolder}/.venv/bin/python also does not work if the folder's path (in the.code-workspace) is defined as ..
  • Finally, and more surprisingly, ${workspaceFolder}/.venv/bin/python also does not work when the folder path (in the .code-workspace file) is defined as one.

NOTE: multiroot workspaces explicitly call out the value of and their support for using relative paths so that the same .code-workspace file may be shared between developers.

All of these results continue to suggest that in order to support multiroot workspaces like the rest of the extension, getInstalledPackagesDiagnostics will need to perform similar path resolution to the rest of the extension.

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Nov 12, 2023
@karrtikr
Copy link

karrtikr commented Nov 13, 2023

Thanks for getting back, let's look at each concern one at a time:

For multiroot workspaces, try using notation mentioned here #18650 (for eg. ${workspaceFolder:python-a}). Using relative paths or just ${workspaceFolder} leads to ambiguity as described here: #18650 (comment).

NOTE: multiroot workspaces explicitly call out the value of and their support for using relative paths so that the same .code-workspace file may be shared between developers.

"Relative paths for folder section" to be exact, settings section are not resolved that way.

I think it would be more accurate to say that the extension does support this scenario in every area I've found exception this one.

As I noted earlier, there could be more scenarios easily, we cannot force every tool to use workspace folder as their cwd, hence it's better to be explicit using a syntax such as ${workspaceFolder} instead when specifying interpreters.

I believe this is because they all assume a cdw of the “nearest” current workspace folder.

That is, this is not guaranteed, we allow users to customize their own cwd when debugging, testing etc.

@github-actions github-actions bot added the info-needed Issue requires more information from poster label Nov 13, 2023
@karrtikr
Copy link

If ${workspaceFolder}/.venv/bin/python does not work for you in single workspace folder scenario (when not using .code-workspace file), do let us know and we can dig further into it.

@tboddyspargo
Copy link
Author

For multiroot workspaces, try using notation mentioned here #18650 (for eg. ${workspaceFolder:python-a}).

I don't see how that will work for the folder defined as { "path": "." }. I'm getting an error with that:

2023-11-13 14:06:56.017 [warning] Failed to check if /private/tmp/workspace_root/${workspaceFolder:.}/.venv/bin/python is an executable [Error: ENOENT: no such file or directory, lstat '/private/tmp/workspace_root/${workspaceFolder:.}/.venv/bin/python'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'lstat',
  path: '/private/tmp/workspace_root/${workspaceFolder:.}/.venv/bin/python'
}

How could one refer to that specific folder (.) in defaultInterpreterPath?

I believe this is because they all assume a cdw of the “nearest” current workspace folder.

That is, this is not guaranteed, we allow users to customize their own cwd when debugging, testing etc.

Exposing a customizable cwd option is great, but so is a versatile default value to fall back on. I would advocate for both in most tools, but I certainly place a higher priority on having an equally helpful default handling as the other tools in this extension.

Using relative paths or just ${workspaceFolder} leads to ambiguity as described here: #18650 (comment).

I actually disagree with the conclusions drawn in that comment. I don't see any ambiguity in the given example. The config would simply point to a file relative to each folder and the designated file simply doesn't exist in python2. It seems both like perfectly valid config and the simplest way to express shared folder configuration. If every folder's setup is not quite interchangeable, then the ${workspaceFolder:specific_folder} syntax provides a helpful way to describe a base path that is specific/relative to a single workspace folder. But I find that using ${workspaceFolder} in .code-workspace settings is quite convenient and intuitive given their hierarchical nature - e.g. when I have a folder-relative path that I want to be used for every folder in a workspace, but possibly be overridden in some. This section of the variables reference documentation expresses it pretty well, too: Without the root folder name, the variable is scoped to the same folder where it is used.. I believe that is comparable to the behavior I'm trying to leverage with python.defaultInterpreterPath. I'd like it to be scoped to the folder where it's being used (and resolved relative to it) both in the scenario where ${workspaceFolder} is used and plain relative path is used.

As I noted earlier, there could be more scenarios easily, we cannot force every tool to use workspace folder as their cwd, hence it's better to be explicit using a syntax such as ${workspaceFolder} instead when specifying interpreters.

I've been a little confused about what you mean by "every tool"... I think you're referring to the different sub-components of the vscode-python package, but correct me if I'm misinterpreting that.

I don't really propose "forcing" anything (it implies perfect compliance, which isn't realistic), but I do advocate for something akin to "eventual consistency" - aligning as a team on a single shared solution to a common problem. It seems to me that sharing logic is probably beneficial for the team to avoid conflicting, diverging, or redundant code maintenance overhead.

Is there a shared and recommended "resolve the interpreter path" code pathway that all tools in the extension can/should be using given that they all share the defaultInterpreterPath setting? Would you be willing to try use a shared code path that resolves the path relative to the nearest workspace folder?

I would certainly be willing to use ${workspaceFolder} if it was correctly resolved in multi-root scenarios by getInstalledPackagesDiagnostics. It would provide a helpful convenience/versatility factor over the ${workspaceFolder:name} syntax that requires much more repetition.

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Nov 13, 2023
@karrtikr
Copy link

karrtikr commented Nov 13, 2023

I think you're referring to the different sub-components of the vscode-python package

Not necessarily, it can be different tool extensions.

I don't see how that will work for the folder defined as { "path": "." }. I'm getting an error with that:

That's actually a bug, should be solved with: #22452.

Reading settings using ${workspaceFolder} can technically be supported as you pointed out, but it's a general issue for all extensions whose support should ideally come from VS Code. At this point we can work on fixing support for ${workspaceFolder:name} as that is already required to be supported for other scenarios: #18650.

@github-actions github-actions bot added the info-needed Issue requires more information from poster label Nov 13, 2023
@karrtikr
Copy link

I've opened #22472 on your behalf to support resolving ${workspaceFolder} correctly where possible, feel free to add/clarify in comments if anything else is required.

@karrtikr karrtikr closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
@tboddyspargo
Copy link
Author

I think you're referring to the different sub-components of the vscode-python package

Not necessarily, it can be different tool extensions.

I see what you mean. I've definitely been focused on intra-extension interpreter path resolution consistency.

At this point we can work on fixing support for ${workspaceFolder:name}

That will be a helpful fix to have!

I've opened #22472 on your behalf to support resolving ${workspaceFolder} correctly where possible, feel free to add/clarify in comments if anything else is required.

Thank you very much, @karrtikr! I was feeling a little unsure of where to go next with my concern since the only solution path I feel confident enough about was within vscode-python. #22472 definitely summarizes my concerns well and if there's a common solution that will allow all parts of the vscode-python extension (including getInstalledPackagesDiagnostics ) to benefit from, that's wonderful!

@github-actions github-actions bot removed the info-needed Issue requires more information from poster label Nov 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
triage-needed Needs assignment to the proper sub-team
Projects
None yet
Development

No branches or pull requests

3 participants