search

jupyter-server / enterprise_gateway

A lightweight, multi-tenant, scalable and secure gateway that enables Jupyter Notebooks to share resources across distributed clusters such as Apache Spark, Kubernetes and others.
https://jupyter-enterprise-gateway.readthedocs.io/en/latest/
Other
623 stars 222 forks source link

Persistent Kernel loading failing in v1.x #711

Closed vikasgarg1996 closed 5 years ago

vikasgarg1996 commented 5 years ago

Hi Kevin,

I was testing Persistent Kernels in JEG code of v1.x and python version 3.7 There is an issue I am facing after startup of persistent kernel was successful.

So when the file (kernel.json) which stores the information about running kernels gets loaded, JEG uses json.load(fp) where fp is file pointer to the file. In the loaded JSON, for every kernel session, kernel_session["connection_info"]["key"] will be a string variable.

While starting the session we change this variable to bytes by using str_to_bytes from ipython_genutils.py3compat. So startup of this kernel gets successful.

In the end, we are trying to commit sessions by saving them again to the same file using json.dump. But the issue comes because kernel_session["connection_info"]["key"] is still a bytes variable and json.dump fails.

In v2.0.0rc2, it is handled using a static method pre_save_transformation. Can we do something similar here?

Screenshot 2019-07-19 at 7 15 58 PM Screenshot 2019-07-19 at 7 16 15 PM Screenshot 2019-07-19 at 7 16 33 PM Screenshot 2019-07-19 at 7 16 42 PM

Environment

esevan commented 5 years ago

Hi, @vikasgarg1996 If you need this before 2.0 release, I think you can import str_to_bytes and bytes_to_str and apply them to your env. Or you can use 2.x rc .

Do you need port this to 1.x and release next version of 1.x before 2.0 release?

BTW, I agree with this should be patched to 1.x branch sooner or later. https://github.com/jupyter/enterprise_gateway/blob/f0a22f2ec6435f6fc99bbdac5475050a1c6e6d9b/enterprise_gateway/services/sessions/kernelsessionmanager.py#L99

kevin-bates commented 5 years ago

Kernel session persistence is still experimental. As @esevan mentions, if you can't move to 2.0 rc2, then we can entertain adding this into 1.x. However, if we backported to 1.x, I'd be inclined to go ahead and update 1.x with the complete functionality of 2.x.

In looking over the various PRs, I suspect this would amount to updating the entire session manager file and then only a couple spots in enterprisegatewayapp.py and kernels/remotemanager.py.

The PRs I find are: #530, #535, #568, #569 and #645.

Not sure how others feel about this, but my thinking is that anything allowing us to get more "air time" on the ultimate target is best. This also enables the ability for others to bring their own session managers that could then contain different content providers - per my comments on #645.

vikasgarg1996 commented 5 years ago

Hi, 1) When are we planning to release 2.0 or 1.3? 2) This issue can be solved in 2 ways in 1.x a) Just include a bytes_to_str before dumping the session information into the file b) Complete functionality of 2.x with pre_save, post_load and FileKernelSessionManager

I am willing to contribute if we want to fix this in 1.x . Please let me know, which way would be more comfortable from your side?

Thanks Vikas

kevin-bates commented 5 years ago

OK - thanks. I'm guessing we could build a 1.3 release sooner than 2.0 although I would hope 2.0 is out within the next month (cc: @lresende), so if you're not going to need other forms of storage in 1.x, I guess I'm inclined to make the simple bytes_to_str change there.

What do others think?

vikasgarg1996 commented 5 years ago

Hi Kevin, I have created a pull request for this #713 . I have tested it with multiple kernels as well.

Please review Thanks Vikas

kevin-bates commented 5 years ago

Fixed via #713