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 Jupyter Server Architecture diagram #801

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

andreyvelich
Copy link
Contributor

I added Jupyter Server diagram to the documentation.
As we discussed on the community meeting, I used draw.io tool, so others can easily modify this in the future.

Please take a look at the doc changes.

In the following PRs I will add workflows for the Session creating and deleting.

cc @blink1073 @Zsailer

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2022

Codecov Report

Merging #801 (e40591d) into main (c51f5a8) will not change coverage.
The diff coverage is n/a.

❗ Current head e40591d differs from pull request most recent head 4a4fee2. Consider uploading reports for the commit 4a4fee2 to get more accurate results

@@           Coverage Diff           @@
##             main     #801   +/-   ##
=======================================
  Coverage   69.96%   69.96%           
=======================================
  Files          62       62           
  Lines        7368     7368           
  Branches     1223     1223           
=======================================
  Hits         5155     5155           
  Misses       1841     1841           
  Partials      372      372           
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.11% <0.00%> (ø)

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 c51f5a8...4a4fee2. Read the comment docs.

blink1073
blink1073 previously approved these changes Apr 25, 2022
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.

Excellent, thanks for this! I added some minor suggestions inline.

docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
docs/source/developers/architecture.rst Outdated Show resolved Hide resolved
ServerApp. It starts a new Kernel for a user's Session and generates a
new Kernel ID.

- **Kernel Spec Manager** parses file with JSON specification for a Kernel.
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
- **Kernel Spec Manager** parses file with JSON specification for a Kernel.
- **Kernel Spec Manager** parses files with JSON specification for Kernels, and provides a list of available kernel configurations.

Copy link
Member

Choose a reason for hiding this comment

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

I find the Jupyter Client reference in the diagram to be a little out of place. I wonder if we were to also add Kernel Manger with a relationship between it and MappingKernelManager it might help clarify. If so, I would suggest adding an entry labeled Jupyter Client with sub-entries of Kernel Spec Manager and Kernel Manager, which would then allow some introduction as to the importance the jupyter_client package serves within the Server architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks @kevin-bates!

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.

Thank you @andreyvelich - this will be very helpful to developers! I had a couple of comments to (perhaps) help clarify things a bit further.

Comment on lines 38 to 42
- **Kernel Gateway** is the web server that provides access to Jupyter Kernels.
There are different ways to create this gateway. If your ServerApp needs to
communicate with remote Kernels, you can use
`the Enterprise Gateway <https://github.com/jupyter-server/enterprise_gateway>`_,
otherwise you can use `the Kernel Gateway <https://github.com/jupyter-server/kernel_gateway>`_.
Copy link
Member

Choose a reason for hiding this comment

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

Because the box states, Kernel Gateway yet both forms of gateway servers are listed here, perhaps using the term Gateway Server may be more clear:

Suggested change
- **Kernel Gateway** is the web server that provides access to Jupyter Kernels.
There are different ways to create this gateway. If your ServerApp needs to
communicate with remote Kernels, you can use
`the Enterprise Gateway <https://github.com/jupyter-server/enterprise_gateway>`_,
otherwise you can use `the Kernel Gateway <https://github.com/jupyter-server/kernel_gateway>`_.
- **Gateway Server** is a web server that, when configured, provides access to Jupyter Kernels running on other hosts.
There are different ways to create a gateway server. If your ServerApp needs to
communicate with remote Kernels residing within resource-managed clusters, you can use
`Enterprise Gateway <https://github.com/jupyter-server/enterprise_gateway>`_,
otherwise, you can use `Kernel Gateway <https://github.com/jupyter-server/kernel_gateway>`_, where Kernels run locally to the gateway server.

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 agree with that, I will rename Kernel Gateway to Gateway Server on the diagram also.

Comment on lines 56 to 58
- **Mapping Kernel Manager** is responsible to operate multiple Kernels in the
ServerApp. It starts a new Kernel for a user's Session and generates a
new Kernel ID.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **Mapping Kernel Manager** is responsible to operate multiple Kernels in the
ServerApp. It starts a new Kernel for a user's Session and generates a
new Kernel ID.
- **Mapping Kernel Manager** is responsible for managing the lifecycles of the Kernels running within the
ServerApp. It starts a new Kernel for a user's Session and facilitates interrupt, restart, and shutdown operations against the kernel.

ServerApp. It starts a new Kernel for a user's Session and generates a
new Kernel ID.

- **Kernel Spec Manager** parses file with JSON specification for a Kernel.
Copy link
Member

Choose a reason for hiding this comment

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

I find the Jupyter Client reference in the diagram to be a little out of place. I wonder if we were to also add Kernel Manger with a relationship between it and MappingKernelManager it might help clarify. If so, I would suggest adding an entry labeled Jupyter Client with sub-entries of Kernel Spec Manager and Kernel Manager, which would then allow some introduction as to the importance the jupyter_client package serves within the Server architecture.

@andreyvelich
Copy link
Contributor Author

Thank you for the review @blink1073 and @kevin-bates.
I modified architecture with Kernel Manager and Gateway Server.
Please let me know if that sounds good.

blink1073
blink1073 previously approved these changes Apr 25, 2022
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.

👍🏼

kevin-bates
kevin-bates previously approved these changes Apr 26, 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.

Great information @andreyvelich - thank you!

@andreyvelich
Copy link
Contributor Author

Thank you for the feedback!
@Zsailer @blink1073 @kevin-bates Are we ready to merge this PR ?

@Zsailer
Copy link
Member

Zsailer commented Apr 26, 2022

Thanks, @andreyvelich. Great work here!

@Zsailer Zsailer merged commit 241f0f2 into jupyter-server:main Apr 26, 2022
@andreyvelich andreyvelich deleted the doc-add-architecture branch April 26, 2022 17:51
@Zsailer
Copy link
Member

Zsailer commented Apr 26, 2022

@andreyvelich, can you share a link to the source document for this diagram? Or maybe you can export the document as XML and we can save it in this repo. That would allow others to edit the diagram in the future using drawIO.

@andreyvelich
Copy link
Contributor Author

andreyvelich commented Apr 26, 2022

@andreyvelich, can you share a link to the source document for this diagram? Or maybe you can export the document as XML and we can save it in this repo. That would allow others to edit the diagram in the future using drawIO.

I believe, https://app.diagrams.net/ allows to import this png file and make some modification.
At least, it works for me and I can edit the diagram.
Did you try that ?

@Zsailer
Copy link
Member

Zsailer commented Apr 26, 2022

The link takes me to a blank diagram and a dialogue to choose from my existing diagrams... but I don't have access to this diagram. Is there a share link specifically for this diagram?

@andreyvelich
Copy link
Contributor Author

@Zsailer Are you able to execute this:
File -> Open From -> Device -> Select the png file from your local file system ?
I didn't create shared diagram on the Google Drive.
Do we need this if contributors can just upload png images directly to the draw.io and make changes ?

@blink1073
Copy link
Contributor

I tested by downloading the file and uploading to draw.io when reviewing the PR.

@Zsailer
Copy link
Member

Zsailer commented Apr 26, 2022

Woah! I didn't realize draw.io could edit PNG. 🤯 .

Sorry for the my ignorance here, @andreyvelich! This is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants