Maybe use initializationOptions as additional source of settings #195

krobelus · 2022-04-10T12:11:53Z

This came up in this issue with the kak-lsp client.
Before the workaround for that issue, kak-lsp sent server configuration via the initialize request (in initializationOptions).
It only sent workspace/didChangeConfiguration when the configuration actually changed.
This caused problems because pylsp ignores initializationOptions. The workaround is to re-send options with workspace/didChangeConfiguration.

I wonder if it's a good idea to use initializationOptions as additional settings source.
I see two possible variants:

Always include initializationOptions but let workspace/didChangeConfiguration override its settings
As soon as workspace/didChangeConfiguration arrives, forget the initializationOptions

Annoyingly, option 2 would not have fixed the kak-lsp issue, because kak-lsp used to an empty
settings object in workspace/didChangeConfiguration (but that's an issue on our side).

It's probably also fine to leave it; LSP is quite underspecified about how these requests interact. I heard that both are deprecated in favor of workspace/configuration..

The text was updated successfully, but these errors were encountered:

rchl · 2022-04-11T11:02:37Z

workspace/didChangeConfiguration is a notification sent from the client to the server while workspace/configuration is request sent from the server to the client. Different use cases and none is deprecated.

initializationOptions' shape is server-defined and might or might not be used for sending the options if server wishes so but since it can only be sent once (on initialization) it is not the best use case for settings that can change at runtime. It's good for options that need to be known before server initializes though.

It doesn't really make sense for this server to read its settings from initializationOptions unless a given option really needs to be known that early.

It sounds like you are suggesting to add extra code and complicate the project because of particular client's limitation/deficiencies (not supporting workspace/configuration and/or workspace/didChangeConfiguration).

krobelus · 2022-04-11T11:28:11Z

It doesn't really make sense for this server to read its settings from initializationOptions unless a given option really needs to be known that early.

Makes sense but IMO an LSP client should not need to know whether an option is required early during initialization.
So the only generic solution I see is to send configuration with both requests.
Anyway, the real problem is that LSP is really vague, so clients need to support different interpretations.

rchl · 2022-04-11T11:36:12Z

I don't think that LSP is vague here. The protocol defines initializationOptions which can have any shape and it's up to the server to define that and it also defines a "Settings" object which is an object unrelated to initializationOptions. Particular server chooses what options it expects from either of those.

And a generic client implementation can never know what settings a particular server expects. It should be always up to the server-specific configuration defined separately from the generic client implementation to define which options to pass where. It's wrong for a generic client implementation to assume that both of those accept the same set of options.

krobelus · 2022-04-11T12:03:16Z

I don't think that LSP is vague here.

OK, not vague but "requires knowledge of servers to implement rcorrectly".

So there are three different configuration options initializationOptions, workspace/didChangeConfiguration and workspace/configuration.
The most flexible client would allow the user to configure each option independently.
However, that'd be pretty bad UI. Of course, VSCode exposes a single config file and I guess the server-specific extensions take care of forwarding the right options.
We've tried to mimic the structure of that config file. This allowed us to use the same settings object for workspace/didChangeConfiguration and workspace/configuration (modulo the server-specific "section name"), which might pose an issue too.
Anyway, apparently we need another bit of information to control initializationOptions.
Thanks for clarifying.

rchl · 2022-04-11T12:22:46Z

So there are three different configuration options initializationOptions, workspace/didChangeConfiguration and workspace/configuration.
The most flexible client would allow the user to configure each option independently.

The workspace/didChangeConfiguration and workspace/configuration are supposed to work on the same underlying configuration options so there shouldn't be a way for a client to specify those separately.

siegel · 2023-07-12T18:55:12Z

At the risk of reopening old wounds :-), it seems typical for language servers to send workspace/configuration to affirmatively request any configuration data from the client, irrespective of whether any was included in initializationOptions. (I'm not here to relitigate the question of whether configuration in initializeOptions is appropriate; I don't think that's a relevant question here.)

At the moment, I've got a customer who can't configure pylsp the way they need to, because it evidently never sends a workspace/configuration request to the client. My code never sends configurationDidChange because there's no need to: configuration is done using external files, and these are read only once. No change, therefore no change notification. (And it feels really weird to send a change notification when there hasn't been a change; as far as I am aware, this requirement is atypical to pylsp.)

So if it's practical to send a workspace/configuration request to the client it seems like that would address what appears to be a recurring reported issue, and would probably improve behavior and compatibiltiy with editors that aren't VS Code or vim. :-)

ccordoba12 · 2023-07-12T19:27:53Z

@siegel, thanks for your feedback. If this requires only sending workspace/configuration and not much else, then I'm ok with it. Just open a PR and I'll review it.

siegel · 2023-07-12T19:38:19Z

@ccordoba12 Thanks! I appreciate your willingness to adopt a change. I regret that the work is outside of my expertise; I am a consumer of the server, but know nothing about its internals (nor, alas, would I feel comfortable trying to learn the internals in any reasonable amount of time).

But if anyone else feels comfortable making the necessary change, then I am delighted to test it when it gets merged and released.

ccordoba12 · 2023-07-13T00:31:08Z

Ok, I have no problem with that, but since this doesn't directly benefit us (i.e. it's a feature we won't use in Spyder), it'll go to the backlog of things I have to do. So, this could take a long time to be solved.

However, you said:

I've got a customer who can't configure pylsp the way they need to

So, since this is for a paid job, I'd say that if you're willing to make a good donation to our Open Collective, then I'd take a look at it sooner (two or three weeks). If you're interested, please send me an email (it's in my profile) to discuss the details.

siegel · 2023-07-13T00:52:09Z

So, since this is for a paid job

I'm not getting paid for this. :-) However, the customer has expressed an interest in working on this themselves, so they may be submitting a PR at some point.

kstrauser · 2023-07-15T16:58:17Z

Hi @ccordoba12! I’m the customer in question. For clarity, my end goal is to use:

BBEdit
…with pylsp
…and python-lsp-ruff
and a baseline configuration that I can use in all the code I’m working on, even when that code doesn’t have a pyproject.toml.

I’d be more than happy to take a crack at this. After digging through the code, though, I’m not entirely sure where would be the right place to start. My hypothesis (feel free to laugh!) would be that it would look something like:

pylsp initializes itself
pylsp sends workspace/configuration to BBEdit
BBEdit responds with the config I put in it
Happiness!

Alternatively, BBEdit already supports workspace-specific configuration. That seems like an ideal and broadly useful feature for pylsp.

siegel · 2023-07-15T19:52:02Z

Alternatively, BBEdit already supports workspace-specific configuration. That seems like an ideal and broadly useful feature for pylsp.

That's a very client-specific feature; I don't know if it makes sense for the server to implement it. But others may know better than I. :-)

rchl · 2023-07-19T09:12:42Z

Alternatively, BBEdit already supports workspace-specific configuration. That seems like an ideal and broadly useful feature for pylsp.

That's a very client-specific feature; I don't know if it makes sense for the server to implement it. But others may know better than I. :-)

Think you misunderstood that. Settings specified in this custom BBEdit configuration will just be communicated to the server using standard LSP requests ("Initialization Options" through intialize request and "Workspace-Specific Configuration" through workspace/configuration request). There is nothing extra for the server to implement to support those as long as it's supporting those basic, standard LSP features).

Not sure as I haven't checked the code but I think this whole issue boils down to pylsp not triggering workspace/configuration itself and instead expecting workspace/didChangeConfiguration to be sent from the client. So the way to fix that would be to trigger workspace/configuration.

ccordoba12 · 2023-07-19T15:53:37Z

Not sure as I haven't checked the code but I think this whole issue boils down to pylsp not triggering workspace/configuration itself and instead expecting workspace/didChangeConfiguration to be sent from the client. So the way to fix that would be to trigger workspace/configuration.

Yep, that's exacly what I thought too. If there are clients (like BBEdit) that don't send workspace/didChangeConfiguration until the server informs them it's ready, then they won't be able to configure it.

ccordoba12 · 2023-07-19T16:03:17Z

@kstrauser, you said:

I’d be more than happy to take a crack at this. After digging through the code, though, I’m not entirely sure where would be the right place to start.

As I said before, I'd be willing to give you a hand if you'd like to make a donation to Spyder (I'm the lead Spyder maintainer and also maintain this project because it's the Python server we use in Spyder).

If not, you can study our code and the LSP protocol and try to make a PR for it. For us this is quite low priority because we don't use this functionality in Spyder. So, I could take a look at it in a year or so (I have tons of other things to do, sorry).

My hypothesis (feel free to laugh!) would be that it would look something like:

pylsp initializes itself

pylsp sends workspace/configuration to BBEdit

BBEdit responds with the config I put in it
Happiness!

You're quite right! That's exactly how this should work.

kstrauser · 2023-07-19T16:22:51Z

First, thanks for everyone involved having taken the time out of your days to discuss this. I appreciate it!

@ccordoba12 I'm willing to have that discussion. Full disclosure: this is something I'd like for my personal use, because it's been a recurring pain point with other editors, too. Today we're specifically talking about me wanting to configure BBEdit, but I'd run into the same problem with Emacs, Nova, and other editors. (What can I say? I'm fickle and editors are my Pokemon -- I've gotta catch them all!) It's easy enough to configure plugins like flake8 and pycodestyle, but python-lsp-ruff had me banging my head against the wall. I'd love to be able to configure a ball of JSON in my editor, toss it to pylsp, and have everything just work.

To that end, I'll put my money where my mouth is. Again, this is for my personal use. My budget -- and my spouse with whom I share a checking account -- won't allow me to send you on a nice vacation. If you don't think this would take much work, and lunch or a pizza for the office or something like that would make it worth your effort, let's make this happen!

And if not, no hard feelings. I appreciate the awesome project you've worked hard on! In fact, let me know if there's a way I can send you some coffee/beer/sparkling-water money.

ccordoba12 · 2023-07-19T23:10:52Z

@ccordoba12 I'm willing to have that discussion. Full disclosure: this is something I'd like for my personal use, because it's been a recurring pain point with other editors, too.

Great! My email is in my profile, so just send me one and we'll start talking about it.

My budget -- and my spouse with whom I share a checking account -- won't allow me to send you on a nice vacation

Don't worry, I'm not expecting that.

If you don't think this would take much work, and lunch or a pizza for the office or something like that would make it worth your effort, let's make this happen!

I think this shouldn't be that difficult, so a relatively small donation should be enough.

I appreciate the awesome project you've worked hard on!

Thanks for your kind words!

In fact, let me know if there's a way I can send you some coffee/beer/sparkling-water money.

I don't have an account for something like that, but you could still donate to our Open Collective.

kstrauser · 2023-07-22T04:48:58Z

Done, and done!

kstrauser · 2023-10-10T20:55:08Z

I haven't heard back about my offer of paid work, so I presume this isn't still an available option.

tkrabel-db · 2023-10-13T11:11:33Z

Re-posting my comment here since this is the better room to discuss this.

I feel the discussion around initializationOptions is unfortunate. There is no consensus around how initializationOptionsshould be used (microsoft/language-server-protocol#567), but many developers find it very useful to configure the server on initialize. I think so too.

Language servers need to be configurable dynamically, especially when they run on a separate VM. workspace/configure is a valid general approach to handle server configuration at runtime, but using that to enable/disable some features is overkill, wasteful and will lead to bad user behaviour.

It's wasteful because you will end up turning off features that might have been enabled already on initialize, consuming resources unnecessarily (think e.g. rope_autoimport, which needs to index python modules; that problem becomes worse when the language server lives in an ephemeral environment like a cloud VM)
It will lead to bad user behaviour since the client will already send messages and potentially receive responses from features that the used didn't even want to use in the first place (e.g. getting ruff diagnostics instead of flake8 ones).
It is overkill because it will require extra logic to make sure the server is responsive while requesting workspace/configuration and while applying its response (something that would be a no-brainer with initializeOptions as the server hasn't send a response to that yet, so the client keeps waiting)

krassowski · 2023-10-13T11:24:59Z

Unfortunately there is no indexing workflow for LSP (microsoft/language-server-protocol#54).

Since we are free to define initializationOptions it could equally well be:

initializationOptions: {
   settings: typeof DidChangeConfigurationParams.settings,
   / * place reserved for other initialization-specific settings to be added in the future */
}

A similar pattern is used by some other servers but naming varies, e.g. LLVM users ConfigSettings field. Then, other servers do not scope it at all 🤷

I think that this should be fixed at the spec level because otherwise we are back to n-m problem which LSP was supposed to solve. I would be much more inclined to approve a PR if there was a blessing from the LSP spec team on going one way or the other.

tkrabel-db · 2023-10-13T20:44:44Z

@krassowski I am curious: is there a protocol around how to name features? I'm wondering if we follow some spec that defines that every feature should always have an enabled field.
I'm asking because if each language server names it's features and their configuration settings independently, then there is a tight coupling between client <> server pairs regarding feature activation anyway.

Also, while I do see the n-m problem, I don't think it's thad bad in our case. workspace/didChangeConfiguration is already the officially supported way of changing configs, so every client that follows that will be able to configure the server (modulo the fact that they need to know python-lsp-servers naming convention!). Supporting setting the same configs using initializeOptions is then just an extra way to allow pushing a config to the server at startup.

kstrauser · 2023-10-14T04:13:26Z

8000

@tkrabel-db With #459, I can now configure pylsp from within BBEdit. I can't tell you how happy this makes me. Thank you!

rchl · 2023-10-14T11:21:08Z

Unfortunately there is no indexing workflow for LSP (microsoft/language-server-protocol#54).

Since we are free to define initializationOptions it could equally well be:
initializationOptions: {
   settings: typeof DidChangeConfigurationParams.settings,
   / * place reserved for other initialization-specific settings to be added in the future */
}
A similar pattern is used by some other servers but naming varies, e.g. LLVM users ConfigSettings field. Then, other servers do not scope it at all 🤷

Having a child "settings" object within the "initializationOptions" feels a bit redundant give how the meaning of "settings" and "options" is more or less the same.

I think that this should be fixed at the spec level because otherwise we are back to n-m problem which LSP was supposed to solve. I would be much more inclined to approve a PR if there was a blessing from the LSP spec team on going one way or the other.

I'm not sure how could the spec fix it. The options/settings are always server specific and the client has to know them before server is initialized.

The best I can think of is having a common way of declaring settings schema and bundling that with the server but that still would have to be somewhat separate from the server itself.

kstrauser · 2023-10-18T19:20:31Z

@rchl The PR #459 solves all the problems I had using pylsp with my editor. If it doesn't cause any new issues, and improves compatibility, would be it OK to merge it as-is?

ccordoba12 · 2023-10-19T01:46:50Z

Given that adding this will help to solve concrete use cases (i.e. what @tkrabel-db and @kstrauser want to achieve) and it doesn't represent a significant change for the way editors/IDEs that support this server configure it, I'm +1 on including support for it in our next version.

It's unfortunate that the LSP spec is unclear about how initializationOptions should work, but once that's better defined, we could adjust things here accordingly.

kstrauser · 2023-11-11T16:30:04Z

A huge thank you to everyone who helped with this!

krobelus closed this as completed Apr 11, 2022

clarfonthey mentioned this issue Dec 4, 2022

pylsp never gets configured kakoune-lsp/kakoune-lsp#655

Closed

ccordoba12 reopened this Jul 13, 2023

krassowski mentioned this issue Oct 12, 2023

Support initializationOptions to configure the server #459

Merged

ccordoba12 added this to the v1.9.0 milestone Oct 19, 2023

ccordoba12 added the enhancement New feature or request label Oct 19, 2023

ccordoba12 closed this as completed in #459 Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maybe use initializationOptions as additional source of settings #195

Maybe use initializationOptions as additional source of settings #195

Maybe use initializationOptions as additional source of settings #195

Maybe use initializationOptions as additional source of settings #195

Comments