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

Enrich Docs with Usage #28

Merged
merged 14 commits into from
Dec 1, 2019

Conversation

echarles
Copy link
Contributor

@echarles echarles commented Nov 5, 2019

This PR enriches the docs with explanations on how kernel_mgmt can be used as standalone or in jupyter_server.

(it also update the env to support markdown)

@codecov-io
Copy link

codecov-io commented Nov 5, 2019

Codecov Report

Merging #28 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #28   +/-   ##
=======================================
  Coverage   71.43%   71.43%           
=======================================
  Files          29       29           
  Lines        2048     2048           
=======================================
  Hits         1463     1463           
  Misses        585      585

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 236d44d...cb88439. Read the comment docs.

Copy link
Collaborator

@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.

Thanks Eric - these additions seem useful. I had a couple comments and I've asked for @takluvyer's review as well.

docs/server.md Outdated Show resolved Hide resolved
@@ -0,0 +1,1933 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

MainKernelSpecHandler will hit KernelFinder prior to any launches (to get the list of available kernels).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx. need to working more on the sequence diagram.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The /api/kernelspecs call hits KernelFinder directly from the kernelspec handler.

docs/standalone.md Outdated Show resolved Hide resolved
@echarles
Copy link
Contributor Author

echarles commented Nov 5, 2019

Thx for the review @kevin-bates. I will inject your comments + draw the interactions in the sequence diagram and will push all that (my) tomorrow.

@takluyver
Copy link
Owner

I would prefer to stick with rst for these docs. I know Markdown is more popular, and it's fine for things like Github comments, but the crosslinking Sphinx can do with rst roles is vastly superior. I also think using the same format across the docs is a good idea.

@echarles
Copy link
Contributor Author

echarles commented Nov 5, 2019

@takluyver Sure, I will upgrade my rst skills and convert tomorrow the markdows. I started with rst but came into sphynx confusions with the links (should be able to overcome that tomorrow).

@echarles
Copy link
Contributor Author

echarles commented Nov 6, 2019

@kevin-bates @takluyver Thx for your reviews. I have pushed an update that take them into account (still need to work on the sequence diagram, will do that tomorrow for final review).

Copy link
Collaborator

@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.

Thanks for the updates. I added a couple more comments for next rev.

docs/server.rst Outdated
'pyimport = jupyter_kernel_mgmt.discovery:IPykernelProvider',
]

External Providers can register their own entypoints, e.g the `kubernetes_kernel_provider <https://github.com/gateway-experiments/kubernetes_kernel_provider>`_ which extends the `remote_kernel_provider <https://github.com/gateway-experiments/remote_kernel_provider>`_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think stating that the kubernetes provider extends remote kernel provider is that pertinent here. The RKP is more of an "abstract base provider". Instead, perhaps adding a link to Thomas' jupyter_ssh_kernels would be useful? @takluyver - are you okay with that? If not, you can reference the yarn kernel provider repo instead.

(What happens here affects the corresponding diagram as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated doc and text
@takluyver ping me if ok to add jupyter_ssh_kernels. Thx

docs/server.rst Outdated Show resolved Hide resolved
docs/standalone.rst Outdated Show resolved Hide resolved

================
Standalone Usage
================
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this section could use more content. Although I'm not entirely aware of how jupyter_client was used outside of Notebook, I know that its used directly. I believe JKM's goal is similar. You might take a look at some of the client test code to get an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added paragraphs for this.

Copy link
Owner

Choose a reason for hiding this comment

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

I may rework this more at a later point. It is meant to be a public Python API as well as a component for Jupyter server.

docs/standalone.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,1933 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The /api/kernelspecs call hits KernelFinder directly from the kernelspec handler.

@echarles
Copy link
Contributor Author

echarles commented Nov 7, 2019

@kevin-bates just pushed your comments + a better sequence diagram

Copy link
Collaborator

@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.

Thanks Eric - the sequence diagram looks great. Just had a couple more comments.

docs/server.rst Outdated Show resolved Hide resolved
docs/server.rst Outdated Show resolved Hide resolved
docs/server.rst Outdated Show resolved Hide resolved
@echarles
Copy link
Contributor Author

echarles commented Nov 7, 2019

@kevin-bates has eagle eyes. Obviously my last push is a copy past of his comments. Thx again!

Copy link
Collaborator

@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.

LGTM - thanks Eric!

@kevin-bates kevin-bates mentioned this pull request Nov 28, 2019

This page describes the way `Jupyter Server <https://github.com/jupyter/jupyter_server>`_ uses the Kernel Management module.

.. image:: ./images/kernel_mgmt_server.svg
Copy link
Owner

Choose a reason for hiding this comment

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

Minor nitpick in this image: the connection between Jupyter server and JKM is labelled "Jupyter Protocol (ZeroMQ)". This isn't quite right - Jupyter Server is using JKM through Python APIs (classes & functions), and through that talks the Jupyter protocol to the kernels themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takluyver great catch. I have updated the diagram to reflect this. There is still room for improvements on that diagram, but I think it is good-enough for now and at least does not show wrong info anymore.

Use with Jupyter Server
=======================

This page describes the way `Jupyter Server <https://github.com/jupyter/jupyter_server>`_ uses the Kernel Management module.
Copy link
Owner

Choose a reason for hiding this comment

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

Ultimately this page probably belongs in the Jupyter Server docs, but if it makes sense to put it here at the moment, I don't have a problem with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will work next week with @Zsailer on the jupyter server doc. If this is merged before, we can reshuffle the details and also do a second pass on the kernel mgmt docs.

@echarles
Copy link
Contributor Author

I have added the hl api in the docs, but the python doc is not generated although the .. currentmodule:: jupyter_kernel_mgmt.hl is defined.

WARNING: don't know which module to import for autodocumenting 'start_kernel_async' (try placing a "module" or "currentmodule" directive in the document, or giving an explicit module name)

@echarles
Copy link
Contributor Author

echarles commented Dec 1, 2019

  • API page generation works - Needed to install jupyter_client as we have some rest reference here and there. Added jupyter_client in the requirements.yml
  • Added a dedicate section for HL API

This is ready for review.

High Level API
==============

.. automethod:: jupyter_kernel_mgmt.hl.start_kernel_async
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: I'd like to document these without referring to the hl module, e.g. jupyter_kernel_mgmt.start_kernel_async. That's effectively an implementation detail.

Copy link
Contributor Author

@echarles echarles Dec 1, 2019

Choose a reason for hiding this comment

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

I have tried to define a currentmodule like in the othe rst files but I receive for the methods the following warning and at the end, the methods are not generated.

WARNING: don't know which module to import for autodocumenting 'start_kernel_async' (try placing a "module" or "currentmodule" directive in the document, or giving an explicit module name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried with other configuration, but not sure how sphynx handles the top level methods instead of the class methods.


:class:`start_kernel_blocking <jupyter_kernel_mgmt.hl.start_kernel_blocking>`: Start a kernel by kernel type name. Returns a manager and a blocking client.

:class:`run_kernel_blocking <jupyter_kernel_mgmt.hl.run_kernel_blocking>`: Context manager to run a kernel by kernel type name. Returns a blocking client.
Copy link
Owner

Choose a reason for hiding this comment

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

This is really just duplicating the API docs, so I'd rather leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now removed that file.

@takluyver takluyver merged commit cb88439 into takluyver:master Dec 1, 2019
@echarles echarles deleted the docs-diagrams branch December 1, 2019 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants