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

Language server not working with notebook cells #105391

Closed
renkun-ken opened this issue Aug 26, 2020 · 13 comments
Closed

Language server not working with notebook cells #105391

renkun-ken opened this issue Aug 26, 2020 · 13 comments
Assignees
Labels
notebook under-discussion Issue is under discussion for relevance, priority, approach

Comments

@renkun-ken
Copy link

renkun-ken commented Aug 26, 2020

  • VSCode Version: 1.49.0-insider
  • OS Version: Ubuntu 20.04

Related: REditorSupport/vscode-R#396.

Steps to Reproduce:

  1. Install vscode-R notebook visx from https://github.com/renkun-ken/vscode-R/actions/runs/211732805
  2. Install vscode-r-lsp and languageserver
  3. Debug the vscode-R extension and open an *.Rmd file as a notebook.
  4. The language server does not work with notebook code cells.

I have not seen any documentation (e.g. https://code.visualstudio.com/api/extension-guides/notebook) or example (e.g. https://github.com/microsoft/vscode-nodebook) yet talking about how existing language server/clients should be modified to work with notebook cells. It looks like Pylance is made to support notebook but it is not open source.

Does this issue occur when all extensions are disabled?: Yes

@jrieken jrieken self-assigned this Aug 26, 2020
@jrieken jrieken added notebook under-discussion Issue is under discussion for relevance, priority, approach labels Aug 26, 2020
@jrieken
Copy link
Member

jrieken commented Aug 26, 2020

@renkun-ken please ask me anything about this ;-) We have this (preview) guide/intro: https://github.com/microsoft/notebook-extension-samples/blob/master/notebook-language-guide/notebook-language-guide.md. Our findings are that this needs involvement from language service authors but we are happy to help!

@renkun-ken
Copy link
Author

renkun-ken commented Aug 26, 2020

@jrieken Thanks!

I made some changes to vscode-r-lsp and the language server now works nicely with notebook code cells!

But executeCell seems no longer works as cell.document got undefined at runtime. Also, changing cell.outputs does not work either. I'm not sure if there's significant api or runtime changes?

@jrieken
Copy link
Member

jrieken commented Aug 26, 2020

I made some changes to vscode-r-lsp and the language server now works nicely with notebook code cells!

Looking very promising... So, you create a new service per notebook-file since a notebook roughly corresponds to a project, right?

But executeCell seems no longer works as cell.document got undefined at runtime. Also, changing cell.outputs does not work either. I'm not sure if there's significant api or runtime changes?

Hm, that shouldn't happen and I cannot reproduce locally. Are you latest insiders and latest vscode.proposed.d.ts? Things are still changing quite a lot these days. With more steps I can also try to debug locally

@renkun-ken
Copy link
Author

renkun-ken commented Aug 26, 2020

Looking very promising... So, you create a new service per notebook-file since a notebook roughly corresponds to a project, right?

Yes, the R language service uses a multi-server approach to handle untitled documents, out-of-workspace files, and multi-root workspace. A notebook behaves like a project with multiple documents which share the same language server workspace.

Are you latest insiders and latest vscode.proposed.d.ts?

Yes, I'm using the latest vscode.proposed.d.ts and latest vscode insiders. cell.document and cell.metadata simply got undefined at runtime. I could reproduce this on both Ubuntu 20.04 and macOS 10.15.6. Not sure if I'm missing something?

@jrieken
Copy link
Member

jrieken commented Aug 27, 2020

cell.document and cell.metadata simply got undefined at runtime.

Can you point me to some code or let me try this? I have done some refactorings in that area but cell.document being undefined should really not happen given that we hold a reference to the document from start and don't allow to change it anymore (reference)

@renkun-ken
Copy link
Author

REditorSupport/vscode-R#394 is the R notebook implementation under development.

At https://github.com/Ikuyadeu/vscode-R/pull/394/files#diff-896fa9128d2e83307ffcf26741d257d2R66-R70, I got cell.document = undefined at runtime when I tried to evaluate a cell, which worked nicely previously but not with the latest version.

I could not get the text of the cell and nor could I set the output of the cell. It's very likely that I have messed up something here. It would be nice if you could clone the project and see if you could reproduce it.

@jrieken
Copy link
Member

jrieken commented Aug 27, 2020

@renkun-ken we have changed how kernels are registered. That might be the problem here. Instead of having it exposed as kernel field it must now be explicitly registered. E.g like so https://github.com/microsoft/vscode-github-issue-notebooks/blob/de869b2d2525f28c1fb7083830fe0d78f2f79b68/src/extension/notebookProvider.ts#L154

@renkun-ken
Copy link
Author

I tried the following but still no luck. I'm not sure how I could fill in the viewType field as the notebook document selector?

public label = 'R Kernel';
  // public kernel = this;

  private kernelScript: string;
  private disposables: vscode.Disposable[] = [];
  private readonly notebooks = new Map<string, RNotebook>();

  private runIndex: number = 0;

  constructor(kernelScript: string) {
    this.kernelScript = kernelScript;
    this.disposables.push(
      vscode.notebook.registerNotebookKernelProvider({
        viewType: 'r'
      }, {
          provideKernels: () => {
          return [this];
        }
      }),
      vscode.notebook.onDidOpenNotebookDocument(document => {
        const docKey = document.uri.toString();
        if (!this.notebooks.has(docKey)) {
          const notebook = new RNotebook(this.kernelScript, document);
          notebook.restartKernel();
          this.notebooks.set(docKey, notebook);
        }
      }),
      vscode.notebook.onDidCloseNotebookDocument(document => {
        const docKey = document.uri.toString();
        const notebook = this.notebooks.get(docKey);
        if (notebook) {
          notebook.dispose();
          this.notebooks.delete(docKey);
        }
      }),
    );
  }

@jrieken
Copy link
Member

jrieken commented Aug 27, 2020

Try r-notebook as viewType in the selector

@renkun-ken
Copy link
Author

r-notebook makes it work! 😄 Thanks!

@renkun-ken
Copy link
Author

Closing now.

Thanks for your timely answers! I'll file new issue if needed.

May I type @jrieken in the issues of vscode-R or vscode-r-lsp extensions repo regarding notebook questions? It's so efficient to get problem solved.

@jrieken
Copy link
Member

jrieken commented Aug 28, 2020

May I type @jrieken in the issues of vscode-R or vscode-r-lsp extensions repo regarding notebook questions? It's so efficient to get problem solved.

Yes, please do so! We are super keen on seeing what others do with native notebooks and we are very interested in feedback and issues that early adopters are facing. So, please let the feedback come, be it API or UX.

I will also subscribe you to #93265 on which we announce new and changed APIs.

@renkun-ken
Copy link
Author

Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
notebook under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

2 participants