-
Notifications
You must be signed in to change notification settings - Fork 308
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
6267330
to
c45ee44
Compare
): | ||
raise KernelSessionRecordConflict( | ||
"A single session_id can only have one kernel_id " | ||
"associated with. These two KernelSessionRecords share the same " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"associated with. These two KernelSessionRecords share the same " | |
"associated with it. These two KernelSessionRecords share the same " |
See #752 to see why this PR might be useful. |
6491f9b
to
2e39b36
Compare
Sorry, this picked up merge conflicts |
2e39b36
to
aded082
Compare
for more information, see https://pre-commit.ci
Thanks, @blink1073! I think this is ready for a final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this 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.
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? |
Thanks, the changes are minimal enough within the |
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
orsession_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 temporaryKernelSessionRecord
is added to the_pending_sessions
attribute. Once the session is saved, the KernelSessionRecord is removed from the list. The same is true fordelete_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.