Skip to content

Improve kernel supervisor wrapper to make environment more predictable and configurable #7797

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jmcphers
Copy link
Collaborator

This change updates the kernel supervisor to 0.1.40, which, together with the Positron changes in this PR, addresses a number of related issues around the supervisor's lifecycle and environment.

The changes are as follows:

  • The supervisor is itself run in a login shell, and it can start kernels in login shells too, so that each kernel start (or restart) runs login scripts and picks up changes to e.g. .bash_profile. This behavior does have a minor impact on startup time, so I've made it optional.
  • The supervisor wrapper script now invokes nohup instead of the other way around. This inversion allows us to better control the behavior of nohup and prevent it from littering the project with nohup.out files.
  • The supervisor can now adjust its shutdown timeout on the fly. This allows the shutdown timeout setting to work correctly in the Remote SSH environment, and also makes this setting take effect immediately instead of requiring a restart.
  • The supervisor is now smarter about environment variables on Windows, addressing an issue that caused PATH to (non deterministically) be incorrect in R and Python sessions.

Addresses #6757
Addresses #7215
Addresses #7274
Addresses #7537

Release Notes

New Features

Bug Fixes

QA Notes

Note that the supervisor and kernels are run in login shells, not interactive shells. This means that variables from .bashrc may not be visible; use e.g. .bash_profile instead. The system will attempt to use your default shell, which is zsh on recent macOS, so make sure you edit the profile script that matches your $SHELL.

Copy link

github-actions bot commented May 21, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical

readme  valid tags

@jmcphers jmcphers requested a review from Copilot May 21, 2025 18:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the kernel supervisor to v0.1.40 and makes its environment and lifecycle more configurable and predictable.

  • Introduces a runInShell flag to optionally start supervisor and kernels in login shells
  • Refactors the wrapper script to invoke nohup internally and avoid stray nohup.out files
  • Adds API endpoints to get/set server configuration (e.g. idle shutdown hours) and dynamic timeout updates

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
extensions/positron-supervisor/src/kcclient/model/serverStatus.ts Add version and processId fields to the server status model
extensions/positron-supervisor/src/kcclient/model/serverConfiguration.ts Introduce the ServerConfiguration model
extensions/positron-supervisor/src/kcclient/model/newSession.ts Add runInShell flag to session creation
extensions/positron-supervisor/src/kcclient/model/models.ts & .openapi-generator/FILES Export the new configuration model
extensions/positron-supervisor/src/kcclient/api/defaultApi.ts Add getServerConfiguration and setServerConfiguration methods
extensions/positron-supervisor/src/KallichoreSession.ts Pass runInShell from settings into new sessions
extensions/positron-supervisor/src/KallichoreAdapterApi.ts Hook config changes for idle shutdown, refactor nohup logic, track real PID
extensions/positron-supervisor/resources/supervisor-wrapper.sh Support optional nohup, run in login shell, and cleaned redirection
extensions/positron-supervisor/package.json & package.nls.json Add kernelSupervisor.runInShell setting and bump kallichore version
Comments suppressed due to low confidence (1)

extensions/positron-supervisor/src/kcclient/api/defaultApi.ts:452

  • New methods getServerConfiguration and setServerConfiguration are added but there are no corresponding unit or integration tests. Adding tests will ensure these endpoints work as expected.
public async getServerConfiguration (options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.IncomingMessage; body: ServerConfiguration;  }> {

@jmcphers jmcphers requested a review from sharon-wang May 21, 2025 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant