feat(toolkit): Simplify deepnote toolkit kernel management, to not use deepnote environments#376
feat(toolkit): Simplify deepnote toolkit kernel management, to not use deepnote environments#376
Conversation
…e deepnote environments
- Updated DeepnoteServerStarter to improve context management and error handling during server startup. - Refactored cancellation token handling to ensure proper disposal and prevent memory leaks. - Enhanced logging for notebook closure to include cleanup of associated metadata. - Added unit tests for controller unselection logic to ensure correct behavior with Deepnote kernels.
- Enhanced the logic for clearing notebook controllers to ensure only tracked controllers are unselected. - Updated the `clearControllerForEnvironment` method to clean up associated metadata correctly. - Added unit tests to verify the behavior of environment configuration and controller unselection for Deepnote kernels. - Ensured that the system correctly handles cases where the active interpreter differs from the cached interpreter.
📝 WalkthroughWalkthroughThe PR refactors Deepnote server management from environment-based tracking to interpreter-based tracking. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #376 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/deepnoteServerStarter.node.ts (1)
397-401: 🧹 Nitpick | 🔵 TrivialParameter name mismatch.
Parameter is
environmentIdbut caller passesinterpreterId. Rename for clarity.Rename suggestion
private async gatherSqlIntegrationEnvVars( deepnoteFileUri: Uri, - environmentId: string, + interpreterId: string, token?: CancellationToken ): Promise<Record<string, string>> {Also update the log at line 412.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 397 - 401, Rename the parameter environmentId to interpreterId in the method gatherSqlIntegrationEnvVars(deepnoteFileUri: Uri, environmentId: string, token?: CancellationToken) to match callers, update all references inside that function to use interpreterId, and adjust the log invocation that mentions the environment id (the log near the top of gatherSqlIntegrationEnvVars) to reference interpreterId instead; also update any call sites that pass interpreterId to this function to match the new parameter name if they rely on named arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/interpreter/installer/pipInstaller.node.ts`:
- Around line 18-19: The import of DEEPNOTE_TOOLKIT_VERSION in
pipInstaller.node.ts violates layering; move the DEEPNOTE_TOOLKIT_VERSION
constant out of the kernels layer into a platform-level constants file (e.g.,
create or update src/platform/interpreter/installer/constants.ts to export
DEEPNOTE_TOOLKIT_VERSION), then update the import in pipInstaller.node.ts to
import DEEPNOTE_TOOLKIT_VERSION from that new installer constants module so the
platform layer no longer depends on kernels.
---
Outside diff comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 397-401: Rename the parameter environmentId to interpreterId in
the method gatherSqlIntegrationEnvVars(deepnoteFileUri: Uri, environmentId:
string, token?: CancellationToken) to match callers, update all references
inside that function to use interpreterId, and adjust the log invocation that
mentions the environment id (the log near the top of
gatherSqlIntegrationEnvVars) to reference interpreterId instead; also update any
call sites that pass interpreterId to this function to match the new parameter
name if they rely on named arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d77ef41f-6739-4534-b695-1cdfc5c8abc0
📒 Files selected for processing (12)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.tssrc/kernels/deepnote/types.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/platform/interpreter/installer/pipInstaller.node.tssrc/platform/interpreter/installer/productInstaller.node.tssrc/platform/interpreter/installer/productInstaller.unit.test.tssrc/platform/interpreter/installer/productNames.tssrc/platform/interpreter/installer/productService.node.tssrc/platform/interpreter/installer/types.tssrc/platform/interpreter/installer/utils.ts
| import { DEEPNOTE_TOOLKIT_VERSION } from '../../../kernels/deepnote/types'; | ||
|
|
There was a problem hiding this comment.
Restricted import: platform layer importing from kernels layer.
Static analysis correctly flags this. DEEPNOTE_TOOLKIT_VERSION should live in platform (e.g., src/platform/interpreter/installer/constants.ts) or a shared location to avoid the dependency inversion.
Proposed fix
Move DEEPNOTE_TOOLKIT_VERSION to a platform-level constants file:
-import { DEEPNOTE_TOOLKIT_VERSION } from '../../../kernels/deepnote/types';
+import { DEEPNOTE_TOOLKIT_VERSION } from './constants';Then create/update a constants file in the installer directory with the version export.
🧰 Tools
🪛 GitHub Check: Lint & Format
[failure] 18-18:
Unexpected path "../../../kernels/deepnote/types" imported in restricted zone. Importing non-platform modules into platform files is not allowed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/interpreter/installer/pipInstaller.node.ts` around lines 18 -
19, The import of DEEPNOTE_TOOLKIT_VERSION in pipInstaller.node.ts violates
layering; move the DEEPNOTE_TOOLKIT_VERSION constant out of the kernels layer
into a platform-level constants file (e.g., create or update
src/platform/interpreter/installer/constants.ts to export
DEEPNOTE_TOOLKIT_VERSION), then update the import in pipInstaller.node.ts to
import DEEPNOTE_TOOLKIT_VERSION from that new installer constants module so the
platform layer no longer depends on kernels.
clearControllerForEnvironmentmethod to clean up associated metadata correctly.Summary by CodeRabbit
New Features
Bug Fixes
Documentation