Add progress reporting

[syphar]

this could be the start for implementing #201.

I would like to get some design feedback here before I invest too much time into the wrong direction, and to see if there is even maintainer capacity for reviewing / discussions (which is OK if the case).

I imagine two possible designs:

implementing it in this server core

Which is what you see in the PR right now. We would show progress for any hook-call and report intermediate progress messages for every plugin. Downside would be that plugins couldn't add more details to progress reporting. Upside is that we immediately have progress for all plugins, including third party plugins.

I'm not sure which is the preferred way to hook into the method calls of each plugin without touching the plugin source. Provide our own _multicall? Wrap the HookImpl.function somehow?

Reporting on the hook-level already works with the code below, but we don't see which plugin is called at the moment.

implementing it on the plugin / hook level

each plugin / hook could get a simple context manager for progress reporting and use it where useful. This would mean less noise where we currently would report progress for fast things where we probably rarely need it.

I could also imagine a combination? Where basic progress reporting could be done on the hook/plugin level and more messages can be added in the hook-methods ?

Anyway, I would love some first feedback and design feedback and then I could continue working on this.

Fixes #201.

[syphar]

I never worked with pluggy so perhaps I'm missing another approach that might make this easier?

[bagel897]

https://github.com/bageljrkhanofemus/python-lsp-server/blob/task_handle/pylsp/plugins/_rope_task_handle.py
This is kinda where I was trying to go, it'll be pretty easy to implement if we have it put in the workspace. I can try your patch against rope-autoimport and see how it goes.

[syphar]

https://github.com/bageljrkhanofemus/python-lsp-server/blob/task_handle/pylsp/plugins/_rope_task_handle.py
This is kinda where I was trying to go, it'll be pretty easy to implement if we have it put in the workspace. I can try your patch against rope-autoimport and see how it goes.

In the end that's the main design question. I believe one "process" with begin & end for each hook is a good start.

For all messages in between there could be magic that generates messages for each hook implementor (= each plugin), or just the option for the plugins do report more messages, or a combination of these.

Or, no progress by default and every plugin has full control, for example via a simple context manager (begin/end/intermediate progress messages)

[syphar]

I'll also start using this myself.

Currently I actually lean towards explicit definition for each plugin

[bagel897]

I've been trying the implementation with this PR and I like it reporting progress for everything, its quite nice. I'm still trying to get autoimport working though.

[syphar]

I added an alternative approach that would give the progress power to plugins, including more detailed progress messages and percentages.

[syphar]

as an example from the ecosystem:

null-ls reports progress from the core, showing a percentage / plugin-names when calling plugins ( = sources)

https://github.com/jose-elias-alvarez/null-ls.nvim/blob/main/lua/null-ls/generators.lua

[bagel897]

I have a kind of working implementation with task handle with the older approach here. This is pretty cool and useful, I'll probably fully implement it when this PR is merged.

[syphar]

I have a kind of working implementation with task handle with the older approach here. This is pretty cool and useful, I'll probably fully implement it when this PR is merged.

This looks awesome!

How you used it actually feels like the explicit approach would be the better approach, so every plugin has to opt-in into progress and then has all the power.

[bagel897]

Alternatively, that could be in each plugins config to use the provided one (say pylint). Some (autoimport) would roll their own and some (jedi) wouldn't need it.

[syphar]

@ccordoba12 I would love to push this forward and I'm happy to invest more time into this.

What's your take on which direction we should take here?

[ccordoba12]

I'm not developing this, so I really don't know. As far as I understand, autoimport is independent of this feature, but it'd be nice if both land on the same release.

[bagel897]

Now that autoimport is merged, do we want to work on this PR? We can do the autoimport task handle here or in a different PR, I've already written the code for it. As for it being enabled for all the hooks by default, I think it should be default off, with an option to turn it on. We can work on long running plugins like autoimport, flake8 and pylint first

[syphar]

@bagel897 I'm happy to finish this up as needed.

I removed the general reporting of all hooks, does the rest of the design roughly match what you would like to have?

This means plugins would have to add reporting themselves, which is OK I guess.

[bagel897]

It looks cool! Thanks and I'll PR the autoimport hook after it gets merged.

[ccordoba12]

Hey @syphar, thanks for your work on this, it looks really interesting! (and sorry for my confusing message above, I thought you were a user asking for the state of this PR).

I think the most important thing missing from you work is tests. Please try to add some tests that check that progress reporting is working as expected.

[syphar]

I think the most important thing missing from you work is tests. Please try to add some tests that check that progress reporting is working as expected.

@ccordoba12 do you have an example where we already have tests that test for messages sent via self._endpoint.notify...)? Otherwise this would be only unit-tests

[ccordoba12]

do you have an example where we already have tests that test for messages sent via self._endpoint.notify...)?

I this this kind of tests is what you're looking for:

https://github.com/python-lsp/python-lsp-jsonrpc/blob/develop/test/test_endpoint.py

[syphar]

@ccordoba12 I added tests for the workspace logic (progress reporting itself).

My idea was to add the reporting to any call that might take longer, so anything that is linting, formatting, but also things like rename, references, defintions.

I wouldn't add progress to highlight, folding or completions.

Agreed?

Should we also add tests if the specific plugins actually report progress?

[syphar]

I just added progress reporting where I believe it would be useful. What do you think?

[syphar]

@ccordoba12 so, what do you think? :)

[ccordoba12]

Thanks for the new tests @syphar! I left two small suggestions for you, then this should be ready.

Also, let me know if you need help solving the linting issues reported by Github actions.

[syphar]

@ccordoba12 thank you for the review :)

I applied your suggestions, I also fixed the static linting errors that I would reproduce locally.

[ccordoba12]

@syphar, my bad, it seems you're using the consumer fixture at least in one test.

[syphar]

also my bad, I could have run the tests after changing them :)

fixed now.

[ccordoba12]

Looks good to me now, thanks for your contribution @syphar!

[syphar]

Thanks for merging! I'm happy to have helped.

Also this was 90% scratching my own itch since mypy linting takes so long in a project I'm working in :)

I'm excited for the release, will then create a PR to https://github.com/python-lsp/pylsp-mypy too

[ccordoba12]

I'm excited for the release, will then create a PR to https://github.com/python-lsp/pylsp-mypy too

That's pretty cool! Thanks @syphar!