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

import resolution for generic virtual environments #365

Closed
gennaro-tedesco opened this issue May 18, 2024 · 22 comments · Fixed by #1006
Closed

import resolution for generic virtual environments #365

gennaro-tedesco opened this issue May 18, 2024 · 22 comments · Fixed by #1006
Labels
config issues relating to config (pyproject.toml, pyrightconfig.json, LSP config or vscode extension) documentation Improvements or additions to documentation

Comments

@gennaro-tedesco
Copy link

First of all, thank you for the awesome project!

I am trying to come to terms with resolution of package imports within virtual environments (which I often found difficult in pyright as well): in particular I am referring to the import resolution section in the docs. Notice I have read #158 and #214 as well.

Description

I am using poetry to manage virtual environments: this means that virtual environments are generally created in some local paths ~/Library/pypoetry/virtualenvs/<projectname>-abc124... (or similar), with python packages locally installed are referred therein.

While running the project within the activated virtual environment, basedpyright always shows the diagnostic "Import could not be resolved" despite explicitly indicating the aforementioned path as per the documentation:

If a venv name is specified along with a python.venvPath setting (or a --venvpath command-line argument), it appends the venv name to the specified venv path...

The documentation moreover mentions

Use the python.pythonPath setting. This setting is defined by the VS Code Python extension and can be configured using the Python extension’s environment picker interface.

which is a little unclear to me if you are not using VS Code (the pythonPath is not a variable that even exists or is/must be explicitly set).

Attempts

[tool.basedpyright]
typeCheckingMode = "standard"
venvPath = "~/Library/pypoetry/virtualenvs/"  (or similar explicit path, or similar grammars)

What is the recommended way to specify paths to virtual environments for package resolution? (Notice I am referring to package imports and not relative imports from other modules, which work fine instead).

@KotlinIsland KotlinIsland added the config issues relating to config (pyproject.toml, pyrightconfig.json, LSP config or vscode extension) label May 18, 2024
@DetachHead
Copy link
Owner

in my experience virtualenvs just work by default in pyright, so i've left the venvPath setting and its documentation unchanged from upstream as i'm not really familiar with it.

could you give a bit more info about how exactly you're running basedpyright? are you installing it outside of your venv and if so, how come?

@gennaro-tedesco
Copy link
Author

Thank you for the prompt reply!

could you give a bit more info about how exactly you're running basedpyright? are you installing it outside of your venv and if so, how come?

I have installed it globally as python library via pip (or rather pipx), hence in that respect yes "outside" the virtual environment. Must it be installed within in (i. e. as a requirement package of a project within its environment)? I would find it a little strange (since then you'll have many basedpyright for each virtual environment rather than one only, which will be configured to "see" the different virtual environments per project).

@DetachHead
Copy link
Owner

Must it be installed within in (i. e. as a requirement package of a project within its environment)?

it's not a requirement, but it's what i've always done and i don't really understand why you wouldn't want to do that. making basedpyright a dev dependency of your project allows you to pin the version used by your IDE and your CI, which will prevent new errors or other changes in behavior from randomly appearing unexpectedly. this was my main motivation for creating basedpyright in the first place. i discuss the issue in more detail here.

as i said there, i recognize many people only use (based)pyright for its language server features and completely disable its type checking. i guess i could see why you'd want to just have it installed globally in that case? but i would highly recommend enabling the type checking and installing it to your venv instead.

all of that said, one of my goals is to maintain backwards compatibility with pyright so that users who don't agree with all of our decisions can simply disable the features they don't like. so this venvPath option should still work and i have no plans to remove it. i've just never personally used it before.

Attempts

[tool.basedpyright]
typeCheckingMode = "standard"
venvPath = "~/Library/pypoetry/virtualenvs/"  (or similar explicit path, or similar grammars)

What is the recommended way to specify paths to virtual environments for package resolution? (Notice I am referring to package imports and not relative imports from other modules, which work fine instead).

is the issue that poetry generates a random folder name for each of its venvs? if so, i think the solution here is to use an in-project venv, then set venvPath to ".venv"

i also want to take this moment to shill pdm as an alternative to poetry. in my experience it's far more stable and most importantly, unlike poetry, the maintainers didn't make the install script randomly fail in the CI 5% of the time. (even though this change was reverted before it was released, this shows that the maintainers are not trustworthy)

@gennaro-tedesco
Copy link
Author

it's not a requirement, but it's what i've always done and i don't really understand why you wouldn't want to do that.

because doing so runs into the problem indicated here, namely the text editors (neovim in this case, but it would be the same for others) look for the general executable into ~/.local/bin (or equivalent); virtual environments are disposable and generated on the fly, and there is no consistent way to specify their generalities to editor configurations (unless you have one configuration per virtual environment).

if so, i think the solution here is to use an in-project venv, then set venvPath to ".venv"

This unfortunately doesn't work and hasn't worked in pyright either unless one explicitly fully qualifies the precise path to the disposable virtual environment, which again defies a little the purpose of configuration per sé.

i also want to take this moment to shill pdm as an alternative to poetry.

Sure, but this issue isn't exclusive to poetry, the server fails to recognise virtual environments created by other dependency managers too: pipenv and pdm too.

Generally speaking my question can be re-formulated as follows: is it possible to have a minimally reproducible example (say in the docs or README) of how to have basedpyright working with general virtual environments? I know the docs address to the .venv solution of pyright, but that has (infamously) never worked, with the proposed solution being to explicitly specify the full name of the disposable environment all the times (which I believe all agree is unusable).

P. S. Please don't get me wrong, this project is great and I appreciate the immense effort in its development! The handling of virtual environments however has kept many away from pyright originally already (most other language servers do recognise the virtual environments automatically), I have been using others successfully so far but wanted to switch to basedpyright given the great improvements it's bringing - which is why I am still a little puzzled that virtual environments being such a basic and common python development feature, one still needs to go to great lengths to have the lsp to work with them "out of the box".

@DetachHead
Copy link
Owner

This unfortunately doesn't work and hasn't worked in pyright either unless one explicitly fully qualifies the precise path to the disposable virtual environment, which again defies a little the purpose of configuration per sé.

oh wow, that's really stupid. we should definitely fix that

@miversen33
Copy link

as i said there, i recognize many people only use (based)pyright for its language server features and completely disable its type checking. i guess i could see why you'd want to just have it installed globally in that case? but i would highly recommend enabling the type checking and installing it to your venv instead.

It's worth calling out that while you make good points here, most editors don't care. Neovim (mason specifically) for example downloads a single LSP and uses it for all matching filetypes. Vscode will do the same. While it is nice to have your LSP as part of your dev requirements, I would hesitate to consider it a requirement to get virtualenvs functional.

@DetachHead
Copy link
Owner

Neovim (mason specifically) for example downloads a single LSP and uses it for all matching filetypes. Vscode will do the same.

the basedpyright vscode extension defaults to using the version installed in the current project's venv, and will only fallback to using the version bundled with the extension if you don't have it installed already. i'm not sure how the extensions for other IDEs work since they were contributed by others and i personally only use vscode, but ideally they would all behave the same way.

While it is nice to have your LSP as part of your dev requirements, I would hesitate to consider it a requirement to get virtualenvs functional.

i agree. as i said in my previous comment, it seems really dumb that the venvPath needs to be absolute, which arguably makes the setting completely useless in its current state. i intend to fix this

@gennaro-tedesco
Copy link
Author

i'm not sure how the extensions for other IDEs work

my experience with terminal text editors is that they default to the global LSP executable because by definition their configurations are global, whereas IDEs tend to have mechanisms to "automatically activate" virtual environments within themselves (although even here I am not sure that this activation is automatic and must not be explicitly invoked by the user instead - I remember using pyright with PyCharm and somehow I could never make it work).

Generally speaking I would say that if one envisions for this project to be adopted in industrial settings (where many people contribute to the same code) then there ought to be an easi(er) way to indicate that the LSP must look for references and packages into a virtual environment first (if present) - the rationale being that if I use it myself only, then I could configure full paths and all the rest on my local machine, but if I work with my team on a shared github repository then we cannot just commit into the pyproject.toml each single independent configuration that points to our local paths.

@bluss
Copy link

bluss commented Oct 13, 2024

I find what has worked best for me in neovim is not the venv/venvPath configuration but to set pythonPath. Here's how you set it.

In lspconfig settings there is a callback called on_new_config which is called for every new workspace root. A simplified version of what you could do is like this:

          on_new_config = function(new_config, new_root_dir)
            local success, py_path = pcall(function()
              local pysupport = require("python_support")
              return pysupport.get_local_python_location(new_root_dir)
            end)
            if success and py_path then
              new_config.settings.python.pythonPath = py_path
            end
            if not success then
              vim.notify("Error: " .. tostring(py_path), vim.log.levels.WARN)
            end
          end,

I have a function there get_local_python_location that returns a string which points out the python binary in the current virtual environment. Like the absolute path to .venv/bin/python or similar. This works well (the venv/venvPath solution doesn't resolve standard library .py files as noted in issue #570, but that problem does not exist with pythonPath)

I don't really know what I'm doing, but maybe there could be a space somewhere in this repo where we can share some documentation bits about neovim. It's a bit tricky to find out exactly what the configuration items are called inside that settings namespace.

@DetachHead
Copy link
Owner

I don't really know what I'm doing, but maybe there could be a space somewhere in this repo where we can share some documentation bits about neovim. It's a bit tricky to find out exactly what the configuration items are called inside that settings namespace.

yeah it seems difficult to find information on how to configure neovim extensions in general, so i'm open to suggestions/contributions to improve the docs.

feel free to contribute some more information about neovim either in the setup instructions (if it's something you believe all users should do) or the language server settings examples (if it's something only some users may want).

the source for the docs is located here

@gennaro-tedesco
Copy link
Author

what you could do is like this:

local success, py_path = pcall(function()
              local pysupport = require("python_support")
              return pysupport.get_local_python_location(new_root_dir)
            end)

what are

require("python_support")
pysupport.get_local_python_location(new_root_dir)

in this context? Are they python in-built utilities in neovim or are they defined elsewhere?

@bluss
Copy link

bluss commented Oct 14, 2024

@gennaro-tedesco What I mean here is that it's something you have to fill in. I have one function there that only handles uv projects (call uv python find). For poetry it would do the equivalent of calling poetry env info -e or something like that. (Gets the python executable in the current project's python environment, which is what you need for the python.pythonPath setting).

I think it would help if I could share the code, but even so it doesn't support poetry projects..

@gennaro-tedesco
Copy link
Author

What I mean here is that it's something you have to fill in.

Sure, but having to manually fill in the paths (be they venvPath, pythonPath or else) defies the purpose of having a disposable virtual environment generated on the fly, doesn't it? That's the point I have tried to raise, namely that pyright in its origin doesn't solve this problem - whether then we want basedpyright to address it may or may not be in scope and is a decision of the maintainer, but users explicitly specifying paths in their configuration isn't the solution (in my opinion).

@bluss
Copy link

bluss commented Oct 14, 2024

I mean fill in the function that finds the right path, not fill in the path though. Just sharing my solution so far - which calls uv python find. I think there could exist an nvim plugin that already detects python per project, or if it doesn't exist, it could be created.

@DetachHead
Copy link
Owner

revisiting this issue again now that i'm more familiar with how these settings work.

This unfortunately doesn't work and hasn't worked in pyright either unless one explicitly fully qualifies the precise path to the disposable virtual environment, which again defies a little the purpose of configuration per sé.

oh wow, that's really stupid. we should definitely fix that

venvPath does in fact allow relative paths. i think my suggestion was just incorrect. you have to specify it like this:

[tool.basedpyright]
venvPath = "."
venv = ".venv"

@DetachHead DetachHead added the awaiting response waiting for more info from the author - will eventually close if they don't respond label Dec 17, 2024
@gennaro-tedesco
Copy link
Author

@DetachHead Thank you for checking up again on this issue: I will test your suggestion with different virtual environment and python package managers in the next days (as soon as I get some time)!

@gennaro-tedesco
Copy link
Author

I confirm that with these settings

[tool.basedpyright]
venvPath = "."
venv = ".venv"

the warnings disappear. I have moreover tested it with virtual environments in non-standard paths and as long as the concatenation of venvPath + venv resolves to the actual location, all works well!

This issue can be closed as far as I am concerned :).

If however I may make a suggestion, I would recommend to create a clear minimal working example in the documentation. The point seems to be - as we have concluded - that venvPath+venv must resolve to the physical location of the virtual environment with no need for pythonPath to enter the game at all. Most virtual environments by default being created in ./.venv by most dependency managers, the configuration indicated above should be the de facto with users not having to specify it at all (given that dependency management is one of the worst python drawbacks, by having basedpyright resolve this "out of the box" your project would jump high up in the standards of what python users must use as standard).

Maybe you could wait for other users to validate this too (in case they use other environments, hardware, versions and so forth).

@DetachHead
Copy link
Owner

yeah i agree that it's confusing. i still don't get why venv and venvPath are two separate settings instead of one.

i will leave this issue open for now until the documentation is updated. perhaps in this section

@DetachHead DetachHead added documentation Improvements or additions to documentation and removed awaiting response waiting for more info from the author - will eventually close if they don't respond labels Dec 17, 2024
cpprust added a commit to cpprust/basedpyright that referenced this issue Jan 17, 2025
fixes DetachHead#365
If project root is found, and ".venv" is found under project root,
use that venv as default.

Project root is found if "pyrightconfig.json" or "pyproject.toml" is
found. ".venv" can change to another name by change the
'[tool.pyright] venv="venv_name"' in "pyproject.toml".
But note that `uv` only support ".venv" so better stick with that.
@DetachHead
Copy link
Owner

thinking about this more, i'm considering just discouraging the venv and venvPath settings entirely. it's needlessly convoluted to have two separate options when it could be one, and it breaks "go to implementation" for standard library modules (#570). i think @bluss' solution to use python.pythonPath is the way to go here.

the pyright docs seem to already recommend against using these settings but i think the documentation can be more clear about that.

@gennaro-tedesco
Copy link
Author

i think @bluss' solution to use python.pythonPath is the way to go here.

What does this solution actually look like? From the comment on the thread the suggestion is for the user to find out by themselves (using uv find or any other framework-specific path finder isn't the right way), and the pyright docs point exclusively to VS Code settings (which by the way still don't work for generic virtual environments).

I am not against recommending pythonPath, but I would like to see a working example (I tried multiple times before even opening this issue and could never get it to work).

@DetachHead
Copy link
Owner

i think we should do what @cpprust did in #1001 except with pythonPath instead of venv/venvPath. if pythonPath is not specified, it will default to searching in the project root for a .venv directory. if that's present, it will find its python executable and use that as the pythonPath

@gennaro-tedesco
Copy link
Author

Yes, I agree, that seems a good solution indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config issues relating to config (pyproject.toml, pyrightconfig.json, LSP config or vscode extension) documentation Improvements or additions to documentation
Projects
None yet
5 participants