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

Changed Server Capabilities are not preserved upon VenvSelect #58

Closed
chrisgrieser opened this issue Aug 29, 2023 · 29 comments
Closed

Changed Server Capabilities are not preserved upon VenvSelect #58

chrisgrieser opened this issue Aug 29, 2023 · 29 comments
Labels
enhancement New feature or request

Comments

@chrisgrieser
Copy link

Thanks for this nice plugin, just stumbled upon it by chance!

So I am using pyright and pylsp at the same time, and I use this snippet to disable pyright's hover capabilities, since I prefer pylsp's:

require("lspconfig").pyright.setup {
  on_attach = function(client, _) client.server_capabilities.hoverProvider = false end
}

However, it seems that the way this plugin sets up the LSPs, only the settings are carried over, but not the disabled server capabilities, meaning that as soon as I use VenvSelect, the hover capability is re-enabled again.

(I tried various versions of updating the lsp config via hooks or simply using local on_attach = vim.deepcopy(pyright.config.on_attach) to preserve the disabled hover, but could not make it work.)

@linux-cultist
Copy link
Owner

linux-cultist commented Aug 29, 2023

You're welcome. :)

Interesting scenario for sure. I added some more debug output just now that prints how the pyright config looks like when you select a virtual env.

It should look something like this (at the end of output) if you type :messages after selecting a virtual env:

VenvSelect: Pyright settings:
VenvSelect: 
{
  python = {
    analysis = {
      autoSearchPaths = true,
      diagnosticMode = "workspace",
      useLibraryCodeForTypes = true
    },
    pythonPath = "/home/cado/Code/sampleprogram/venv/bin/python"
  }
}

Can you print how your pyright lsp settings look like before VenvSelect overwrites them?

@chrisgrieser
Copy link
Author

Do I have to do something to get the output?

I tried :mess before and after using :VenvSelect, if both cases nothing gets added from this plugin.

I just run the snippet above without passing anything to settings, which is maybe why no settings are shown? As far as I can tell, server_capabilities are not part of lsp settings? 🤔

@linux-cultist
Copy link
Owner

Sorry, you need to add enable_debug_output = true to the plugin settings also. :)

I havent looked into the pyright LSP details either, I just did what was necessary to get the plugin to work when I first started writing it. But I was curious how your pyright settings look like after you run your snippet there, to maybe get some clues how it should look like to not overwrite the client.server_capabilities.hoverProvider setting.

Maybe its not the right way and I just need to read up more about the lsp and figure out how to do this. :)

@chrisgrieser
Copy link
Author

ah, got it. Okay, so here is the output then:

VenvSelect: 
{
  anaconda_base_path = "",
  anaconda_envs_path = "/Users/chrisgrieser/.conda/envs",
  auto_refresh = true,
  cache_dir = "/Users/chrisgrieser/.cache/venv-selector/",
  cache_file = "/Users/chrisgrieser/.cache/venv-selector/venvs.json",
  changed_venv_hooks = { <function 1>, <function 2>, <function 3> },
  dap_enabled = true,
  enable_debug_output = true,
  hatch_path = "~/Library/Application/Support/hatch/env/virtual",
  name = { ".venv" },
  notify_user_on_activate = false,
  parents = 0,
  pipenv_path = "~/.local/share/virtualenvs",
  poetry_path = "~/Library/Caches/pypoetry/virtualenvs",
  pyenv_path = "~/.pyenv/versions",
  search = true,
  search_venv_managers = true,
  search_workspace = true,
  venvwrapper_path = "~/.virtualenvs"
}
VenvSelect: Setting fd_binary_name to 'fd' since it was found on system.
VenvSelect: Telescope path: /Users/chrisgrieser/Repos/axelrod-prisoner-dilemma
VenvSelect: Looking for parent venvs in '/Users/chrisgrieser/Repos/axelrod-prisoner-dilemma' using the following parameters:
VenvSelect: 
{ "--absolute-path", "--color", "never", "-E", "/proc", "-HItd", "(^.venv$)", "/Users/chrisgrieser/Repos/axelrod-prisoner-dilemma" }
VenvSelect: No virtual env selected in telescope.
VenvSelect: Found venv in parent search: /Users/chrisgrieser/Repos/axelrod-prisoner-dilemma/.venv/
VenvSelect: Found workspace folder: /Users/chrisgrieser/Repos/axelrod-prisoner-dilemma
VenvSelect: Running search for workspace venvs with: fd -HItd --absolute-path --color never '(^.venv$)' /Users/chrisgrieser/Repos/axelrod-prisoner-dilemma 
VenvSelect: Found venv in Workspace search: /Users/chrisgrieser/Repos/axelrod-prisoner-dilemma/.venv/
VenvSelect: Found no venv manager directories to search for venvs.
VenvSelect: There are 1 results to show:

@linux-cultist
Copy link
Owner

I think you need to have the pyright LSP activated also for the important part to show up. So open a python file also, activate the venv with VenvSelect and then do the :messages command. :)

@chrisgrieser
Copy link
Author

chrisgrieser commented Sep 3, 2023

  • enable_debug_output = true
  • in a python file
  • pyright active
  • no venv enabled
  • then run :VenvSelect

gets me exactly the output I posted above

@cmetz
Copy link

cmetz commented Sep 3, 2023

yeah it's a "bug" in the plugin, i also have trouble there. the options @chrisgrieser is mentioning are not under settings instead they are config options like: .capabilities, .handlers, ... https://github.com/neovim/nvim-lspconfig/blob/a27356f1ef9c11e1f459cc96a3fcac5c265e72d6/doc/lspconfig.txt#L132

venv-select is only taking a deep copy of settings, but not the other configuration parameters, if that is even possible and then starting a new lsp server with it's own then reduced configuration.

i see there are only three/four solutions:

  1. maybe the on_attach can be modified by the hook without a new server setup and then call a server restart/reset if there is one
  2. copy other (or complete) configure options as well, not only settings, if that is possible
  3. allow the user to define their own config options in the lsp hook
  4. or the user just replaces the complete hook with there own one, this might already be possible

@linux-cultist
Copy link
Owner

linux-cultist commented Sep 3, 2023

Ok thank you @cmetz and @chrisgrieser , sounds like a bit more digging is required here.

venv-select is only taking a deep copy of settings, but not the other configuration parameters, if that is even possible and then starting a new lsp server with it's own then reduced configuration.

Yes this is correct. The plugin calls the pyright hook which makes a copy of settings here:

--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
M.execute_for_client("pyright", function(pyright)
local utils = require("venv-selector.utils")
local settings = vim.deepcopy(pyright.config.settings)
lspconfig.pyright.setup({
settings = settings,
before_init = function(_, c)
c.settings.python.pythonPath = venv_python
utils.dbg("Pyright settings:")
utils.dbg(c.settings)
end,
})
end)
end

There are instructions in the README how to write your own hook replacing this one, which can be a start for exploring how to add support for multiple LSP's, if you want to give it a shot. I havent ventured very deep into that particular jungle myself at this point. :)

@cmetz
Copy link

cmetz commented Sep 3, 2023

i replaced the hook with that code below and it works, but i don't know how that whole setup can handle different buffers with different venv. maybe there is a way to have multiple lspserver running for different workspaces, or just start new vim session.

--- @alias VenvChangedHook fun(venv_path: string, venv_python: string): nil
--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
  M.execute_for_client("pyright", function(pyright)
    local utils = require("venv-selector.utils")
    pyright.config.before_init = function(_, c)
      c.settings.python.pythonPath = venv_python
      utils.dbg("Pyright settings:")
      utils.dbg(c.settings)
    end
    vim.cmd(string.format("LspRestart %d", pyright.id))
  end)
end

maybe there is a better way to reload the server, but that seems not that easy
https://github.com/neovim/nvim-lspconfig/blob/a27356f1ef9c11e1f459cc96a3fcac5c265e72d6/plugin/lspconfig.lua#L89

this works also:

--- @alias VenvChangedHook fun(venv_path: string, venv_python: string): nil
--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
  M.execute_for_client("pyright", function(pyright)
    pyright.config.settings.python.pythonPath = venv_python
    vim.cmd(string.format("LspRestart %d", pyright.id))
  end)
end

i don't know why it was originally done by before_init.

@linux-cultist
Copy link
Owner

linux-cultist commented Sep 3, 2023

Using the before_init function is almost instant and avoids restarting the entire LSP server. I also think LspRestart restarts all languages LSP servers in the editor. Some servers are slow in the beginning before they have cached the project.

It would be nice to keep using the before_init to not restart other lsp servers. Do both solutions above work for your usecases?

@cmetz
Copy link

cmetz commented Sep 3, 2023

@linux-cultist LspRestart with an client id only restarts specific server. the current use of .setup(..) is also restarting the whole server only evaluating a before init handler to manipulate the config. this might be useful for setting some dynamic stuff e.g. when calling an restart and evaluate some dynamic code. but the code is setting only a fix venv value, so i'm pretty sure this can be done completely in the config.settings not using the init function.

or if you want to set some value depending on a config value like they did it here:

https://github.com/neovim/nvim-lspconfig/blob/a27356f1ef9c11e1f459cc96a3fcac5c265e72d6/lua/lspconfig/server_configurations/codeqlls.lua#L11

from the documentation:
https://neovim.io/doc/user/lsp.html

before_init: Callback with parameters (initialize_params, config) invoked before the LSP "initialize" phase, where params contains the parameters being sent to the server and config is the config that was passed to vim.lsp.start_client(). You can use this to modify parameters before they are sent.

@linux-cultist
Copy link
Owner

I remember reading this quickly when I started writing this plugin a long time ago. It was my first plugin and I was learning lua at the same time, so I was just happy to get something working. :)

@cmetz
Copy link

cmetz commented Sep 3, 2023

yeah you did really a great job. Lazvim uses it as default :)

but i just found out that for pyright this can even be done quicker. there is a command which allows you to change the pythonpath and then evokes an workspace update in the lsp server, so no restart of the server needed :)

--- @alias VenvChangedHook fun(venv_path: string, venv_python: string): nil
--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
  if vim.fn.exists(":PyrightSetPythonPath") > 0 then
    vim.cmd(string.format("PyrightSetPythonPath %s", venv_python))
  end
end

https://github.com/neovim/nvim-lspconfig/blob/a27356f1ef9c11e1f459cc96a3fcac5c265e72d6/lua/lspconfig/server_configurations/pyright.lua#L21C16-L21C31

maybe it's better to not rely on a user command and invoke that lsp command directly

--- @alias VenvChangedHook fun(venv_path: string, venv_python: string): nil
--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
  M.execute_for_client("pyright", function(pyright)
    pyright.config.settings =
      vim.tbl_deep_extend("force", pyright.config.settings, { python = { pythonPath = venv_python } })
    pyright.notify("workspace/didChangeConfiguration", { settings = nil })
  end)
end

Some more technical background notes for nerds:

@linux-cultist
Copy link
Owner

linux-cultist commented Sep 3, 2023

Thats super cool, I will try it out tomorrow right away. :) Thanks so much for helping out!

@linux-cultist
Copy link
Owner

I really like the last solution here:

--- @alias VenvChangedHook fun(venv_path: string, venv_python: string): nil
--- @type VenvChangedHook
function M.pyright_hook(_, venv_python)
  M.execute_for_client("pyright", function(pyright)
    pyright.config.settings =
      vim.tbl_deep_extend("force", pyright.config.settings, { python = { pythonPath = venv_python } })
    pyright.notify("workspace/didChangeConfiguration", { settings = nil })
  end)
end

Seems to work equally well for me, and this also solves the issue for both of you?

@linux-cultist linux-cultist added the enhancement New feature or request label Sep 5, 2023
@cmetz
Copy link

cmetz commented Sep 5, 2023

@linux-cultist yeah this solves the issue for me, but maybe it would make sense to iterate over all matching pyright clients if there are more than one.

meanwhile i switched to configuring my pythonPath through an neoconf config file, as it is installed with lazyvim by default and also respects some other configs options like that from vscode and others.

maybe i'll try to write a venvselect hook that will configuring the .neoconf.json file for a specific workspace.

this is an example .neoconf.json

{
  "lspconfig": {
    "pyright": {
      "python.pythonPath": "venv/bin/python"
    }
  }
}

i also configured my dap resolve_path via an function which resolves the pythonpath from the pyright config

  {
    "mfussenegger/nvim-dap-python",
    init = function()
      require("dap-python").resolve_python = function()
        return vim.lsp.get_active_clients({ name = "pyright" })[1].config.settings.python.pythonPath
      end
    end,
  },

i just switched to nvim again couple of days ago and it's quite interesting to see different packages solving things a different way. but none of them is having a venvselect :)

@chrisgrieser
Copy link
Author

chrisgrieser commented Sep 6, 2023

okay, I am not sure, I can follow everything in the discussion, but the last snippet here:

function M.pyright_hook(_, venv_python)
  M.execute_for_client("pyright", function(pyright)
    pyright.config.settings =
      vim.tbl_deep_extend("force", pyright.config.settings, { python = { pythonPath = venv_python } })
    pyright.notify("workspace/didChangeConfiguration", { settings = nil })
  end)
end

seems to work for me!

Haven't done thorough testing, but haven't noticed any issues with it either. Can be merged in the plugin, I think?

@linux-cultist
Copy link
Owner

Yeah I think it seems to work fine and it would make sense to replace the default hook with this one. Just wanted to make sure it doesnt do anything weird. Im not too read-up on the specifics on the LSP. @cmetz knows more than me for sure. :)

@linux-cultist
Copy link
Owner

i just switched to nvim again couple of days ago and it's quite interesting to see different packages solving things a different way. but none of them is having a venvselect :)

Im still on it but after years of maintaining my own config, I actually switched to Lazyvim due to always having some weird small bugs in my own configs. Its fun to have my own config but its also kind of hard to understand why something is buggy now and then.

@cmetz
Copy link

cmetz commented Sep 6, 2023

@linux-cultist i don't understand your commit it seems redundant for me handlig some lsp servers.

in the venv.lua (M.set_venv_and_system_paths) the pythonpath is set twice, once via an autocommand i don't understand why this is used.:

 M.set_pythonpath(venv_python)

and a second time via hook

  for _, hook in ipairs(config.settings.changed_venv_hooks) do
    hook(venv_path, venv_python)
  end

maybe the autocommand is used to override other plugins switching the pythonpath, but in my understanding this will create a new and add a new autocommand every time you switch the environment, not removing the old one.

@linux-cultist
Copy link
Owner

linux-cultist commented Sep 6, 2023

The original way (first thing you are showing above) is how VenvSelect was setting python interpreter before someone added hooks to the codebase.

But the hook doesn't modify the system path as I understand it, so it's still necessary to modify the path (putting the venv first in the path) for the venv to be activated in any terminal windows opened from inside neovim.

That was my original thinking anyway. Maybe it's time to have a look at that code and remove anything it's doing that is now redundant....

And the commit was just trying to improve the default hook that was in the code base, but maybe I misunderstood what we discussed above?

@linux-cultist
Copy link
Owner

I removed the changed code now, so we can understand better what needs to be done.

@cmetz
Copy link

cmetz commented Sep 6, 2023

@linux-cultist the hook is fine, but i think the code got a little bit redundant on different places. may we could do a life coding session at some point to test and debug the code a little bit. i'm not very deep into lua and nvim. just startet a few days ago using neovim again.

@linux-cultist
Copy link
Owner

Yeah i think it probably is. I cant really do a live coding session since I work full time and have family etc, so its hard to plan in advance when i can spend time on this, and also I try to be a bit anonymous... but you seem pretty good at understanding the LSP, specially for someone who just came back to neovim.. :)

@cmetz
Copy link

cmetz commented Sep 6, 2023

ah that is fine, I'm in a same situation, maybe i can invest some time and create a pull request.

@linux-cultist
Copy link
Owner

I would love if you do. I think you already know more than me about the LSP and having another person go through the code will be very helpful I think. Just be aware that some parts may look a bit out of place - there has been many people contributing (which I actually like, but can lead to one person not knowing the entire code base). :)

@mennohofste
Copy link
Contributor

I created a PR that essentially reimplements the commit shown above. The only difference is that it loops over all pyright clients instead of just changing the first one.

the hook is fine, but i think the code got a little bit redundant on different places.

As @cmetz said, the commit was good and solves the issue. The fact that a new autocommand is created every time you set a different python environment is a different issue. I think a new issue should be opened for that one.

The code change solves this issue, and does not make the other problem better or worse, they are not related. So I would argue to solve this bug first, and then to look at the duplication after.

@mennohofste
Copy link
Contributor

I can also make this issue if you want. 👍

@linux-cultist
Copy link
Owner

I love any support you can offer. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants