Skip to content

Conversation

@Artmann
Copy link
Member

@Artmann Artmann commented Oct 9, 2025

Previously, ipykernel specs were installed with --user flag, placing them in ~/Library/Jupyter/kernels/. This caused the Deepnote Jupyter server to fall back to generic Python kernels because it was unable to discover the venv-specific kernel spec.

This resulted in a mismatch where:

  • Shell commands (!pip install) used the venv's Python (via PATH)
  • But the kernel interpreter used the system Python
  • Leading to ModuleNotFoundError for packages installed in the venv

Now installing kernel specs with --prefix into the venv itself at /share/jupyter/kernels/. Jupyter automatically discovers kernel specs in this location when started with the venv's Python, ensuring the correct interpreter is used.

Each .deepnote file maintains its own isolated venv with its own kernel spec, preventing conflicts when multiple notebooks are open.

Summary by CodeRabbit

  • Bug Fixes

    • Kernel installation now targets the selected virtual environment and waits/retries for any pending or failed installations to avoid conflicts.
  • Chores

    • Improved runtime logging: shows virtual environment path at start, notes when waiting for other installs, and reports the exact kernel spec destination; existing error handling unchanged.
  • Documentation

    • Added guidance to use Uri.joinPath() for platform-correct file path construction.

Previously, ipykernel specs were installed with --user flag, placing them
in ~/Library/Jupyter/kernels/. This caused the Deepnote Jupyter server to
fall back to generic Python kernels because it couldn't discover the
venv-specific kernel spec.

This resulted in a mismatch where:
- Shell commands (!pip install) used the venv's Python (via PATH)
- But the kernel interpreter used system Python
- Leading to ModuleNotFoundError for packages installed in the venv

Now installing kernel specs with --prefix into the venv itself at
<venv>/share/jupyter/kernels/. Jupyter automatically discovers kernel
specs in this location when started with the venv's Python, ensuring
the correct interpreter is used.

Each .deepnote file maintains its own isolated venv with its own kernel
spec, preventing conflicts when multiple notebooks are open.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

📝 Walkthrough

Walkthrough

  • Logs the virtual environment path at installation start.
  • Waits for and logs any pending installation for the same venv, retrying if a prior install failed.
  • Changes kernel spec installation from user scope (--user) to venv-scoped (--prefix <venvPath>).
  • Computes and logs the exact kernel spec install path inside the venv on success.
  • Failure handling remains unchanged; surrounding logs updated to reflect venv-targeted behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant Installer as DeepnoteToolkitInstaller
  participant Venv as VirtualEnv
  participant Kernel as ipykernel installer

  User->>Installer: startInstall(venvPath)
  Installer->>Installer: log("Installing into venv:", venvPath)
  Installer->>Installer: checkPendingInstall(venvPath)
  alt pending install exists
    Installer->>Installer: log("Waiting for pending install for venv")
    Installer-->>Installer: await pending completion / retry if needed
  else no pending
    Note over Installer: Proceed immediately
  end

  Installer->>Kernel: install --prefix venvPath
  Kernel-->>Installer: success | error

  alt success
    Installer->>Installer: compute kernelSpecPath in venv
    Installer->>Installer: log("Kernel spec installed at <kernelSpecPath>")
  else error
    Installer->>Installer: log("Kernel install failed")  %% failure handling unchanged
  end
Loading

Possibly related PRs

Suggested reviewers

  • m1so

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 clearly summarizes the primary change of installing the Jupyter kernel spec into the virtual environment rather than the user directory, directly reflecting the pull request’s core fix in a concise and specific manner.
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 c0f19d6 and 42007bd.

📒 Files selected for processing (2)
  • CLAUDE.md (1 hunks)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3 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
🧠 Learnings (3)
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/**/{fileSystem.ts,fileSystem.node.ts,platformService.web.ts,platformService.node.ts} : Use URI-based file operations and vscode-path for cross-platform path handling in file system and platform service implementations

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 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:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-09-25T09:58:15.799Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:37-38
Timestamp: 2025-09-25T09:58:15.799Z
Learning: In Deepnote environments, the virtual environment must be created at ~/venv path because deepnote-toolkit expects this specific location.

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
src/platform/logging/index.ts (1)
  • logger (35-48)
🔇 Additional comments (3)
CLAUDE.md (1)

4-4: LGTM! Documentation aligns with implementation.

This guideline matches the actual code changes in the PR and promotes cross-platform path handling.

src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)

70-80: Race condition handling looks solid.

The wait-and-retry logic correctly prevents concurrent installations and handles failures gracefully.


215-226: Core fix implemented correctly.

Switching from --user to --prefix with the venv path ensures Jupyter discovers the venv-specific kernel spec. This resolves the ModuleNotFoundError issue described in the PR objectives.


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: 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 b2649a7 and c0f19d6.

📒 Files selected for processing (1)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3 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
🧠 Learnings (2)
📚 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:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-09-25T09:58:15.799Z
Learnt from: hc2p
PR: deepnote/environments#62
File: gpu/Dockerfile:37-38
Timestamp: 2025-09-25T09:58:15.799Z
Learning: In Deepnote environments, the virtual environment must be created at ~/venv path because deepnote-toolkit expects this specific location.

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
src/platform/logging/index.ts (1)
  • logger (35-48)
⏰ 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). (2)
  • GitHub Check: Build & Test
  • GitHub Check: Lint & Format
🔇 Additional comments (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)

215-227: Change correctly targets venv for kernel spec installation.

The switch from --user to --prefix ensures Jupyter discovers the kernel spec when started with the venv's Python, resolving the ModuleNotFoundError issue described in the PR.

const venvPath = this.getVenvPath(deepnoteFileUri);
const venvKey = venvPath.fsPath;

logger.info(`Virtual environment at ${venvKey}.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Clarify the log message.

"Virtual environment at ${venvKey}." doesn't convey intent. Consider "Using virtual environment at ${venvKey}" or "Installing to virtual environment at ${venvKey}".

🤖 Prompt for AI Agents
In src/kernels/deepnote/deepnoteToolkitInstaller.node.ts around line 68, the log
message "Virtual environment at ${venvKey}." is ambiguous; change it to a
clearer intentful message such as "Using virtual environment at ${venvKey}" or
"Installing to virtual environment at ${venvKey}" depending on context so
callers understand whether the venv is being used or written to; update the
logger.info call to include the chosen phrase and keep the venvKey
interpolation.

@Artmann Artmann marked this pull request as ready for review October 9, 2025 11:17
@Artmann Artmann requested a review from FilipPyrek October 9, 2025 11:17
@Artmann Artmann merged commit 8f22894 into main Oct 9, 2025
4 checks passed
@Artmann Artmann deleted the chris/fix-module-import branch October 9, 2025 11:20
Artmann added a commit that referenced this pull request Oct 13, 2025
* fix: Install kernel spec into venv instead of user directory

Previously, ipykernel specs were installed with --user flag, placing them
in ~/Library/Jupyter/kernels/. This caused the Deepnote Jupyter server to
fall back to generic Python kernels because it couldn't discover the
venv-specific kernel spec.

This resulted in a mismatch where:
- Shell commands (!pip install) used the venv's Python (via PATH)
- But the kernel interpreter used system Python
- Leading to ModuleNotFoundError for packages installed in the venv

Now installing kernel specs with --prefix into the venv itself at
<venv>/share/jupyter/kernels/. Jupyter automatically discovers kernel
specs in this location when started with the venv's Python, ensuring
the correct interpreter is used.

Each .deepnote file maintains its own isolated venv with its own kernel
spec, preventing conflicts when multiple notebooks are open.
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