-
Notifications
You must be signed in to change notification settings - Fork 284
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
Kernel Provisioning - initial implementation #612
Conversation
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.
A few minor comments, but it's looking clean @kevin-bates .
jupyter_client/provisioning.py
Outdated
"""Launch the kernel process returning the class instance and connection info.""" | ||
pass | ||
|
||
async def cleanup(self, restart=False) -> None: |
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.
Should we force this to be abstract 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.
I'm okay with that. Since the default implementation didn't need this (although the connection file management is a concern that could change that), I didn't want to force its (potentially unnecessary) implementation. That said, cleanup()
is something generally expected - with the act of nothing to do being the exception.
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.
I'm fine either way. I think forcing implementers to consider what cleanup they need isn't the worst overhead even if it's a pass
implementation but my opinion there isn't a strong one.
Let me know if I am missing other issues that need attention. I have less time available so I am missing some threads here and there |
Thanks Matt - I really appreciate your reviews thus far. |
This enables the ability to specify provisioner traits on command line or config file and override via kernelspec config stanza.
Fix typo. Co-authored-by: David Brochart <[email protected]>
Thank you @blink1073 and @davidbrochart for your reviews. |
I just merged #645, looks like you have conflicts now, but that should not be a big deal. Sorry Kevin! |
No worries at all David. I think we should get at least another approval before merging this anyway. I'll try to resolve the conflict later today. |
Let's plan to merge this by next Thursday (22 July) if we don't get any more feedback. @MSeal you had some early feedback if you'd like to take a final look. |
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 great @kevin-bates!
I added one minor comment in line. The PR is going to need a rebase before we can merge. Otherwise, I think it's ready to go!
It looks like this PR encountered an updated version of |
interactions still occur via the ``KernelManager`` and ``KernelClient`` | ||
classes within ``jupyter_client`` and potentially subclassed by the | ||
application. | ||
|
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.
Does the KernelManager or KernelClient API changes (additional (optional) methods, method signature change...) ? Is there any impact for current consumers of those APIs?
Maybe worth stating this in the documentation?
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.
Nothing changed on KernelClient
. As for KernelManager
there were some changes to "internal" methods and I agree those kinds of changes should be documented (primarily calls to the Popen instance are replaced with calls to the provisioner instance). As a result, only KernelManager
subclass authors should be affected (and hopefully minimally). However, I would suggest those be part of the 7.0 release docs (or whatever major release this falls in) rather than embedded in functional/dev docs pertaining to Kernel Provisioning itself.
to use the YARN REST API. Or, similarly, a Kubernetes-based provisioner | ||
would need to implement the process-control methods using the Kubernetes client | ||
API, etc. | ||
|
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.
Are thos poll
, wait
... lifecycle methods enforced by the API? Here also, would be good to have a list (or link) to the lifecycle methods (if the number is not too high and does not break the reading flow).
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.
Again, it's not clear to me what you mean - an example will help me. I also don't understand what you mean by enforced by the API. Sorry.
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.
I should look at the code but did not. My question is "Do all remote provisioners need to implement poll, wait...?". Another way to ask is "Are pool, wait... abstract methods that need to be implemented by all remote provisioners?
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.
Ah - yes, abstract methods are used where "enforcement" is required. These methods are documented on KernelProvisionerBase
and in the API portion of the provisioner docs.
Left a few comments/questions with the future idea to embed a schema for the config as to have the ability for the user to override the config. I see 3 approvers already. I don't want to block the merge and my questions/ideas can for sure be discussed in subsequent PRs. Feel free to merge, but really interested to get feedback on your comments. |
I really appreciate your review Eric - thank you. I've left you with a couple of questions to clarify my understanding regarding your comments. |
Feeling good with that warning 👍
|
As further experiments, I have poc on top of this PR what it would be for JupyterLab to get a json schema baked in the kernelspec, display a form pre-populated with the default values shipped in the schema, send the user values to the kernel service that replaces the placholders defined in the kernelspec with the user values to launch the kernel with parameters. It is doable, we just need to review the changes (not so much to be honest). I hope we can discuss this at the next server meeting. cc/ @kevin-bates @Zsailer @blink1073 @mlucool @goanpeca @ericdatakelly @Carreau |
Huge thanks to @kevin-bates for all the work you did here! |
Ready for preview: https://pypi.org/project/jupyter-client/7.0.0a1/ |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/errors-with-asyncio-io-loop-in-custom-ipython-kernel/10908/2 |
Here are initial changes to introduce Kernel Provisioning (aka, Environment Provisioning). By default, all kernel launches will use the
LocalProvisioner
unless a differentprovisioner_name
is specified in the kernel specification'smetadata.kernel_provisioner
stanza. TheLocalProvisioner
should yield behavior equivalent to today's kernel lifecycle management. As a result, we should see zero changes to the existing tests (other than changes needed for continuing migration topytest
). Any changes to existing tests must be justified so as to preserve compatibility - few exceptions are expected.Provisioners are registered via the entrypoints framework and discovered via the group name
jupyter_client.kernel_provisioners
. When kernelspecs are located, each is checked for aprovisioner_name
entry. If found, the provisioner is loaded usingentrypoints
. If the load fails, a warning message is logged indicating the provisioner package is not available and that kernel specification is dropped from the results (if usingget_all_specs()
) or results in aNoSuchKernel
exception (if usingget_kernel_spec(name)
). If noprovisioner_name
is found (which will constitute nearly 100% of the cases initially), theLocalProvisioner
is instantiated as if it had been configured (per the first paragraph).A singleton is used as a kernel provisioner factory and cache. A map of provisioner name to
EntryPoint
instance is built at startup and maintained during the process's lifetime whenever the kernelspec manager encounters a provisioner name that is not in the map (and that entry point load succeeds).This pull request will remain in DRAFT mode until the following has been addressed:
KernelManager
, but the point at which they are set will need to change. Since the provisioner should return the connection file information - which is also consistent with the behavior described in Kernel handshaking pattern proposal. Also,write_connection_file()
is called before launch (which makes sense for local-only and non-handshaking) but this should be moved closer to the actual launch. (Utilizedself.parent
(ie., kernel_manager) to write local connection file in local-provisioner for now.)kernel_cmd
trait be removed as its been deprecated since 4.0 (April 2014)? Its removal will resolve one potential (and minor) backwards compatibility issue for any applications using that trait. (SeeTODO: Potential B/C issue...
in comment inEnvironmentProvisionerBase.pre_launch()
.) (Removed in [Release 7.0] Remove deprecations in kernel manager #643)Implements/Resolves #608