Skip to content

Commit

Permalink
Move notebook runtime URI fixing to the main-thread. (#4017)
Browse files Browse the repository at this point in the history
Addresses #4016. 

Move the reviving of a URI to `ExtHostLanguageRuntime.$createLanguageRuntimeSession()` when starting a runtime.
Fixes problem where sometimes the URI would get silently blown away by
bad to-string conversion and then the runtime would not know where it
was supposed to be running.

Previously I added a URI "revival" step on the `JupyterKernel` code to deal with the fact that positron notebooks sometimes provide non-fully-formed URIs. Turns out this didn't actually work and just resulted in undefined variables due to the serialized URI-like object not having a`.toString()` method implemented. This caused the notebook to start from the vaunted `[Object object]` path. 
I thought it worked because in that scenario the kernel will still start, just at the project root path. The problems with this are immediately apparent when you try and run a notebook nested in a folder. 

Now we run `URI.revive()` in `ExtHostLanguageRuntime.$createLanguageRuntimeSession()` if there's a `notebookUri` present to convert from the serialized version back to a real URI. 

<!-- Thank you for submitting a pull request.
If this is your first pull request you can find information about
contributing here:
  * https://github.com/posit-dev/positron/blob/main/CONTRIBUTING.md

We recommend synchronizing your branch with the latest changes in the
main branch by either pulling or rebasing.
-->

<!--
  Describe briefly what problem this pull request resolves, or what
  new feature it introduces. Include screenshots of any new or altered
  UI. Link to any GitHub issues but avoid "magic" keywords that will 
  automatically close the issue. If there are any details about your 
approach that are unintuitive or you want to draw attention to, please
  describe them here.
-->

### QA Notes
The normal VSCode notebooks should be able to open resources relative to
themselves. Like a csv with `pd.read_csv()`
  • Loading branch information
nstrayer authored Jul 15, 2024
1 parent 1a48e23 commit a5c8d78
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
10 changes: 1 addition & 9 deletions extensions/jupyter-adapter/src/JupyterKernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,24 +132,16 @@ export class JupyterKernel extends EventEmitter implements vscode.Disposable {
private _receivedInitialStatus = false;
private _receivedInitialReply = false;

/** The URI of the notebook that owns this kernel, if any */
private readonly _notebookUri?: vscode.Uri;

constructor(
private readonly _context: vscode.ExtensionContext,
spec: JupyterKernelSpec,
private readonly _runtimeId: string,
private readonly _channel: vscode.OutputChannel,
readonly notebookUri?: vscode.Uri,
private readonly _notebookUri?: vscode.Uri,
readonly extra?: JupyterKernelExtra,
) {
super();

if (notebookUri) {
// Sometimes this uri comes in only partially formed, so we need to make sure its a full
// uri before we store it.
this._notebookUri = vscode.Uri.parse(notebookUri.toString());
}
this._spec = spec;
this._extra = extra;
this._control = null;
Expand Down
10 changes: 10 additions & 0 deletions src/vs/workbench/api/common/positron/extHostLanguageRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ export class ExtHostLanguageRuntime implements extHostProtocol.ExtHostLanguageRu
sessionMetadata: IRuntimeSessionMetadata): Promise<extHostProtocol.RuntimeInitialState> {
// Look up the session manager responsible for restoring this session
const sessionManager = await this.runtimeManagerForRuntime(runtimeMetadata, true);

if (sessionMetadata.notebookUri) {
// Sometimes the full URI doesn't make it across the serialization boundary.
// By reviving the URI here we make sure we're operating with a full URI
// rather than a serialized one that may be missing parameters.
sessionMetadata = {
...sessionMetadata,
notebookUri: URI.revive(sessionMetadata.notebookUri)
};
}
if (sessionManager) {
const session =
await sessionManager.manager.createSession(runtimeMetadata, sessionMetadata);
Expand Down

0 comments on commit a5c8d78

Please sign in to comment.