fix: Prompt the user to select an environment when running one or more blocks.#310
Merged
fix: Prompt the user to select an environment when running one or more blocks.#310
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Deepnote pre-execution environment configuration flow and expands IDeepnoteKernelAutoSelector with three public methods: ensureEnvironmentConfiguredBeforeExecution(notebook, token): Promise, clearControllerForEnvironment(notebook, environmentId): void, and handleKernelSelectionError(error, notebook): Promise. VSCodeNotebookController now invokes ensureEnvironmentConfiguredBeforeExecution (using a CancellationTokenSource) for DEEPNOTE notebooks and aborts execution when it returns false. DeepnoteKernelAutoSelector replaces a single global loading controller with per-notebook placeholder controllers, shows an environment picker when needed, persists selection, disposes placeholders on real controller takeover or notebook close, and delegates execution to the real kernel. Sequence Diagram(s)sequenceDiagram
actor User
participant VS as VSCodeNotebookController
participant Selector as DeepnoteKernelAutoSelector
participant Placeholder as PlaceholderController
participant Picker as EnvironmentPicker
participant Real as RealKernelController
User->>VS: Request execute cells
VS->>Selector: ensureEnvironmentConfiguredBeforeExecution(notebook, token)
alt environment not configured
Selector->>Placeholder: createPlaceholderController()
Selector->>Picker: showEnvironmentPicker()
Picker-->>User: display UI
User->>Picker: select / cancel
alt user selects
Picker->>Selector: persist selection
Selector->>Real: createOrRetrieveKernelForEnv()
Selector->>Placeholder: dispose()
Selector-->>VS: return true
else user cancels
Picker->>Selector: cancel
Selector-->>VS: return false
end
else environment configured
Selector->>Real: retrieveExistingKernel()
Selector-->>VS: return true
end
opt execution proceeds
VS->>Real: execute cells
Real-->>User: execution results
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/kernels/deepnote/types.ts`:
- Around line 224-233: The interface IDeepnoteKernelAutoSelector has public
members out of the required alphabetical order; move the method
ensureEnvironmentConfiguredBeforeExecution(notebook: vscode.NotebookDocument,
token: vscode.CancellationToken): Promise<boolean> into the correct alphabetical
position among the other public members of IDeepnoteKernelAutoSelector so that
all public methods/properties are ordered alphabetically by name (preserving
accessibility), updating only ordering and not signatures or comments.
In `@src/notebooks/controllers/vscodeNotebookController.ts`:
- Around line 441-459: The inline new CancellationTokenSource created for the
Deepnote flow is never disposed, risking listener leaks; update the block in
vscodeNotebookController.ts so you create a local variable (e.g., const cts =
new CancellationTokenSource()), pass cts.token into
IDeepnoteKernelAutoSelector.ensureEnvironmentConfiguredBeforeExecution(notebook,
cts.token), and wrap the await call and subsequent hasEnvironment check in a
try/finally that calls cts.dispose() in finally to ensure cleanup when using
DEEPNOTE_NOTEBOOK_TYPE.
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 830-853: The code contains several early returns with no
separating blank line (e.g. after the environment existence branch and after
checking selectedEnvironment) — edit the method containing
existingEnvironmentId, environmentManager.getEnvironment,
notebookEnvironmentMapper.removeEnvironmentForNotebook,
Cancellation.throwIfCanceled, pickEnvironment and selectedEnvironment to insert
a blank line immediately before each return statement to match the style guide
(add a blank line before the returns that follow the logger.info(...) calls and
after the stale-mapping removal branch).
- Around line 82-84: The private field placeholderControllers (Map<string,
NotebookController>) is out of alphabetical order; move its declaration
(including the leading comment) so that all private fields in
deepnoteKernelAutoSelector.node.ts are sorted alphabetically by name while
preserving accessibility and type, e.g., place placeholderControllers in the
correct spot among other private readonly fields; ensure no other field ordering
or visibility is changed and run lint/format to validate.
- Around line 861-878: Wrap the window.withProgress(...) call in a try/catch
inside the method so user cancellation doesn't throw; specifically call
withProgress around the current await
this.ensureKernelSelectedWithConfiguration(...) invocation and in the catch
detect cancellation by checking the method's incoming CancellationToken (token)
or the caught error for cancellation (or progressToken.isCancellationRequested)
and return false; ensure the method still returns the normal boolean on success
and only returns false on cancellation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 927-947: The retry loop in findNotebookEditor uses magic numbers
(10 retries and 100ms delay); extract them to clearly named constants (e.g.,
NOTEBOOK_EDITOR_RETRY_COUNT and NOTEBOOK_EDITOR_RETRY_DELAY_MS) near the top of
the module and replace the literal 10 and 100 with those constants, keeping the
existing behavior and preserving the current await/new Promise sleep logic
inside the for loop.
- Around line 1156-1158: The loop that calls kernelExecution.executeCell over
cells should handle per-cell errors so one failing cell doesn't stop the rest;
update the for (const cell of cells) { await kernelExecution.executeCell(cell);
} loop in deepnoteKernelAutoSelector.node.ts to wrap each await
kernelExecution.executeCell(cell) in a try/catch, mirror the behavior in
VSCodeNotebookController.executeQueuedCells (lines ~696-733) by catching errors
per cell, logging/reporting the error (use the same logger or cell.status/error
reporting used elsewhere) and continuing to the next cell; ensure any resources
or cell-specific cleanup still run inside a finally block if needed.
- Around line 452-500: The duplicated block that creates a placeholder
controller, sets its affinity, finds the NotebookEditor and calls
commands.executeCommand('notebook.selectKernel') (currently repeated after
createPlaceholderController and after
notebookEnvironmentMapper.removeEnvironmentForNotebook) should be extracted into
a helper method — e.g. add an async method selectPlaceholderController(notebook:
NotebookDocument) that calls this.createPlaceholderController(notebook),
placeholder.updateNotebookAffinity(...), this.findNotebookEditor(notebook) and
runs the notebook.selectKernel command (with id: placeholder.id and extension:
JVSC_EXTENSION_ID) while logging the same warning when no editor is found; then
replace both duplicated blocks with await
this.selectPlaceholderController(notebook).
- Extract selectPlaceholderController helper to eliminate duplicate code - Add NOTEBOOK_EDITOR_RETRY_COUNT and NOTEBOOK_EDITOR_RETRY_DELAY_MS constants - Add per-cell error handling in placeholder controller execute handler
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 1116-1119: The CancellationTokenSource named `cts` in the block
around the call to `ensureEnvironmentConfiguredBeforeExecution(doc, cts.token)`
is never cancelled externally and adds noise; replace it by passing
`CancellationToken.None` (or remove `cts` entirely) when calling
`ensureEnvironmentConfiguredBeforeExecution` (or, if there is a deliberate
reason to keep `cts`, add a comment explaining why it's required) so the call
becomes `ensureEnvironmentConfiguredBeforeExecution(doc,
CancellationToken.None)` and eliminates the unused `cts` object.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 795-823: The current branch returns true when an environment
exists but its kernel/controller might be missing; update the logic in
deepnoteKernelAutoSelector.node.ts so after fetching existingEnvironmentId and
environment via notebookEnvironmentMapper.getEnvironmentForNotebook and
environmentManager.getEnvironment you also verify the corresponding controller
exists (e.g., via a method like getControllerForEnvironment or kernel/controller
lookup used elsewhere in this class); if the controller is missing, log the
situation, remove the stale mapping with
notebookEnvironmentMapper.removeEnvironmentForNotebook(baseFileUri) (or trigger
re-run of setup) and continue to the picker flow instead of returning true so
setup can be retried.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 785-907: The method ensureEnvironmentConfiguredBeforeExecution can
return true even if ensureKernelSelectedWithConfiguration returned early without
creating a controller; after each await to ensureKernelSelectedWithConfiguration
(both in the existing-environment branch and the selectedEnvironment branch)
check this.notebookControllers.get(notebookKey) to confirm a controller was
created, log a clear message (e.g., using logger.info or logger.warn with
getDisplayPath(notebook.uri) and environment.name), and return false if no
controller exists so we don't falsely report success; keep existing
cancellation/error handling around the ensureKernelSelectedWithConfiguration
awaits and perform the controller existence guard immediately after those
awaits.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 805-867: The early-return when an existingController is found can
skip interpreter-mismatch checks; modify the branch that handles an
existingController (use notebookControllers.get(notebookKey) and environment
from environmentManager.getEnvironment(existingEnvironmentId)) to validate the
controller's interpreter matches the environment (reuse the same
interpreter-path check logic used in ensureKernelSelectedWithConfiguration). If
the interpreter matches, return true as before; if it does not match or is
stale, log the mismatch and proceed to call
ensureKernelSelectedWithConfiguration (the same window.withProgress setup path)
to recreate/repair the controller, then verify
notebookControllers.get(notebookKey) afterwards before returning.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 1184-1189: The current neverCancelledToken in executeHandler
creates a fake CancellationToken that prevents user-initiated aborts; replace it
with a real cancellable token tied to the notebook/cell lifecycle (e.g. use the
executeHandler's incoming CancellationToken if available or create a
CancellationTokenSource and subscribe to the notebook close/cell cancellation
event) so cancellation requests propagate; update references to
neverCancelledToken (and its onCancellationRequested) to use that real token and
dispose/unsubscribe the source when execution completes.
- Around line 820-827: The early-return path that uses
notebookControllers.get(notebookKey) (existingController) should validate the
interpreter before returning: locate the existingController handling in
ensureKernelSelectedWithConfiguration and replicate its interpreter check
(compare existingInterpreter.uri.fsPath to expectedInterpreter.fsPath) when an
existing controller is found; if the interpreter URIs differ, treat it as not
configured (do not return true) so the selection/configuration flow continues
and the kernel can be updated accordingly. Ensure you reference the same
interpreter objects/fields (existingInterpreter.uri.fsPath and
expectedInterpreter.fsPath) and only return true when they match.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 823-848: Duplicate interpreter path verification exists in
deepnoteKernelAutoSelector.node.ts (the block using
existingController.connection.interpreter and comparing to expectedInterpreter),
mirroring logic in ensureKernelSelectedWithConfiguration; extract this into a
helper (e.g., isControllerInterpreterValid(controller, venvPath) or
getVenvInterpreterUri(venvPath)) and replace both occurrences to call the helper
from the existingController check and from
ensureKernelSelectedWithConfiguration, ensuring the helper returns true when no
interpreter is present and compares existingInterpreter.uri.fsPath to the
expected path for validity so the recreate/dispose logic in
setupKernelForEnvironment remains unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #304
Summary by CodeRabbit
New Features
Reliability
Tests
Documentation