Skip to content

Commit 460ba6d

Browse files
hc2phc2p
committed
WIP: Environment switching with controller disposal workaround
This commit implements environment switching but with a known limitation regarding controller disposal and queued cell executions. ## Changes ### Port Allocation Refactoring - Refactored DeepnoteServerProvider to handle both jupyterPort and lspPort - Updated DeepnoteServerStarter to allocate both ports - Updated DeepnoteEnvironmentManager to store complete serverInfo - All port-related code now consistently uses the dual-port model ### Kernel Selection Logic Extraction - Extracted kernel selection logic into public `selectKernelSpec()` method - Added 4 unit tests for kernel selection (all passing) - Method is now testable and validates environment-specific kernel preference - Falls back to generic Python kernels when env-specific kernel not found ### Environment Switching Implementation - Added environment switching via tree view context menu - Switching calls `rebuildController()` to create new controller - Shows warning dialog if cells are currently executing - Updates notebook-to-environment mapping on switch ## Known Issue: Controller Disposal Problem When switching environments, we encountered a critical "notebook controller is DISPOSED" error. The issue occurs because: 1. User queues cell execution (VS Code references current controller) 2. User switches environments 3. New controller created and marked as preferred 4. Old controller disposed 5. Queued execution tries to run 5+ seconds later → DISPOSED error ### Current Workaround (Implemented) We do NOT dispose old controllers at all. Instead: - Old controllers stay alive to handle queued executions - New controller is marked as "Preferred" for new executions - Garbage collection cleans up eventually ### Why This Is Not Ideal - Potential memory leak with many switches - No guarantee new executions use new controller - Users might execute on wrong environment after switch - VS Code controller selection not fully deterministic ### What We Tried 1. Adding delay before disposal → Failed (timing unpredictable) 2. Disposing after setting preference → Failed 3. Never disposing → Prevents error but suboptimal ### Proper Solution Needed May require VS Code API changes to: - Force controller selection immediately - Cancel/migrate queued executions - Query if controller has pending executions See TODO.md for full analysis and discussion. ## Testing - ✅ All 40+ unit tests passing - ✅ Kernel selection tests passing (4 new tests) - ✅ Port allocation working correctly - ⚠️ Environment switching works but with limitations above Related files: - src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:306-315 - src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:599-635 - src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts:542-561
1 parent 8b702c7 commit 460ba6d

9 files changed

+933
-250
lines changed

‎TODO.md‎

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
# Deepnote Kernel Management - TODO
2+
3+
## Current Status
4+
5+
### ✅ Completed Phases
6+
7+
- **Phase 1**: Core Models & Storage
8+
- **Phase 2**: Refactor Existing Services
9+
- **Phase 3**: Tree View UI (with 40+ passing unit tests)
10+
- **Phase 4**: Server Control Commands (completed in Phase 3)
11+
- **Phase 5**: Package Management (completed in Phase 3)
12+
- **Phase 7 Part 1**: Configuration picker infrastructure
13+
14+
### ✅ Completed: Phase 7 Part 2 - Kernel Auto-Selector Integration
15+
16+
**Status**: Integration completed successfully! 🎉
17+
18+
**Implemented**:
19+
1. ✅ Injected `IDeepnoteConfigurationPicker` and `IDeepnoteNotebookConfigurationMapper` into `DeepnoteKernelAutoSelector`
20+
2. ✅ Modified `ensureKernelSelected()` method:
21+
- Checks mapper for existing configuration selection first
22+
- Shows picker if no selection exists or config was deleted
23+
- Saves user's selection to mapper
24+
- Uses selected configuration instead of auto-creating venv
25+
3. ✅ Implemented configuration server lifecycle handling:
26+
- Auto-starts server if configuration exists but not running
27+
- Shows progress notifications during startup
28+
- Updates configuration's lastUsedAt timestamp
29+
4. ✅ Updated connection metadata creation:
30+
- Uses configuration's venv interpreter
31+
- Uses configuration's server info
32+
- Registers server with provider using configuration ID
33+
5. ✅ Edge case handling:
34+
- User cancels picker → falls back to legacy auto-create behavior
35+
- Configuration deleted → shows picker again
36+
- Multiple notebooks can share same configuration
37+
- Graceful error handling throughout
38+
39+
**Implementation Details**:
40+
- Created two helper methods:
41+
- `ensureKernelSelectedWithConfiguration()` - Uses selected configuration
42+
- `ensureKernelSelectedLegacy()` - Fallback to old auto-create behavior
43+
- Configuration selection persists in workspace state
44+
- Backward compatible with existing file-based venv system
45+
- Full TypeScript compilation successful
46+
47+
### ⚠️ Current Challenge: Controller Disposal & Environment Switching
48+
49+
**Issue**: When switching environments, we encountered a "notebook controller is DISPOSED" error that occurs when:
50+
1. User queues cell execution (VS Code stores reference to current controller)
51+
2. User switches environments via tree view
52+
3. New controller is created and set as preferred
53+
4. Old controller is disposed
54+
5. Queued execution tries to run 5+ seconds later → DISPOSED error
55+
56+
**Current Workaround** (implemented but not ideal):
57+
- We do **NOT** dispose old controllers at all
58+
- Old controllers are left alive to handle any queued executions
59+
- New controller is marked as "Preferred" so new executions use it
60+
- Garbage collection cleans up old controllers eventually
61+
62+
**Why This Is Not Ideal**:
63+
- Memory leak potential if many environment switches occur
64+
- No guarantee that new executions will use the new controller
65+
- VS Code's controller selection logic is not fully deterministic
66+
- Users might still execute on old environment after switch
67+
68+
**What We've Tried**:
69+
1. ❌ Adding delay before disposal → Still failed (timing unpredictable)
70+
2. ❌ Disposing after marking new controller as preferred → Still failed
71+
3. ✅ Never disposing old controllers → Prevents error but suboptimal
72+
73+
**Proper Solution Needed**:
74+
- Need a way to force VS Code to use the new controller immediately
75+
- Or need to cancel/migrate queued executions to new controller
76+
- Or need VS Code API to query if controller has pending executions
77+
- May require upstream VS Code API changes
78+
79+
**Related Code**:
80+
- src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:306-315 (non-disposal logic)
81+
- src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:599-635 (extracted selectKernelSpec)
82+
- src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts:542-561 (warning dialog)
83+
84+
**Testing Done**:
85+
- ✅ All 40+ unit tests passing
86+
- ✅ Kernel selection logic extracted and tested
87+
- ✅ Port allocation refactored (both jupyterPort and lspPort)
88+
- ⚠️ Environment switching works but with above limitations
89+
90+
### 🎯 Next: E2E Testing & Validation
91+
92+
**Testing Plan**:
93+
1. **Happy Path**:
94+
- Create config "Python 3.11 Data Science" via tree view
95+
- Add packages: pandas, numpy
96+
- Start server via tree view
97+
- Open test.deepnote
98+
- See configuration picker
99+
- Select config from picker
100+
- Verify kernel starts and cells execute
101+
- Close notebook and reopen
102+
- Verify same config auto-selected (no picker shown)
103+
104+
2. **Multiple Notebooks**:
105+
- Create 2 configs with different Python versions
106+
- Open notebook1.deepnote → select config1
107+
- Open notebook2.deepnote → select config2
108+
- Verify both work independently
109+
110+
3. **Auto-Start Flow**:
111+
- Stop server for a configuration
112+
- Open notebook that uses that config
113+
- Verify server auto-starts before kernel connects
114+
115+
4. **Fallback Flow**:
116+
- Open new notebook
117+
- Cancel configuration picker
118+
- Verify falls back to legacy auto-create behavior
119+
120+
### ⏸️ Deferred Phases
121+
122+
**Phase 6: Detail View** (Optional enhancement)
123+
- Webview panel with configuration details
124+
- Live server logs
125+
- Editable fields
126+
- Action buttons
127+
128+
**Phase 8: Migration & Polish**
129+
- Migrate existing file-based venvs to configurations
130+
- Auto-detect and import old venvs on first run
131+
- UI polish (icons, tooltips, descriptions)
132+
- Keyboard shortcuts
133+
- User documentation
134+
135+
## Implementation Notes
136+
137+
### Current Architecture
138+
139+
```
140+
User creates config → Config stored in globalState
141+
User starts server → Server tracked by config ID
142+
User opens notebook → Picker shows → Selection saved to workspaceState
143+
Notebook uses config → Config's venv & server used
144+
```
145+
146+
### Key Files to Modify
147+
148+
1. `src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts` - Main integration point
149+
2. `src/kernels/deepnote/configurations/deepnoteConfigurationPicker.ts` - Already created ✅
150+
3. `src/kernels/deepnote/configurations/deepnoteNotebookConfigurationMapper.ts` - Already created ✅
151+
152+
### Backward Compatibility
153+
154+
The old auto-create behavior should still work as fallback:
155+
- If user cancels picker → show error or fall back to auto-create
156+
- If no configurations exist → offer to create one
157+
- Consider adding setting: `deepnote.kernel.autoSelect` to enable old behavior
158+
159+
## Testing Strategy
160+
161+
### Manual Testing Steps
162+
163+
1. **Happy Path**:
164+
- Create config "Python 3.11 Data Science"
165+
- Add packages: pandas, numpy
166+
- Start server
167+
- Open test.deepnote
168+
- Select config from picker
169+
- Run cell: `import pandas; print(pandas.__version__)`
170+
- Verify output
171+
172+
2. **Multiple Notebooks**:
173+
- Create 2 configs with different Python versions
174+
- Open notebook1.deepnote → select config1
175+
- Open notebook2.deepnote → select config2
176+
- Verify both work independently
177+
178+
3. **Persistence**:
179+
- Select config for notebook
180+
- Close notebook
181+
- Reopen notebook
182+
- Verify same config auto-selected (no picker shown)
183+
184+
4. **Edge Cases**:
185+
- Delete configuration while notebook open
186+
- Stop server while notebook running
187+
- Select stopped configuration → verify auto-starts
188+
189+
### Unit Tests Needed
190+
191+
- [ ] Test mapper stores and retrieves selections
192+
- [ ] Test picker shows correct configurations
193+
- [ ] Test auto-selector uses mapper before showing picker
194+
- [ ] Test fallback behavior when config not found
195+
196+
## Documentation
197+
198+
Files to update after completion:
199+
- [ ] KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md - Update Phase 7 status to complete
200+
- [ ] README.md - Add usage instructions for kernel configurations
201+
- [ ] CHANGELOG.md - Document new feature
202+
203+
## Future Enhancements
204+
205+
1. **Configuration Templates**: Pre-defined package sets (Data Science, ML, Web Dev)
206+
2. **Configuration Sharing**: Export/import configurations as JSON
207+
3. **Workspace Scoping**: Project-specific configurations
208+
4. **Resource Monitoring**: Show memory/CPU usage in tree
209+
5. **Auto-Cleanup**: Delete unused configurations after X days
210+
6. **Cloud Sync**: Sync configurations across machines

‎src/kernels/deepnote/deepnoteServerProvider.node.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export class DeepnoteServerProvider
8585
for (const [handle, info] of this.servers.entries()) {
8686
servers.push({
8787
id: handle,
88-
label: `Deepnote Toolkit (${info.port})`,
88+
label: `Deepnote Toolkit (jupyter:${info.jupyterPort}, lsp:${info.lspPort})`,
8989
connectionInformation: {
9090
baseUrl: Uri.parse(info.url),
9191
token: info.token || ''

0 commit comments

Comments
 (0)