Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve restarting behavior of the reticulate session. #5019

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Oct 14, 2024

Addresses #4887 by making sure the correct order of action happens:

  1. Shutdown the Reticulate Python kernel
  2. Restart the R session
  3. Start a new reticulate session

We also added a new dialog when restarting the reticulate kernel, to make sure that the user is aware that the R kernel is also restarting, not just the reticulate kernel.

We can't restart the reticulate kernel without restarting the R session because Reticulate binds to a single Python session only once. If simply restarted Ipykernel, it would still point to the same Python session as before (eg same variables, etc).

@dfalbel dfalbel requested a review from jmcphers October 15, 2024 12:17
extensions/positron-reticulate/src/extension.ts Outdated Show resolved Hide resolved
extensions/positron-reticulate/src/extension.ts Outdated Show resolved Hide resolved
}
});

await this.shutdown(positron.RuntimeExitReason.SwitchRuntime);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be ExitReason.Restart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we send a ExitReason.Restart the Jupyter Adapter will try to reinitialize it as soon as it exitted, but we actually want to wait for the new R session to be started.

if (this._restarting) {
this._kernel.clearSession();
this._restarting = false;
// Defer the start by 500ms to ensure the kernel has processed its
// own exit before we ask it to restart. This also ensures that the
// kernel's status events as it starts up don't overlap with the
// status events emitted during shutdown (which can happen on the
// Positron side due to internal buffering in the extension host
// interface)
setTimeout(() => {
this._kernel.start();
}, 500);
}
}

My reasonming was that this 'looks like' we are effectively switching runtimes, as we don't preserve the session RuntimeSessionObject, etc. Maybe RuntimeExitReason.Shutdown is better?

extensions/positron-reticulate/src/extension.ts Outdated Show resolved Hide resolved
dfalbel and others added 2 commits October 15, 2024 17:36
Co-authored-by: Jonathan <jonathan@rstudio.com>
Signed-off-by: Daniel Falbel <dfalbel@gmail.com>
@dfalbel dfalbel requested a review from jmcphers October 15, 2024 20:37
jmcphers
jmcphers previously approved these changes Oct 15, 2024
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since this does make things better but in my testing I noticed some intermittent issues that my be worth investigating.

First, this happened intermittently on restart:

Failed to initialize and connect to the Reticulate Python session: Cannot invoke 'reticulate_start_kernel'; no UI comm is open.

Secondly, during the restart I sometimes found that R and Python both got stuck "restarting" (according to the console toolbar) even when the restart was finished. Might be a UI bug since eventually you could use the kernels and the next operation cleared the restarting status.

@dfalbel
Copy link
Contributor Author

dfalbel commented Oct 16, 2024

I could reproduce and after some testing I figured I accidentally commited the experiment with setting this to ExitReason.Restart: #5019 (comment)

Should be working great now. I can no longer reproduce both issues.

@dfalbel dfalbel merged commit b17da0a into main Oct 16, 2024
3 checks passed
@dfalbel dfalbel deleted the bugfix/reticulate-restart branch October 16, 2024 17:19
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants