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

Removed dependency on Kernel Gateway #734

Merged
merged 5 commits into from
Sep 18, 2019

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Sep 13, 2019

This PR consists of the following changes to remove Enterprise Gateway's dependency on Kernel Gateway:

  • Copy necessary handlers from Kernel Gateway to EG
    • Promote WebSocketPersonality code since NotebookHTTPPersonality will not be supported and we only have one personality (whew! 😄)
  • Update all dependency files (setup, requirements, etc)
  • Rename KG_ prefixed variables (env and code) to EG_. For env variables we fallback to checking the "old" KG_ form when the EG_ form is not set - allowing existing configs to continue working.
  • Address lint issues that are not required in KG, but are in EG
  • Update/remove tests accordingly
  • Update references in docs (and README) - replace "built on Kernel Gateway" with things like "instead of Kernel Gateway". Add explanation of when you'd chose one over the other.
  • Extend /api endpoint to return Gateway version info as well (only returns underlying notebook version)

Fixes #733

@kevin-bates kevin-bates added this to the v2.x milestone Sep 13, 2019
@kevin-bates kevin-bates force-pushed the remove-jkg-dependency branch 3 times, most recently from 0c137f4 to 76e55c1 Compare September 13, 2019 22:01
kevin-bates and others added 3 commits September 13, 2019 15:03
Copied the JupyterWebSocket, kernels, kernelspecs, sessions,
base handlers and mixins from Jupyter Kernel Gateway.  The
WebSocketPersonality was promoted as a top-level set of
handlers since the NotebookHttpPersonality was not copied
and there's no need to retain multiple personalities.

The following appear as authors on at least one of the files
copied from Juptyer Kernel Gateway...

Co-authored-by: Brian Burns <[email protected]>
Co-authored-by: Corey A. Stubbs <[email protected]>
Co-authored-by: Graham Dumpleton <[email protected]>
Co-authored-by: Kevin Bates <[email protected]>
Co-authored-by: Luciano Resende <[email protected]>
Co-authored-by: Min RK <[email protected]>
Co-authored-by: Nitin Dahyabhai <[email protected]>
Co-authored-by: Peter Parente <[email protected]>
Co-authored-by: Roland Weber <[email protected]>
Co-authored-by: SimonBiggs <[email protected]>
For the environment variable default values, we'll go ahead and
reference the previous KG_ names only when the EG_ values are not
present. I.e., we leverage the default parameter with another
getenv call, but against the KG_ name, before returning the "fallback"
default.
@kevin-bates kevin-bates changed the title [WIP] Removed dependency on Kernel Gateway Removed dependency on Kernel Gateway Sep 16, 2019
Copy link
Contributor

@esevan esevan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

2. You wish to configured the [notebook-http mode](https://jupyter-kernel-gateway.readthedocs.io/en/latest/http-mode.html) functionality where a specific Notebook provides HTTP endpoints

Note that Enterprise Gateway also supports local kernels by default. However, HA/DR functionality won't be affective unless kernels run remotely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sooooo clear! Thanks! 🎉

enterprise_gateway/enterprisegatewayapp.py Outdated Show resolved Hide resolved
Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks for the attributions on the JKG commit

@kevin-bates kevin-bates merged commit fc4969c into jupyter-server:master Sep 18, 2019
@kevin-bates kevin-bates deleted the remove-jkg-dependency branch September 19, 2019 23:39
@echarles
Copy link
Member

@kevin-bates I understand this PR removes the ability for EG to support notebook-http-mode which is a big feature loose for me. I would love to use EG for both WebSocket and HTTP modes.

Any reason to have taken that decision? Would it make sense that I look to bring back that feature with a PR?

@lresende
Copy link
Member

lresende commented Oct 19, 2019

@echarles We would prefer to have the notebook-http-mode as its own library either from JKG or a new one. Also, the JKG personalities are kind of mutually exclusive, which is sort of why we decided to go in this direction.

@echarles
Copy link
Member

@lresende Thx for the answer. A HTTP Personality which runs on a distributed cluster will be at some point needed. If EG does not offer it, I will look at other options for that. JKG is not a valid option to me as it can not be distributed.

@kevin-bates
Copy link
Member Author

@echarles - so you were using http-personality via EG to host a "remote web server"? Which form of remote kernel were you using?

Sorry, but the http-personality is just not in the design center for EG as it requires the complete server just to host a kernel.

@echarles
Copy link
Member

@kevin-bates I am not using EG for http-personality for now but it sounded logical to me, in my quest of a scalable jupyter stack, to consider it.

Saying in another way, if I want to serve the users' kernels runtime via EG websocket-mod, all Websocket communication would go through the single EG Websocket server (which is also BTW a single HTTP Tornado Server). This means single server limitation which can be overcomed eg by running that single server on a platform like K8S.

The same logic applies exactly for http-mode. Say if you want to expose that cell to a user (eg javascript REST client), your endpoint server should be able to access your spark distributed cluster.

# GET /api/filter
res = spark.load(...)
...

With this change, this is no more possible.

@kevin-bates
Copy link
Member Author

Yes, but in http-mode only ONE kernel can be served by the gateway server - so "scale" doesn't really apply. If you're interested in k8s kernels, then just run JKG in k8s with its single local kernel. Remote kernels really don't come into play when there can only be one.

Likewise for a Spark cluster. JKG can launch a client-mode kernel (rather than cluster-mode) since there will only be one kernel - having it run in client mode isn't that big a deal - it still can access the spark distributed cluster.

@echarles
Copy link
Member

echarles commented Oct 21, 2019

oh Should have done more tests to realize that only ONE kernel would be shared across all kenerl-gateway clients.

This leads to the need to support session based communication as discussed in jupyter-server/jupyter_server#122

@kevin-bates
Copy link
Member Author

Hmm - I'm not sure I follow. In http-mode, the gateway process user is the notebook user in the sense that it is the thing actually issuing the kernel cell execution requests. The actual user (from the client) is merely hitting different web endpoints, each of which correspond to a different cell to execute. I suspect most notebooks used in this way would be completely stateless between cells so as to not side-affect other endpoint results.

@echarles
Copy link
Member

yeah maybe I expect too much from http-mode (it has been a long time I have used it). Let's leave it like that now and maybe I will try out something when I have a more clear idea about how it works with kernel gateway and the requirements I would like to have.

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.

Remove dependency on Kernel Gateway
4 participants