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

Standalone transclusions are not reusing connections anymore #959

Open
krassowski opened this issue Aug 26, 2023 · 5 comments
Open

Standalone transclusions are not reusing connections anymore #959

krassowski opened this issue Aug 26, 2023 · 5 comments

Comments

@krassowski
Copy link
Member

Standalone documents represent fragments of code which are independent of any preceding and following code. An example in IPython is (each block represents a cell):

x = 1
%%python
print('I do not have access to `x` as I am a separate process')

There used to be a logic for reusing connections for standalone documents, which greatly improved performance (without it LSP becomes unavailable in the standalone document for 1-2 seconds while a new connection is established; it also risks saturating the number of websockets etc)

private choose_foreign_document(extractor: IForeignCodeExtractor) {
let foreign_document: VirtualDocument;
// if not standalone, try to append to existing document
let foreign_exists = this.foreign_documents.has(extractor.language);
if (!extractor.standalone && foreign_exists) {
foreign_document = this.foreign_documents.get(extractor.language)!;
this.unused_documents.delete(foreign_document);
} else {
// if standalone, try to re-use existing connection to the server
let unused_standalone = this.unused_standalone_documents.get(
extractor.language
);
if (extractor.standalone && unused_standalone.length > 0) {
foreign_document = unused_standalone.pop()!;
this.unused_documents.delete(foreign_document);
} else {
// if (previous document does not exists) or (extractor produces standalone documents
// and no old standalone document could be reused): create a new document
foreign_document = this.open_foreign(
extractor.language,
extractor.standalone,
extractor.file_extension
);
}
}
return foreign_document;
}

This was removed when the code was moved upstream to @jupyterlab/lsp - it is no longer in chooseForeignDocument. The removal was not complete as unusedStandaloneDocuments was retained, even though it is no longer populated.

It might be possible to monkeypatch the VirtualDocument downstream to restore this logic, or to restore this upstream but it might benefit from a larger refactor as arguably it was not very well written (it should be a responsibility of connection manager, not virtual document).

Before I start working on it I would like ask for some community support to indicate whether there is a need for this - if you are using this extension in JupyterLab 4.0 and are affected by the performance in standalone documents, please leave a comment/upvote.

@krassowski
Copy link
Member Author

This results in a new connection being opened on every keypress within the nested documents.

image

It is quite hard to resolve here, as all the problematic logic is upstream, but does upstream even want transclusions? The overrides were removed but extractors were retained, so I got mixed messages.

@krassowski
Copy link
Member Author

krassowski commented Aug 28, 2023

And, this also breaks diagnostics in nested documents because by the time the diagnostics response arrives the ID of virtual document has changed. only related to standalone documents

@krassowski
Copy link
Member Author

There is a partial workaround I included earlier, but this only prevents infinite loops, not replacement of virtual document connections:

protected async onForeignDocumentOpened(
_: VirtualDocument,
context: Document.IForeignContext
): Promise<void> {
/*
Opening a standalone foreign document can result in an inifnite loop,
as a new connection gets opened for these, and once that is ready
`updateDocuments()` gets called.
To avoid the problem, `updateDocuments()` gets suppressed for standalone
documents. It does not affect non-standalone documents, because no new
connection gets opened for these.
*/
try {
this._blockUpdates = true;
await super.onForeignDocumentOpened(_, context);
} finally {
this._blockUpdates = false;
}
}
updateDocuments(): Promise<void> {
if (this._blockUpdates) {
return Promise.resolve();
}
return super.updateDocuments();
}
private _blockUpdates = false;

@trungleduc
Copy link

It is quite hard to resolve here, as all the problematic logic is upstream, but does upstream even want transclusions? The overrides were removed but extractors were retained, so I got mixed messages.

IIRC we decided to drop the transclusion via magics but keep supporting the LSP for different cell types, that's why the extractors were kept.

@gogakoreli
Copy link

I agree this issue needs to be solved, because if I want to have different cells to have different language support inside the python notebook, I need to have standalone virtual documents for each cell respectively (specific use case is that I can have sql cells and python cells at the same time inside the notebook). Now this is an issue in JL4, which was not happening in JL3. As shown in the screenshot every keystroke opens a new connection if I specify standalone as true and causes infinitely duplicated lsp connection to the same cells.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants