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

add hook to observe pending sessions #751

Merged
merged 8 commits into from
Mar 29, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Mar 23, 2022

In the case where a (new) session has been created and a new kernel is launched, there can be time gap between when a kernel is actually started and its session is actually saved—specifically, if ._finish_kernel_start() takes any time at all. The same is true for deleting sessions; shutdown_kernel can take some time. I call this a "pending session".

I've been working on some tooling to synchronize sessions and kernels (more on that coming in a future PR). To achieve this, I need to be "aware" of pending sessions. Unfortunately, this isn't easily achievable without adding the plumbing directly to source code, hence this pull request.

Here, I've added a "KernelSessionRecordList". This is a thread-safe list of kernels started by the SessionManager. The items in this list can be searched by their kernel_id or session_id. Two records are equal if either of these two keys match each other. When updating this list, we check if the record already exists. If so, we update the value, rather than append.

When create_session is first called, a temporary KernelSessionRecord is added to the _pending_sessions attribute. Once the session is saved, the KernelSessionRecord is removed from the list. The same is true for delete_session.

The unit tests I've added are a good demonstration of how the ._pending_sessions attribute works.

I've opened this PR in draft state for discussion at our next meeting. I'm hoping to open another PR to show my use-case and justify merging. This is an additive feature that won't affect most users, so the risk of merging is fairly low.

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

Codecov Report

Merging #751 (13a470d) into main (64c0730) will increase coverage by 0.12%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   70.23%   70.36%   +0.12%     
==========================================
  Files          62       62              
  Lines        7486     7558      +72     
  Branches     1221     1243      +22     
==========================================
+ Hits         5258     5318      +60     
- Misses       1855     1858       +3     
- Partials      373      382       +9     
Impacted Files Coverage Δ
jupyter_server/services/sessions/sessionmanager.py 88.26% <83.33%> (-2.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64c0730...13a470d. Read the comment docs.

@Zsailer Zsailer marked this pull request as draft March 23, 2022 23:10
):
raise KernelSessionRecordConflict(
"A single session_id can only have one kernel_id "
"associated with. These two KernelSessionRecords share the same "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"associated with. These two KernelSessionRecords share the same "
"associated with it. These two KernelSessionRecords share the same "

@Zsailer
Copy link
Member Author

Zsailer commented Mar 24, 2022

See #752 to see why this PR might be useful.

@blink1073
Copy link
Contributor

Sorry, this picked up merge conflicts

@Zsailer
Copy link
Member Author

Zsailer commented Mar 29, 2022

Thanks, @blink1073! I think this is ready for a final review.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit d32b887 into jupyter-server:main Mar 29, 2022
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

This looks good @Zsailer. I had one comment regarding the record's update method.

I guess I'm not following what is going to act on pending sessions other than perhaps the synchronization POC and why this isn't included in that effort? Are there other areas in which something will be asking if a given session is "pending" or that pending sessions exist? Not a big deal - just trying to understand what else is going to use this.

@Zsailer
Copy link
Member Author

Zsailer commented Mar 30, 2022

I guess I'm not following what is going to act on pending sessions other than perhaps the synchronization POC and why this isn't included in that effort? Are there other areas in which something will be asking if a given session is "pending" or that pending sessions exist? Not a big deal - just trying to understand what else is going to use this.

Good question. You're right, my main reason for this PR was to be used by the synchronization POC.

So why did I split this work into a separate PR?

Well, I can see a case for why the synchronizer should actually be a server extension and not live in core Jupyter Server. It doesn't actually require any changes to core, and it likely won't be used by most users.

The changes in this PR, however, add plumbing to the Session Manager necessary if an extension developer needs to know about pending sessions (much like our efforts to expose APIs for pending kernels). Clients of Jupyter Server cannot know if a session is pending (because of a slow starting/stopping kernel) without inline changes in some of the SessionManager methods.

I can't think of another immediate use case for this outside the synchronizer, but given that slow starting kernels are becoming a more common scenario around Jupyter Server's, I believe that having a hook available to developers to know when slow starting kernels cause slow starting sessions is advantageous in the long run.

What do you think?

@kevin-bates
Copy link
Member

Thanks, the changes are minimal enough within the SessionManager to see how they unfold and its been merged anyway.

@Zsailer Zsailer deleted the pending-sessions branch January 16, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants