feat: Add a Python language server #220
Conversation
📝 WalkthroughWalkthroughAdds LSP support for Deepnote notebooks. Introduces DeepnoteLspClientManager (per-notebook Python/SQL LSP clients), an exported IDeepnoteLspClientManager interface, and registers the implementation as a singleton and activation service. DeepnoteKernelAutoSelector now computes venv interpreter URIs and starts LSP clients after server provisioning. DeepnoteToolkitInstaller adds python-lsp-server to venv installs. Adds LSP.md documentation, tests for the manager and kernel selector, and a cspell entry for "pylsp". No existing public API signatures were removed. Sequence DiagramsequenceDiagram
actor User
participant VS as "VS Code"
participant KAS as "DeepnoteKernelAutoSelector"
participant LSP as "DeepnoteLspClientManager"
participant Server as "python-lsp-server (pylsp)"
participant Installer as "DeepnoteToolkitInstaller"
User->>VS: Open Deepnote notebook
VS->>KAS: Provision server & select kernel
rect rgb(220,255,220)
Note over KAS,LSP: After provisioning, LSP startup is invoked
KAS->>LSP: startLspClients(serverInfo, notebookUri, interpreter)
LSP->>LSP: Check pendingStarts & clients
alt not running
LSP->>LSP: createPythonLspClient()
LSP->>Server: Spawn pylsp (stdio)
Server-->>LSP: Initialize / Ready
LSP->>LSP: Store client per-notebook
LSP-->>KAS: Acknowledge LSP ready
else already running
LSP-->>KAS: Skip start (duplicate prevention)
end
end
rect rgb(255,245,220)
Note over VS,LSP: Teardown / disposal flows
VS->>LSP: stopLspClients(notebookUri) or stopAllClients()
LSP->>Server: Stop client process and dispose
LSP->>LSP: Remove entries from maps
LSP-->>VS: Stopped
end
rect rgb(235,235,255)
Note over Installer: Installer change
Installer->>Installer: Ensure python-lsp-server[all] installed in venv
end
Pre-merge checks✅ Passed checks (3 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
281-297: python-lsp-server install failures are silently treated as successYou log that deepnote-toolkit, ipykernel, and python-lsp-server are being installed, but
ensureVenvAndToolkitreturns success only ifisToolkitInstalled()passes. Since pip runs withthrowOnStdErr: false, a failed python-lsp-server install won't crash the process. If LSP fails to install while toolkit succeeds, the function reports success—and LSP startup fails later with a generic error, obscuring that the package was never installed.Add at minimum a check for
pylspavailability post-install (e.g.,python -m pylsp --help) and log a warning on failure, or treat python-lsp-server install failure as a hard error if LSP is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
LSP.md(1 hunks)src/kernels/deepnote/deepnoteLspClientManager.node.ts(1 hunks)src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts(1 hunks)src/kernels/deepnote/deepnoteToolkitInstaller.node.ts(2 hunks)src/kernels/deepnote/types.ts(1 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts(4 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts(4 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/test/datascience/.vscode/settings.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.tssrc/kernels/deepnote/deepnoteLspClientManager.node.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts
🧬 Code graph analysis (7)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/kernels/deepnote/types.ts (1)
DEEPNOTE_TOOLKIT_VERSION(346-346)
src/kernels/deepnote/types.ts (1)
src/kernels/jupyter/interpreter/jupyterCommand.node.ts (1)
interpreter(35-37)
src/notebooks/serviceRegistry.node.ts (1)
src/kernels/deepnote/types.ts (2)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-344)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1)
src/kernels/deepnote/types.ts (2)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-344)
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts (1)
.vscode-test.mjs (1)
__dirname(9-9)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)
src/kernels/deepnote/types.ts (2)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-344)src/platform/logging/index.ts (1)
logger(35-48)
src/kernels/deepnote/deepnoteLspClientManager.node.ts (2)
src/kernels/deepnote/types.ts (3)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-344)DeepnoteServerInfo(170-175)src/platform/common/types.ts (1)
IDisposable(211-213)
🪛 markdownlint-cli2 (0.18.1)
LSP.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
127-127: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
133-133: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
143-143: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
169-169: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
183-183: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
343-343: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
344-344: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
356-356: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
361-361: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
410-410: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
416-416: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
421-421: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
428-428: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
440-440: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
455-455: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
465-465: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
470-470: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
475-475: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
src/test/datascience/.vscode/settings.json (1)
40-40: No action needed. The.envfile exists atsrc/test/datascience/.envwith valid test configuration. Thepython.envFilesetting correctly references it and it's properly tracked in git—appropriate for test workspace data.src/kernels/deepnote/deepnoteLspClientManager.node.ts (1)
147-168: Document selector and watcher choices look reasonableThe document selectors target Deepnote notebook cells (
vscode-notebook-cell/filewith*.deepnote) and the file watcher is limited to**/*.py, which is a sane minimal scope for Python LSP features.No changes needed here; just noting that the selection is coherent with the Deepnote notebook format.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
29-29: LGTM - LSP client manager dependency properly injected.Standard DI pattern consistent with other service injections.
Also applies to: 44-44, 86-86
src/notebooks/serviceRegistry.node.ts (1)
68-68: LGTM - LSP client manager properly registered.Service registration follows established patterns, activation binding ensures lifecycle hooks fire correctly.
Also applies to: 74-74, 204-205
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/kernels/deepnote/deepnoteLspClientManager.node.ts (1)
227-232: Missing cleanup if client.start() fails.If
start()throws (e.g., pylsp not installed), theLanguageClientobject is created but never disposed, leaking its resources.Wrap start() with error handling:
// Check cancellation before starting client if (token?.isCancellationRequested) { throw new Error('Operation cancelled'); } - await client.start(); + try { + await client.start(); + } catch (error) { + logger.error('Failed to start Python LSP client', error); + await client.stop().catch(noop); + client.dispose(); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/kernels/deepnote/deepnoteLspClientManager.node.ts(1 hunks)src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts(1 hunks)src/kernels/deepnote/types.ts(1 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts(6 hunks)src/test/datascience/.vscode/settings.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteLspClientManager.node.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts
🧬 Code graph analysis (4)
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts (1)
.vscode-test.mjs (1)
__dirname(9-9)
src/kernels/deepnote/types.ts (2)
src/kernels/jupyter/interpreter/jupyterCommand.node.ts (1)
interpreter(35-37)src/test/mocks/vsc/index.ts (1)
CancellationToken(100-112)
src/kernels/deepnote/deepnoteLspClientManager.node.ts (3)
src/kernels/deepnote/types.ts (3)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-348)DeepnoteServerInfo(170-175)src/platform/common/types.ts (1)
IDisposable(211-213)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)
src/kernels/deepnote/types.ts (2)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-348)src/platform/logging/index.ts (1)
logger(35-48)
🪛 GitHub Actions: CI
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts
[warning] 48-48: Unknown word 'pylsp'.
[warning] 61-61: Unknown word 'pylsp'.
[warning] 99-99: Unknown word 'pylsp'.
src/kernels/deepnote/deepnoteLspClientManager.node.ts
[warning] 195-195: Unknown word 'pylsp'.
🔇 Additional comments (12)
src/test/datascience/.vscode/settings.json (2)
33-33: ✓ Correctly fixed hard-coded path.The workspace-relative reference replaces the previous machine-specific absolute path, making the configuration portable across developers and CI/CD environments.
40-40: No action needed. The.envfile exists.The
.envfile is present atsrc/test/datascience/.env, which is the correct location for this workspace. The${workspaceFolder}/.envsetting properly resolves to it. No warnings or missing file issues.src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts (4)
48-48: Pipeline warning is a false positive."pylsp" is the correct module name for python-lsp-server. Add it to the dictionary or ignore the warning.
79-86: Good edge case coverage.
88-93: LGTM.
95-126: Duplicate prevention test is sound.Same pipeline warning issue on line 99 (technical term, not a typo).
src/kernels/deepnote/types.ts (1)
320-348: Interface is clean and well-documented.CancellationToken support properly included on all async methods.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (4)
29-29: Dependency injection is correct.Also applies to: 44-44, 86-86
797-801: Good refactor addressing duplicate logic.Helper correctly handles platform-specific venv paths.
618-618: Consistent use of helper method.
573-576: Review comment is incorrect.PythonEnvironment interface contains only
idandurifields. The code provides both with correct types—no incomplete initialization. The type assertion is redundant but harmless.Likely an incorrect or invalid review comment.
src/kernels/deepnote/deepnoteLspClientManager.node.ts (1)
195-195: Pipeline warning is a false positive."pylsp" is the correct module name for python-lsp-server.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/kernels/deepnote/deepnoteLspClientManager.node.ts (3)
54-60: Concurrent callers bypass in-progress startup.When
pendingStartsis true, the method returns immediately instead of awaiting the ongoing operation. Callers may proceed before the LSP client fully initializes.Store the startup Promise in
pendingStartsand return it to concurrent callers:- private readonly pendingStarts = new Map<string, boolean>(); + private readonly pendingStarts = new Map<string, Promise<void>>();Then in
startLspClients:const pendingStart = this.pendingStarts.get(notebookKey); if (pendingStart) { logger.trace(`LSP client is already starting up for ${notebookKey}.`); - return; + return pendingStart; } ... - this.pendingStarts.set(notebookKey, true); + const startupPromise = (async () => { + // ... existing startup logic ... + })(); + this.pendingStarts.set(notebookKey, startupPromise); + + try { + await startupPromise; + } finally { + this.pendingStarts.delete(notebookKey); + }
182-186: Document fire-and-forget disposal behavior.
dispose()triggers async cleanup viavoid this.stopAllClients()but doesn't wait for completion. SinceIDisposable.dispose()is synchronous, this is necessary but should be documented.Add a comment:
+ /** + * Dispose of the manager synchronously. Note: LSP client cleanup happens asynchronously + * in the background. Clients are marked as disposed immediately but may take time to fully stop. + */ public dispose(): void { this.disposed = true; void this.stopAllClients(); }
236-246: Add error handling around client.start().If
client.start()throws (e.g., pylsp missing), theLanguageClientinstance created at lines 229-234 is never disposed, leaking resources.Wrap the start call:
// Check cancellation before starting client if (token?.isCancellationRequested) { throw new Error('Operation cancelled'); } - await client.start(); + try { + await client.start(); + } catch (error) { + logger.error('Failed to start Python LSP client', error); + client.dispose(); + throw error; + } logger.info(`Python LSP client started for ${notebookUri.toString()}`); return client;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
LSP.md(1 hunks)cspell.json(1 hunks)src/kernels/deepnote/deepnoteLspClientManager.node.ts(1 hunks)src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/deepnote/deepnoteLspClientManager.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
src/kernels/deepnote/types.ts (2)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-348)
src/kernels/deepnote/deepnoteLspClientManager.node.ts (1)
src/kernels/deepnote/types.ts (3)
IDeepnoteLspClientManager(320-320)IDeepnoteLspClientManager(321-348)DeepnoteServerInfo(170-175)
🪛 markdownlint-cli2 (0.18.1)
LSP.md
343-343: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
344-344: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
356-356: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
361-361: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
410-410: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
416-416: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
421-421: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
465-465: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
470-470: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
475-475: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (7)
cspell.json (1)
59-59: LGTM!Adding "pylsp" to the ignore list is appropriate—it's the standard abbreviation for python-lsp-server and is directly relevant to this PR's LSP integration. Placement is correct alphabetically.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
29-29: LGTM on dependency injection setup.Standard inversify pattern. Imports and injection align with the new LSP client manager interface.
Also applies to: 44-44, 86-86
571-587: LGTM on LSP client integration.Properly creates PythonEnvironment from venv interpreter, passes cancellation token, and handles errors. Kernel setup continues even if LSP startup fails, which is appropriate.
797-801: LGTM on helper extraction.Cleanly eliminates code duplication for platform-specific interpreter path resolution.
src/kernels/deepnote/deepnoteLspClientManager.node.ts (3)
30-36: LGTM on activation and registration.Constructor properly registers with disposables. Activation is appropriately minimal.
103-140: LGTM on stopLspClients implementation.Checks cancellation once before cleanup, then proceeds completely. Both
stop()anddispose()are called for each client. Error handling ensures both clients are attempted even if one fails.
142-180: LGTM on stopAllClients implementation.Properly chains
stop()thendispose()sequentially for each client. Parallel cleanup across clients is appropriate. Error handling prevents one failure from blocking others.
Set up and use the Deepnote LSP server for Python.
When writing Python code:
What doesn't work:
This also adds the "Deepnote Python LSP" output channel, where you can see all the communication with the LSP server.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.