Skip to content

Conversation

@jankuca
Copy link
Contributor

@jankuca jankuca commented Oct 5, 2025

Summary by CodeRabbit

  • New Features
    • Server-native Python kernel selection and persistent server/controller reuse for more stable notebooks.
  • Bug Fixes
    • Fewer installation failures by upgrading pip inside venvs, reordering install steps, and treating venv warnings (stderr) as non‑fatal.
  • Chores
    • Improved install logging and cancellation handling, ensured ipykernel gets installed in venvs, documentation updates, and added a Markdown formatting rule.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

📝 Walkthrough

Walkthrough

  • Adds a pip-upgrade step inside the created venv (python -m pip install --upgrade pip) with stdout/stderr logging and cancellation checks.
  • Defers creation/use of the venv process service until after venv creation and interpreter verification.
  • Installs deepnote-toolkit and ipykernel using the venv Python; installer stderr is logged but no longer causes a hard throw; cancellation checks added.
  • Surfaces venv-creation warnings (stderr) without failing the install flow.
  • Switches kernel handling to prefer server-native kernel specs (startUsingDeepnoteKernel) and venv-based environment config rather than registering per-venv kernel specs.
  • Adds Deepnote-specific public types/interfaces and services (DeepnoteKernelConnectionMetadata, IDeepnoteToolkitInstaller, IDeepnoteServerStarter, IDeepnoteKernelAutoSelector, DeepnoteServerInfo, DeepnoteServerProvider) and wires them into service registration.
  • Adds a Prettier override for Markdown files (tabWidth: 2).

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant VenvCreator as Venv Creation
  participant VenvProcSvc as Venv Process Service
  participant PythonVenv as venv Python
  participant Installer as pip installer (toolkit + ipykernel)
  participant ServerMgr as Deepnote Server / Kernel Manager

  Note over Client,VenvCreator: start venv creation
  Client->>VenvCreator: create venv
  VenvCreator-->>Client: venv path + stderr warnings (logged) / interpreter verified

  Note over VenvCreator,VenvProcSvc: after venv created
  VenvCreator->>VenvProcSvc: create process service for venv
  VenvProcSvc->>PythonVenv: run "python -m pip install --upgrade pip"
  PythonVenv-->>VenvProcSvc: stdout/stderr (logged) / cancellation check

  VenvProcSvc->>Installer: run pip install deepnote-toolkit ipykernel (use venv Python)
  Installer-->>VenvProcSvc: stdout/stderr (logged, do not throw on stderr) / cancellation check

  VenvProcSvc->>ServerMgr: provide venv-based env (PATH, VIRTUAL_ENV, PYTHONHOME)
  ServerMgr-->>Client: prefer server-native kernel spec (startUsingDeepnoteKernel) / server started or reused
Loading

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the main change of adding a pip upgrade step to the Deepnote toolkit installation process and aligns with the key modifications in the PR by focusing on pip upgrading in the installer flow.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 940b663 and 41cb843.

📒 Files selected for processing (1)
  • DEEPNOTE_KERNEL_IMPLEMENTATION.md (12 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
📚 Learning: 2025-09-25T13:16:13.796Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/{interactiveWindow.ts,interactiveWindowController.ts} : Implement robust kernel error handling: automatic restart, clear error display in system info cells, graceful degradation when kernels are unavailable, and user notifications for critical failures

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
🪛 markdownlint-cli2 (0.18.1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md

182-182: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


187-187: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


192-192: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1

(MD029, ol-prefix)


229-229: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


235-235: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c35f414 and b67da89.

📒 Files selected for processing (1)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
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.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/kernels/execution/cellExecutionCreator.ts (1)
  • token (41-43)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b67da89 and 12fd559.

📒 Files selected for processing (1)
  • DEEPNOTE_KERNEL_IMPLEMENTATION.md (6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
🪛 markdownlint-cli2 (0.18.1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md

51-51: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


52-52: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


53-53: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


116-116: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


185-185: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


190-190: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


195-195: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1

(MD029, ol-prefix)


232-232: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


238-238: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


421-421: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


424-424: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


425-425: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


426-426: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md (1)

115-161: Annotate the flow diagram fence.

The fence still lacks a language marker; add one (e.g., ```text) so MD040 stops failing.

-```
+```text
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12fd559 and 2018067.

📒 Files selected for processing (1)
  • DEEPNOTE_KERNEL_IMPLEMENTATION.md (7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
📚 Learning: 2025-09-25T13:16:13.796Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
🪛 markdownlint-cli2 (0.18.1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md

50-50: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


51-51: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


52-52: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


115-115: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


182-182: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


187-187: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


192-192: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1

(MD029, ol-prefix)


229-229: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


235-235: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


415-415: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


416-416: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)


417-417: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2018067 and 940b663.

📒 Files selected for processing (2)
  • .prettierrc.js (1 hunks)
  • DEEPNOTE_KERNEL_IMPLEMENTATION.md (12 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
📚 Learning: 2025-09-25T13:16:13.796Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:0-0
Timestamp: 2025-09-25T13:16:13.796Z
Learning: In Deepnote's Docker environment setup, packages installed in the underlying Docker image should be in system site packages, not in virtual environments. Server/kernels are launched in a virtual environment created by the installer, which creates a pth file where system site packages are appended first, giving them higher precedence and making them first in the search path. This is expected behavior for the deepnote-toolkit setup.

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/{interactiveWindow.ts,interactiveWindowController.ts} : Implement robust kernel error handling: automatic restart, clear error display in system info cells, graceful degradation when kernels are unavailable, and user notifications for critical failures

Applied to files:

  • DEEPNOTE_KERNEL_IMPLEMENTATION.md
🪛 markdownlint-cli2 (0.18.1)
DEEPNOTE_KERNEL_IMPLEMENTATION.md

115-115: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


182-182: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


187-187: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)


192-192: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1

(MD029, ol-prefix)


229-229: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1

(MD029, ol-prefix)


235-235: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1

(MD029, ol-prefix)

🔇 Additional comments (1)
.prettierrc.js (1)

14-19: LGTM.

Markdown override mirrors the YAML pattern correctly. tabWidth consistency maintained.

@jankuca jankuca marked this pull request as ready for review October 5, 2025 16:26
@jankuca jankuca merged commit c5b24c3 into main Oct 5, 2025
3 checks passed
@jankuca jankuca deleted the jk/fix/pip-install branch October 5, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants