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

R: completion not available for tidyverse packages #95

Closed
syu-id opened this issue Nov 5, 2019 · 9 comments
Closed

R: completion not available for tidyverse packages #95

syu-id opened this issue Nov 5, 2019 · 9 comments
Labels
bug Something isn't working

Comments

@syu-id
Copy link

syu-id commented Nov 5, 2019

tidyverse is a collection of some of the most popular packages in R which all play within the "tidy data" framework. library(tidyverse) reduces the effort to load various tidyverse packages by loading all the core packages at once: ggplot2, tidyr, dplyr, etc. It should be noted that tidyverse uses a rather sophisticated method to load the dependencies.

The problem: Currently it seems that completion for tidyverse packages is not available on JupyterLab if they are loaded with library(tidyverse) rather than loaded individually. For example, no completion is suggested for ggplot2's plotting functions such as geom_*.

However, developers of R's languageserver package have already addressed tidyverse's problem, and I can confirm that completion for tidyverse packages works nicely on Vim with the LanguageClient-neovim plugin. So the current issue might be specific to jupyterlab-lsp.

randy3k, languageserver's developer, suggests that the problem might have something to do with the following issues:

My environment:

  • jupyter-lsp: v0.6.0b0
  • jupyterlab-lsp: v0.6.1
  • languageserver (R): v0.3.2
@krassowski
Copy link
Member

krassowski commented Nov 5, 2019 via email

@syu-id
Copy link
Author

syu-id commented Nov 5, 2019

Thank you for the reply!
Unfortunately, completion for tidyverse packages didn't work in either notebook or file editor.
Support for tidyverse would be great because these packages are virtually indispensable for many R users.

@krassowski
Copy link
Member

krassowski commented Nov 5, 2019

@RongMu I have just tried with the latest (not yet released) version of R languageserver and with the latest (not yet released) version of jupyter-lsp and jupyterlab-lsp and it worked for me:

Screenshot from 2019-11-05 22-39-34

I installed it with:

source("https://install-github.me/REditorSupport/languageserver")

Not sure if there was a fix between 0.3.2 and master or if the change in paths handling on our side did the job, but please try giving it a go with R's languageserver master.

@syu-id
Copy link
Author

syu-id commented Nov 6, 2019

Thank you for testing out the issue.
Yes, completion works fine if packages are loaded individually eg. library(ggplot2).
The trouble is that if you use a single shot of library(tidyverse) to load the tidyverse packages, then completion for these packages is not available on JupyterLab (but it is available on other editors such as Vim).

@randy3k
Copy link

randy3k commented Nov 6, 2019

tidyverse uses a non stadnard way to attach packages to the current workspace. Due to computation cost, we only resolve the package dependencies of library(tidyverse) when the file is saved or opened.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 6, 2019

Ah, I feared that. There are some features of pyls that must have the same requirements, and no doubt, this will happen all over.

I've been dreading when we really had to start doing something different than other implementations, but it is kind of inevitable for the notebook case.

Crazy Option 1:
Over on https://github.com/krassowski/jupyterlab-lsp/pull/81#issuecomment-547914298, i proposed a potential way to react to incoming languages. We could (ab)use that to actually write files out to disk (buh).

@mgr.on_message(languages=".*", methods=r"textDocument/didSave$")
async def do_thing_with_message(message, sender, session):
    uri = message["textDocument"]["uri"]

    if ".lsp.ipynb" not in uri:
        return message

    if message["method"] == "textDocument/didChange":
        await patch_on_disk(uri, message["params"]["contentChanges"])

Crazy Option 2:
If URIs are actually URIs, and not always file:///s, we could run an https://localhost:8888/lsp/virtual and offer the files up that way, and never actually write to disk, but still use the listener interface. But i have a feeling that won't work for most servers.

Crazy Option 3:
Or... we start submitting PRs to language servers to start supporting .ipynb. We've got a leg up on this for python with https://github.com/deathbeds/importnb, and i've tried some approaches, this sounds like a ton of work.

Crazy Option 4:
We start patching Kernels to provide language server support.

@krassowski
Copy link
Member

@bollwyvl I suspect that we do not need to go so far for this case. LSP offers textDocument/didSave and I think that what Randy meant is that they listen for this signal rather than watch files. Anyways the issue is on us - as far as I remember we do not emit this signal.

But you are right that we need something like this for pyls - they use rope for renaming and it works exclusively on the filesystem (and this is why we do not have rename action in notebooks yet - experimented with it a few days ago)

Crazy option (1) sounds like the best short-term solution. The question is where and how to write notebooks. Probably we would want plain hidden files in the same directory as notebooks so that advanced refactoring tools do not have a problem with unresolved relative paths.

@krassowski krassowski added the bug Something isn't working label Nov 6, 2019
@randy3k
Copy link

randy3k commented Nov 6, 2019

@bollwyvl I suspect that we do not need to go so far for this case. LSP offers textDocument/didSave and I think that what Randy meant is that they listen for this signal rather than watch files. Anyways the issue is on us - as far as I remember we do not emit this signal. ...

Exactly, we listen for didSave and didOpen notifications to perform some of the more intensive jobs.

@randy3k
Copy link

randy3k commented Nov 6, 2019

I just realized that implementing textDocument/didSave is not sufficient to fix this. It is because languageserver loads the document from the disk upon textDocument/didOpen and textDocument/didSave. It will require some work in languageserver to support virtual documents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants