From 1cfe11cdb48b97deee59a12215f4841d1ec1ea2d Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Sun, 16 Apr 2023 23:08:21 +0100 Subject: [PATCH 1/6] Make `.virtual_documents` optional, e.g. in pyright --- docs/Configuring.ipynb | 14 +++++--- .../jupyter_lsp/schema/schema.json | 5 +++ .../jupyter_lsp/serverextension.py | 15 ++++++-- .../jupyter_lsp/jupyter_lsp/specs/pyright.py | 1 + .../tests/test_virtual_documents_shadow.py | 35 +++++++++++++++---- .../jupyter_lsp/jupyter_lsp/types.py | 6 ++-- .../jupyter_lsp/virtual_documents_shadow.py | 8 ++++- 7 files changed, 69 insertions(+), 15 deletions(-) diff --git a/docs/Configuring.ipynb b/docs/Configuring.ipynb index 3c376b3fa..5860ebc22 100644 --- a/docs/Configuring.ipynb +++ b/docs/Configuring.ipynb @@ -80,6 +80,9 @@ " matching the `contentsModel` against the registered file types using\n", " `getFileTypeForModel()` method (if multiple MIME types are present, the\n", " first one will be used).\n", + "- `requires_documents_on_disk` should be `false` for all new specifications, as\n", + " any code paths requiring documents on disks should be fixed in the LSP servers\n", + " rather than masked by using the `.virtual_documents` workaround.\n", "\n", "```python\n", "# ./jupyter_server_config.json ---------- unique! -----------\n", @@ -211,10 +214,13 @@ "\n", "> default: os.getenv(\"JP_LSP_VIRTUAL_DIR\", \".virtual_documents\")\n", "\n", - "Path to virtual documents relative to the content manager root directory.\n", + "Path (relative to the content manager root directory) where a transient copy of\n", + "the virtual documents should be written allowing LSP servers to access the file\n", + "using operating system's file system APIs if they need so (which is\n", + "discouraged).\n", "\n", - "Its default value can be set with `JP_LSP_VIRTUAL_DIR` environment variable and\n", - "fallback to `.virtual_documents`." + "Its default value can be set with `JP_LSP_VIRTUAL_DIR` environment variable. The\n", + "fallback value is `.virtual_documents`." ] }, { @@ -340,7 +346,7 @@ "name": "python", "nbconvert_exporter": "python", "pygments_lexer": "ipython3", - "version": "3.7.9" + "version": "3.8.6" }, "widgets": { "application/vnd.jupyter.widget-state+json": { diff --git a/python_packages/jupyter_lsp/jupyter_lsp/schema/schema.json b/python_packages/jupyter_lsp/jupyter_lsp/schema/schema.json index 61f31d1cb..a5c4b43c9 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/schema/schema.json +++ b/python_packages/jupyter_lsp/jupyter_lsp/schema/schema.json @@ -126,6 +126,11 @@ "title": "Extensions", "type": "array" }, + "requires_documents_on_disk": { + "default": true, + "description": "Whether to write un-saved documents to disk in a transient `.virtual_documents` directory. Well-behaved language servers that work against in-memory files should set this to `false`, which will become the default in the future.", + "type": "boolean" + }, "install": { "$ref": "#/definitions/install-bundle", "description": "a list of installation approaches keyed by package manager, e.g. pip, npm, yarn, apt", diff --git a/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py b/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py index 008b7b5cf..638b9cd28 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py @@ -16,12 +16,23 @@ async def initialize(nbapp, virtual_documents_uri): # pragma: no cover from .virtual_documents_shadow import setup_shadow_filesystem - manager = nbapp.language_server_manager + manager: LanguageServerManager = nbapp.language_server_manager with concurrent.futures.ThreadPoolExecutor() as pool: await nbapp.io_loop.run_in_executor(pool, manager.initialize) - setup_shadow_filesystem(virtual_documents_uri=virtual_documents_uri) + servers_requiring_disk_access = [ + server_id + for server_id, server in manager.language_servers.items() + if server.get("requires_documents_on_disk", True) + ] + + if any(servers_requiring_disk_access): + nbapp.log.debug( + "[lsp] servers that requested virtual documents on disk: %s", + servers_requiring_disk_access, + ) + setup_shadow_filesystem(virtual_documents_uri=virtual_documents_uri) nbapp.log.debug( "[lsp] The following Language Servers will be available: {}".format( diff --git a/python_packages/jupyter_lsp/jupyter_lsp/specs/pyright.py b/python_packages/jupyter_lsp/jupyter_lsp/specs/pyright.py index 2df194207..5eade382c 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/specs/pyright.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/specs/pyright.py @@ -20,4 +20,5 @@ class PyrightLanguageServer(NodeModuleSpec): jlpm="jlpm add --dev {}".format(key), ), config_schema=load_config_schema(key), + requires_documents_on_disk=False, ) diff --git a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py index 31f69ccb2..80c2ddb65 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py @@ -1,4 +1,5 @@ from pathlib import Path +from types import SimpleNamespace from typing import List import pytest @@ -90,6 +91,13 @@ def shadow_path(tmpdir): return str(tmpdir.mkdir(".virtual_documents")) +@pytest.fixture +def manager(): + return SimpleNamespace( + language_servers={"python-lsp-server": {"requires_documents_on_disk": True}} + ) + + @pytest.mark.asyncio @pytest.mark.parametrize( "message_func, content, expected_content", @@ -99,26 +107,41 @@ def shadow_path(tmpdir): [did_save_with_text, "content at save", "content at save"], ], ) -async def test_shadow(shadow_path, message_func, content, expected_content): +async def test_shadow(shadow_path, message_func, content, expected_content, manager): shadow = setup_shadow_filesystem(Path(shadow_path).as_uri()) ok_file_path = Path(shadow_path) / "test.py" message = message_func(ok_file_path.as_uri(), content) - result = await shadow("client", message, ["python"], None) + result = await shadow("client", message, "python-lsp-server", manager) assert isinstance(result, str) - with open(str(ok_file_path)) as f: # str is a Python 3.5 relict + with open(ok_file_path) as f: assert f.read() == expected_content -@pytest.mark.asyncio -async def test_shadow_failures(shadow_path): +async def test_short_circuits_for_well_behaved_server( + shadow_path, +): + """We call server well behaved when it does not require a disk copy""" + shadow = setup_shadow_filesystem(Path(shadow_path).as_uri()) + ok_file_path = Path(shadow_path) / "test.py" + manager = SimpleNamespace( + language_servers={"python-lsp-server": {"requires_documents_on_disk": False}} + ) + + message = did_open(ok_file_path.as_uri(), "content\nof\nopened\nfile") + result = await shadow("client", message, "python-lsp-server", manager) + assert result is None + + +@pytest.mark.asyncio +async def test_shadow_failures(shadow_path, manager): shadow = setup_shadow_filesystem(Path(shadow_path).as_uri()) ok_file_uri = (Path(shadow_path) / "test.py").as_uri() def run_shadow(message): - return shadow("client", message, ["python"], None) + return shadow("client", message, "python-lsp-server", manager) # missing textDocument with pytest.raises(ShadowFilesystemError, match="Could not get textDocument from"): diff --git a/python_packages/jupyter_lsp/jupyter_lsp/types.py b/python_packages/jupyter_lsp/jupyter_lsp/types.py index 0c52afa48..6641330be 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/types.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/types.py @@ -44,7 +44,7 @@ def __call__( scope: Text, message: LanguageServerMessage, language_server: Text, - manager: "HasListeners", + manager: "LanguageServerManagerAPI", ) -> Awaitable[None]: ... @@ -89,7 +89,7 @@ async def __call__( scope: Text, message: LanguageServerMessage, language_server: Text, - manager: "HasListeners", + manager: "LanguageServerManagerAPI", ) -> None: """actually dispatch the message to the listener and capture any errors""" try: @@ -195,6 +195,8 @@ async def wait_for_listeners( class LanguageServerManagerAPI(LoggingConfigurable, HasListeners): """Public API that can be used for python-based spec finders and listeners""" + language_servers: KeyedLanguageServerSpecs + nodejs = Unicode(help=_("path to nodejs executable")).tag(config=True) node_roots = List_([], help=_("absolute paths in which to seek node_modules")).tag( diff --git a/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py b/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py index f014b0a25..5cbdb60ef 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py @@ -8,6 +8,7 @@ from .manager import lsp_message_listener from .paths import file_uri_to_path +from .types import LanguageServerManagerAPI # TODO: make configurable MAX_WORKERS = 4 @@ -101,7 +102,7 @@ class ShadowFilesystemError(ValueError): """Error in the shadow file system.""" -def setup_shadow_filesystem(virtual_documents_uri): +def setup_shadow_filesystem(virtual_documents_uri: str): if not virtual_documents_uri.startswith("file:/"): raise ShadowFilesystemError( # pragma: no cover @@ -125,6 +126,11 @@ async def shadow_virtual_documents(scope, message, language_server, manager): Returns the path on filesystem where the content was stored. """ + # short-circut if language server does not require documents on disk + server_spec = manager.language_servers[language_server] + if not server_spec.get("requires_documents_on_disk", True): + return + if not message.get("method") in WRITE_ONE: return From 076a04c18c419e03478750b5abed38b6d5000e11 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Tue, 18 Apr 2023 22:28:40 +0100 Subject: [PATCH 2/6] Cast type in `HasListeners` mixing --- python_packages/jupyter_lsp/jupyter_lsp/types.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python_packages/jupyter_lsp/jupyter_lsp/types.py b/python_packages/jupyter_lsp/jupyter_lsp/types.py index 6641330be..d9581f809 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/types.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/types.py @@ -20,12 +20,14 @@ Pattern, Text, Union, + cast, ) try: from jupyter_server.transutils import _i18n as _ except ImportError: # pragma: no cover from jupyter_server.transutils import _ + from traitlets import Instance from traitlets import List as List_ from traitlets import Unicode, default @@ -182,7 +184,7 @@ async def wait_for_listeners( scope_val, message=message, language_server=language_server, - manager=self, + manager=cast("LanguageServerManagerAPI", self), ) for listener in listeners if listener.wants(message, language_server) From e1d65e2d31353a9ff4b582903eb4afc91e087022 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Tue, 18 Apr 2023 22:29:48 +0100 Subject: [PATCH 3/6] Include new test in asyncio group --- .../jupyter_lsp/tests/test_virtual_documents_shadow.py | 1 + 1 file changed, 1 insertion(+) diff --git a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py index 80c2ddb65..e393c2249 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py @@ -119,6 +119,7 @@ async def test_shadow(shadow_path, message_func, content, expected_content, mana assert f.read() == expected_content +@pytest.mark.asyncio async def test_short_circuits_for_well_behaved_server( shadow_path, ): From 2a7126fb27276747c038702f096c56c195bdcf08 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 19 Apr 2023 22:16:39 +0100 Subject: [PATCH 4/6] Delay creation of `.virtual_documents` until actually needed because deployments may primarily use a well-behaved server that does not need virtual documents folder together with secondary servers that do require it but are only used from time to time when opening specific file types. This can also marginally improve startup times as there is no need to wait on file system operation of purging the directory which is now only done on first relevant LSP message. --- .../jupyter_lsp/serverextension.py | 7 ++++- .../tests/test_virtual_documents_shadow.py | 30 +++++++++++++++++-- .../jupyter_lsp/virtual_documents_shadow.py | 18 +++++++---- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py b/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py index 638b9cd28..2640e429f 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/serverextension.py @@ -29,10 +29,15 @@ async def initialize(nbapp, virtual_documents_uri): # pragma: no cover if any(servers_requiring_disk_access): nbapp.log.debug( - "[lsp] servers that requested virtual documents on disk: %s", + "[lsp] Servers that requested virtual documents on disk: %s", servers_requiring_disk_access, ) setup_shadow_filesystem(virtual_documents_uri=virtual_documents_uri) + else: + nbapp.log.debug( + "[lsp] None of the installed servers require virtual documents" + " disabling shadow filesystem." + ) nbapp.log.debug( "[lsp] The following Language Servers will be available: {}".format( diff --git a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py index e393c2249..426b1970f 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py @@ -120,12 +120,13 @@ async def test_shadow(shadow_path, message_func, content, expected_content, mana @pytest.mark.asyncio -async def test_short_circuits_for_well_behaved_server( +async def test_no_shadow_for_well_behaved_server( shadow_path, ): """We call server well behaved when it does not require a disk copy""" - shadow = setup_shadow_filesystem(Path(shadow_path).as_uri()) - ok_file_path = Path(shadow_path) / "test.py" + shadow_path_for_well = Path(shadow_path) / "well" + shadow = setup_shadow_filesystem(Path(shadow_path_for_well).as_uri()) + ok_file_path = Path(shadow_path_for_well) / "test.py" manager = SimpleNamespace( language_servers={"python-lsp-server": {"requires_documents_on_disk": False}} @@ -133,7 +134,30 @@ async def test_short_circuits_for_well_behaved_server( message = did_open(ok_file_path.as_uri(), "content\nof\nopened\nfile") result = await shadow("client", message, "python-lsp-server", manager) + # should short-circuit for well behaved server assert result is None + # should not create the directory + assert not shadow_path_for_well.exists() + + +@pytest.mark.asyncio +async def test_shadow_created_for_ill_behaved_server( + shadow_path, +): + shadow_path_for_ill = Path(shadow_path) / "ill" + shadow = setup_shadow_filesystem(shadow_path_for_ill.as_uri()) + ok_file_path = Path(shadow_path_for_ill) / "test.py" + + manager = SimpleNamespace( + language_servers={"python-lsp-server": {"requires_documents_on_disk": True}} + ) + + message = did_open(ok_file_path.as_uri(), "content\nof\nopened\nfile") + result = await shadow("client", message, "python-lsp-server", manager) + assert result is not None + # should create the directory at given path + assert shadow_path_for_ill.exists() + assert shadow_path_for_ill.is_dir() @pytest.mark.asyncio diff --git a/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py b/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py index 5cbdb60ef..33caead60 100644 --- a/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py +++ b/python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py @@ -110,13 +110,8 @@ def setup_shadow_filesystem(virtual_documents_uri: str): + virtual_documents_uri ) + initialized = False shadow_filesystem = Path(file_uri_to_path(virtual_documents_uri)) - # create if does no exist (so that removal does not raise) - shadow_filesystem.mkdir(parents=True, exist_ok=True) - # remove with contents - rmtree(str(shadow_filesystem)) - # create again - shadow_filesystem.mkdir(parents=True, exist_ok=True) @lsp_message_listener("client") async def shadow_virtual_documents(scope, message, language_server, manager): @@ -125,6 +120,7 @@ async def shadow_virtual_documents(scope, message, language_server, manager): Only create the shadow file if the URI matches the virtual documents URI. Returns the path on filesystem where the content was stored. """ + nonlocal initialized # short-circut if language server does not require documents on disk server_spec = manager.language_servers[language_server] @@ -147,6 +143,16 @@ async def shadow_virtual_documents(scope, message, language_server, manager): if not uri.startswith(virtual_documents_uri): return + # initialization (/any file system operations) delayed until needed + if not initialized: + # create if does no exist (so that removal does not raise) + shadow_filesystem.mkdir(parents=True, exist_ok=True) + # remove with contents + rmtree(str(shadow_filesystem)) + # create again + shadow_filesystem.mkdir(parents=True, exist_ok=True) + initialized = True + path = file_uri_to_path(uri) editable_file = EditableFile(path) From fac9ff6f6c392cca17dc1b7cdec662a9c174d19a Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Sun, 23 Apr 2023 17:43:40 +0100 Subject: [PATCH 5/6] Use `rootUri` when server does not require on-disk files --- .../jupyterlab-lsp/src/connection_manager.ts | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/jupyterlab-lsp/src/connection_manager.ts b/packages/jupyterlab-lsp/src/connection_manager.ts index f152bc842..2d6e5e17a 100644 --- a/packages/jupyterlab-lsp/src/connection_manager.ts +++ b/packages/jupyterlab-lsp/src/connection_manager.ts @@ -391,27 +391,36 @@ export namespace DocumentConnectionManager { virtual_document: VirtualDocument, language: string ): IURIs { - const { settings } = Private.getLanguageServerManager(); - const wsBase = settings.wsUrl; + const serverManager = Private.getLanguageServerManager(); + const wsBase = serverManager.settings.wsUrl; const rootUri = PageConfig.getOption('rootUri'); const virtualDocumentsUri = PageConfig.getOption('virtualDocumentsUri'); - const baseUri = virtual_document.has_lsp_supported_file - ? rootUri - : virtualDocumentsUri; - // for now take the best match only - const matchingServers = - Private.getLanguageServerManager().getMatchingServers({ - language - }); - const language_server_id = + const serverOptions: ILanguageServerManager.IGetServerIdOptions = { + language + }; + const matchingServers = serverManager.getMatchingServers(serverOptions); + const languageServerId = matchingServers.length === 0 ? null : matchingServers[0]; - if (language_server_id === null) { + if (languageServerId === null) { throw `No language server installed for language ${language}`; } + const specs = serverManager.getMatchingSpecs(serverOptions); + const spec = specs.get(languageServerId); + if (!spec) { + throw `Specification not available for server ${languageServerId}`; + } + const requiresOnDiskFiles = spec.requires_documents_on_disk ?? true; + const supportsInMemoryFiles = !requiresOnDiskFiles; + + const baseUri = + virtual_document.has_lsp_supported_file || supportsInMemoryFiles + ? rootUri + : virtualDocumentsUri; + // workaround url-parse bug(s) (see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/595) let documentUri = URLExt.join(baseUri, virtual_document.uri); if ( @@ -435,7 +444,7 @@ export namespace DocumentConnectionManager { wsBase, ILanguageServerManager.URL_NS, 'ws', - language_server_id + languageServerId ) }; } From b4136fb2ff8de03febbcfb2fe00c78d29daabb47 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Sun, 23 Apr 2023 18:03:03 +0100 Subject: [PATCH 6/6] Only warn about missing spec for now --- packages/jupyterlab-lsp/src/connection_manager.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/jupyterlab-lsp/src/connection_manager.ts b/packages/jupyterlab-lsp/src/connection_manager.ts index 2d6e5e17a..2399a5bc0 100644 --- a/packages/jupyterlab-lsp/src/connection_manager.ts +++ b/packages/jupyterlab-lsp/src/connection_manager.ts @@ -411,9 +411,11 @@ export namespace DocumentConnectionManager { const specs = serverManager.getMatchingSpecs(serverOptions); const spec = specs.get(languageServerId); if (!spec) { - throw `Specification not available for server ${languageServerId}`; + console.warn( + `Specification not available for server ${languageServerId}` + ); } - const requiresOnDiskFiles = spec.requires_documents_on_disk ?? true; + const requiresOnDiskFiles = spec?.requires_documents_on_disk ?? true; const supportsInMemoryFiles = !requiresOnDiskFiles; const baseUri =