Adding support NotebookDocumentClientCapabilities

[MrBago]

The LSP 3.17 specification added support for notebook documents via NotebookDocumentClientCapabilities. I'd love to have support for this capability in python-lsp-server and to be able to use the language server with Jupyter or other notebooks.

I'd like to understand what would be need and how much effort it would be to add support for notebook documents.

If there is interest in including this improvement and someone could help provide guidance on the work, I may have capacity to work on this and submit a patch.

[ccordoba12]

Hey @MrBago, thanks for your interest in adding this feature! I think @krassowski is the right person to provide guidance here because he has worked a lot on integrating LSP features in notebooks.

[MrBago]

@ccordoba12 Thanks for the response. I've reached out to @krassowski as well. The main guidance I need is around understanding how text syncing currently works. The NotebookDocumentClientCapability is structured on top of the text syncing capability where a notebook is a collection of text documents. To support the notebook we'd need to be able to have multiple text documents represent a single "unit" for static or error analysis.

Does the language server keep an internal copy of documents when handing text sync messages? Is the text sync capability written as a plugin?

[krassowski]

The LSP 3.17 specification added support for notebook documents via NotebookDocumentClientCapabilities

It did indeed...

I'd love to have support for this capability in python-lsp-server and to be able to use the language server with Jupyter or other notebooks.

🎉. Happy to help.

I'd like to understand what would be need and how much effort it would be to add support for notebook documents.

I would naively say that this should should be rather straightforward.

Does the language server keep an internal copy of documents when handing text sync messages?

Yes.

Is the text sync capability written as a plugin?

No. textDocument/didChange is handled here:

python-lsp-server/pylsp/python_lsp.py

Lines 411 to 419 in 2e98dfa

	def m_text_document__did_change(self, contentChanges=None, textDocument=None, **_kwargs):
	workspace = self._match_uri_to_workspace(textDocument['uri'])
	for change in contentChanges:
	workspace.update_document(
	textDocument['uri'],
	change,
	version=textDocument.get('version')
	)
	self.lint(textDocument['uri'], is_saved=False)

python-lsp-server/pylsp/workspace.py

Lines 116 to 118 in 2e98dfa

	def update_document(self, doc_uri, change, version=None):
	self._docs[doc_uri].apply_change(change)
	self._docs[doc_uri].version = version

Briefly, update_document stores an in-memory cache of the source file and updates it as needed. However, some plugins (annoyingly) relay on the file existing on disk (and being up to date/saved), e.g.:

python-lsp-server/pylsp/plugins/pylint_lint.py

Line 272 in 2e98dfa

cmd.extend(['--from-stdin', document.path])

I think that we should rewrite such plugins and remove the document path from public API (maybe it is too drastic, but hiding behind underscore may make plugin developers think twice before they use it).

[rchl]

Briefly, update_document stores an in-memory cache of the source file and updates it as needed. However, some plugins (annoyingly) relay on the file existing on disk (and being up to date/saved), e.g.:

python-lsp-server/pylsp/plugins/pylint_lint.py

Line 272 in 2e98dfa

cmd.extend(['--from-stdin', document.path])

I think that we should rewrite such plugins and remove the document path from public API (maybe it is too drastic, but hiding behind underscore may make plugin developers think twice before they use it).

This particulate example doesn't rely on file being on disk though. It gets data from stdin. The path is passed because some linter rules might need the file path to be able to work.

[samrat]

Are there any updates on this issue?

[ccordoba12]

No, there aren't, sorry.

[tkrabel-db]

Hey folks - picking this up from @MrBago. I got capacity to work on this the next two weeks. 🎉 Will add a comment later about my thoughts on this!

[tkrabel-db]

@krassowski I looked into the LSP specs regarding the NotebookDocument support and I am not 100% sure how things fit together. Do you have a clear idea how to implement notebook support from reading the specs?

Do you have time for a call to discuss this? Others are also happily invited!

To give you context, I have the following open questions:

• I am not sure what notebookCellTextDocumentFilter is useful for.
• In the specs, it says that cell contents can be synchronized to the server using the standard textDocument/did* notification. Assume we do that: how would the Language Server get the content of the other cells for hooks that need the full notebook content?
• In general, I am thinking about how to add notebook support without adding regressions to the normal textDocument support. Maybe the notebookDocument/* notifications help...

[tkrabel-db]

FYI: I have a draft PR open, see #389