Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Improve user/library code distinction in searchPaths and PYTHONPATH #794

Closed
jakebailey opened this issue Mar 19, 2019 · 7 comments
Closed
Assignees
Milestone

Comments

@jakebailey
Copy link
Member

For #720.

We need to distinguish between user and library paths for searchPaths (in initializationOptions) and PYTHONPATH.

The general way to do this for now will be to simply check if a given path is rooted inside the workspace. If it is, then treat it as user code (run extra analysis), otherwise treat it as library code. The current behavior treats everything set by searchPaths to be user code.

Additionally, we'll also be getting PYTHONPATH from the environment, which we should treat the same way. This will help with #469, such that the language server can be started without searchPaths set and still be able to work with more esoteric setups. get_search_paths.py can be modified to distinguish items in sys.path that came from PYTHONPATH, or the language server can read the environment variable directly during the initialization.

@jakebailey jakebailey added this to the March 2019.2 milestone Mar 19, 2019
@jakebailey jakebailey self-assigned this Mar 19, 2019
@jakebailey
Copy link
Member Author

Better yet; we could drop searchPaths from the initialization options altogether. The only reason it was there in the first place was so we had directories to search at the very beginning. PYTHONPATH being set could be enough at the start, then after the configuration comes with extraPaths, we do the actual analysis of the user code with that tacked on.

@gramster gramster removed this from the March 2019.2 milestone Mar 26, 2019
@Jma353
Copy link

Jma353 commented May 2, 2019

Question that is relevant to this issue:

Is it in the cards to make the extraPaths (or searchPaths, since they seem to be used interchangeably) config something that can be changed on the fly? A.k.a. with LSP didChangeConfiguration notification?

From reading the code on master, and looking at issues like #507, it doesn't seem like this language server is capable of doing this yet. It would be nice to dynamically change what paths the language server considers for things such as module imports, without having to restart the language server.

If I am missing something + this type of functionality is available today, I'd be happy to be proven wrong :).

@jakebailey
Copy link
Member Author

jakebailey commented May 2, 2019

There's a number of moving parts here.

searchPaths is a list of strings we get in our initialization options, which are sent once at the beginning and never again. extraPaths is an extension configuration, and when the LS starts, the extension combines extraPaths with PYTHONPATH (which also involves reading .env files) and passes us everything as searchPaths, which we then split apart into what we consider to be "user" and "non-user" (trying to determine how to do that was the original purpose of this issue, though just looking at where a directory is relative to the root appears to be working fine).

We could adopt extraPaths ourselves and read it out, but we'd need to then also merge things from the environment and recreate the final list. This is doable, as my most recent update to the extension LS startup code passes the environment though with this as an intended end state. However, we set those search paths pretty early (during initialization) because we need to do a bunch of work before we're able to receive any more requests, like analyzing the builtins. This poses a problem for eliminating searchPaths and doing the work ourselves, as we can't get any requests, including a didChangeConfiguration, until we respond to the initialize request, so we won't have extraPaths until later.

We have some code in place to reload things when we detect that a package is installed, but I'm unsure if we'd be able to apply that if the search paths themselves are changing. I think it would be (PathResolver.SetInterpreterSearchPaths/SetUserSearchPaths), but I have not tested.

@MikhailArkhipov
Copy link

Merge into #1213?

@jakebailey
Copy link
Member Author

Maybe. I'll have to see how it goes; this was more focused when I thought PYTHONPATH would be "the thing" to use (versus our current stance of trying to make sure this happens in a config instead of the env var that Python itself uses). I'll close it if I think there's nothing to do here.

For the most part, my PR which added the path sorting logic "fixed" this by reclassifying things based on their location, but that logic could be improved as well with some extra configuration options.

@jakebailey
Copy link
Member Author

#1289 is merged, which moves the path stuff later in the process. This should allow us to do some of the above (see also #507).

@jakebailey
Copy link
Member Author

The classification of import paths was already done in #1289. Dropping searchPaths and doing things on the fly outside of initialization I've opened as #1388.

Closing, as the original purpose of this issue was handled in #1289.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants