Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix issues around comm and session lifecycles #6278

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

jmcphers
Copy link
Collaborator

@jmcphers jmcphers commented Feb 11, 2025

This change addresses a few problems found by @DavisVaughan around session and comm lifecycles. Specifically:

  • Comms were trying to close themselves on disposal even after the kernel has exited, leading to some errors logged since the close requests could not be delivered to the kernel.
  • Runtime sessions dispose methods were never being called. This is an artifact from a time when runtime sessions could be started again after fully exiting. Now, "restarting" an exited runtime actually starts a new session. The dispose() method is implemented in the language pack and does things like clean up log streamers/file handles/sockets/etc.

Addresses #2908
Addresses #3964

While these changes are important for code quality and hygiene, aside from addressing some leaks, there ought to be no observable difference in behavior.

QA Notes

Since one of these issues results in errors being logged, it should be pretty easy to verify. For the other, the main risk here is that the runtime is going to get prematurely disposed -- once it's disposed we no longer get events, logs, anything (it's inert). So no heavy verification needed, but good to check start/stop/start cycles and make sure we're getting good behavior, events, and appropriate logs.

@jmcphers jmcphers requested a review from seeM February 11, 2025 02:21
Copy link

github-actions bot commented Feb 11, 2025

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

readme  valid tags

@jmcphers jmcphers marked this pull request as draft February 11, 2025 02:57
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Code changes LGTM! I'll test the behavior once this comes out of draft.

@jmcphers jmcphers marked this pull request as ready for review February 11, 2025 16:10
@jmcphers jmcphers force-pushed the bugfix/session-comm-disposal branch from 8f7075e to f806183 Compare February 13, 2025 22:57
@jmcphers jmcphers requested a review from seeM February 13, 2025 22:58
@jmcphers
Copy link
Collaborator Author

@seeM should be ready for review now!

Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Looks good! I confirmed locally with the debugger that KallichoreSession.dispose and PythonRuntimeSession.dispose are being called, and there are no longer any console errors when shutting down a session.

@jmcphers jmcphers merged commit 51ecc96 into main Feb 14, 2025
10 checks passed
@jmcphers jmcphers deleted the bugfix/session-comm-disposal branch February 14, 2025 20:03
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2025
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants