Skip to content

Conversation

@tkislan
Copy link
Member

@tkislan tkislan commented Oct 31, 2025

Reverts #52

Summary by CodeRabbit

  • New Features

    • Added orphan process cleanup mechanism with lock-file based session tracking for improved server lifecycle management.
  • Refactor

    • Migrated server management from environment-based to per-notebook model for streamlined kernel selection.
    • Removed Deepnote environments UI view, environment management commands, and persistent environment storage.
  • Style

    • Updated output channel naming from "Deepnote" to "Jupyter."
    • Simplified server information display to single port value.
  • Documentation

    • Removed comprehensive debugging guide for kernel management.

@tkislan tkislan requested a review from a team as a code owner October 31, 2025 14:24
@linear
Copy link

linear bot commented Oct 31, 2025

Issue reopened: GRN-4913 Manage Deepnote kernels

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

This PR refactors the Deepnote kernel extension from an environment-centric model to a file-based server orchestration model. It removes the entire environments management UI (views, commands, storage, picker) and replaces per-environment server tracking with per-notebook-file tracking. Key additions include a lock-file-based orphan process cleanup mechanism for multi-window safety, simplified API signatures centered on file URIs rather than environment IDs, and consolidated port handling in server metadata. Configuration and localization entries for environments management are also removed.

Sequence Diagram(s)

sequenceDiagram
    participant NB as Notebook Opening
    participant KAS as DeepnoteKernelAutoSelector
    participant TI as ToolkitInstaller
    participant SS as ServerStarter
    participant KC as KernelController

    rect rgb(220, 240, 220)
    Note over NB,KC: New: File-Based Server Workflow
    NB->>KAS: ensureKernelSelected(notebook)
    KAS->>TI: ensureInstalled(interpreter, deepnoteFileUri)
    TI->>TI: Create venv, install toolkit
    TI-->>KAS: PythonEnvironment
    KAS->>SS: getOrStartServer(interpreter, deepnoteFileUri)
    SS->>SS: Write lock file, start server
    SS-->>KAS: DeepnoteServerInfo (single port)
    KAS->>KAS: Enumerate kernel specs from server
    KAS->>KC: Create & register controller
    KC-->>NB: Ready
    end

    rect rgb(240, 220, 220)
    Note over SS: Orphan Cleanup (startup)
    SS->>SS: Enumerate candidate PIDs
    alt Lock file exists
        SS->>SS: Check session ID match
        SS->>SS: SKIP if active
    else No lock file
        SS->>SS: Check parent process
        SS->>SS: KILL if orphaned
    end
    end
Loading

Possibly related PRs

Suggested reviewers

  • Artmann
  • andyjakubowski

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between dee6066 and 0360ee3.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (35)
  • .vscode/launch.json (1 hunks)
  • CLAUDE.md (1 hunks)
  • DEBUGGING_KERNEL_MANAGEMENT.md (0 hunks)
  • ORPHAN_PROCESS_CLEANUP_IMPLEMENTATION.md (1 hunks)
  • cspell.json (0 hunks)
  • package.json (3 hunks)
  • package.nls.json (1 hunks)
  • src/kernels/deepnote/deepnoteServerProvider.node.ts (1 hunks)
  • src/kernels/deepnote/deepnoteServerStarter.node.ts (16 hunks)
  • src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts (5 hunks)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (14 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentUi.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (0 hunks)
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts (0 hunks)
  • src/kernels/deepnote/types.ts (3 hunks)
  • src/kernels/helpers.ts (0 hunks)
  • src/notebooks/controllers/vscodeNotebookController.ts (0 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (7 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (0 hunks)
  • src/notebooks/serviceRegistry.node.ts (1 hunks)
  • src/platform/common/utils/localize.ts (2 hunks)
  • src/platform/logging/index.ts (0 hunks)

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

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72%. Comparing base (dee6066) to head (0360ee3).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #151     +/-   ##
=======================================
  Coverage     72%     72%             
=======================================
  Files        568     548     -20     
  Lines      45226   43061   -2165     
  Branches    5450    5252    -198     
=======================================
- Hits       32827   31337   -1490     
+ Misses     10610    9984    -626     
+ Partials    1789    1740     -49     
Files with missing lines Coverage Δ
src/kernels/helpers.ts 61% <ø> (+<1%) ⬆️
.../notebooks/controllers/vscodeNotebookController.ts 36% <ø> (+1%) ⬆️
src/platform/common/utils/localize.ts 88% <100%> (ø)
src/platform/logging/index.ts 58% <ø> (+<1%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@saltenasl saltenasl enabled auto-merge (squash) October 31, 2025 14:26
@saltenasl saltenasl disabled auto-merge October 31, 2025 14:26
@saltenasl saltenasl merged commit c6640a4 into main Oct 31, 2025
12 of 13 checks passed
@saltenasl saltenasl deleted the revert-52-hannes/grn-4913-manage-deepnote-kernels branch October 31, 2025 14:27
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