-
Notifications
You must be signed in to change notification settings - Fork 283
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 Jupyter Events and emit events from the Kernel Manager #832
Conversation
This looks like a major change, so hopefully for Jupyter-client 8, right? |
@ccordoba12, yeah, I think that's fair. This adds an additional dependency, jupyter_events, which I think warrants a major release. |
Good call @ccordoba12, I've opened #833 |
kernel_id: | ||
oneOf: | ||
- type: string | ||
- type: "null" |
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 - just have one question.
Is this in anticipation of supporting other actions, because all of the currently defined actions fire with a non-None kernel_id
? If we retained that approach, we could then apply a format or pattern, allowing only UUID formats (and remove the oneOf
and null
). This might allow event consumers to have an easier experience as well.
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.
because all of the currently defined actions fire with a non-None kernel_id?
I don't think start_kernel
or pre_start_kernel
methods strictly require a kernel_id to start, right? This logic seems to suggest that if None
is provided, it creates a uuid on the fly.
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.
That is correct (on both counts; non-required kernel_id and auto-create logic). The provisioners should have access to the kernel-id from the get-go so they can do indexing, etc. as necessary.
49f8240
to
62025bc
Compare
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.
LGTM, thanks!
This requires a release of Jupyter Events...done 👍