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

Allow REPL/Kernel to be embedded and share classes/variables with its caller #102

Merged
merged 6 commits into from
Sep 28, 2020

Conversation

fmagin
Copy link
Contributor

@fmagin fmagin commented Sep 9, 2020

Currently the Kotlin kernel supports something similar to IPython.start_kernel. This PR aims to support something that covers the use cases of IPython.embed_kernel.

What this PR provides

An application can now call org.jetbrains.kotlin.jupyter.iKotlin.kernelServer directly and pass a desired KernelConfig and the runtime properties where the embedded property is set to true and return the KernelConfig as a standard JSON to user in an application specific way.
The user can then run jupyter console --existing=path/to/kernelconfig.json to attach a Kotlin REPL to the kernel and run arbitrary Kotlin code inside the application context with the same state as the the application.

What this PR does not allow

This does not allow attaching a Jupyter Notebook to the Kernel. This is a limitation of Jupyter Notebooks and not the Kotlin kernel.

Status

This should work already, but I'm currently still writing the actual embedding code for the application I want to use it in, so until then I still consider this PR a draft, but appreciate an feedback on the general idea and current implementation.

EDIT:
The first proof of concept for this already works and I think anything else I wanted to achieve is an issue with the application embedding the kernel. This embedding could be simplified by allowing the embedding Java Application to acess defaultRuntimeProperties. I am also feeling unsure if the runtimeproperties is really the right place for the embedding option, and if KernelArgs/KernelConfig wouldn't be a better place.

@fmagin fmagin marked this pull request as ready for review September 10, 2020 13:24
@fmagin fmagin marked this pull request as draft September 10, 2020 14:15
@fmagin fmagin marked this pull request as ready for review September 16, 2020 12:29
@fmagin
Copy link
Contributor Author

fmagin commented Sep 16, 2020

I have been testing and using this for the past week and it works quite well, and allows some really powerful extensions to at least one Java GUI tool that I am using. The approach should be general enough that this also works for every other JVM project that wants to simply embed a kernel in its context.

Copy link
Contributor

@ileasile ileasile left a comment

Choose a reason for hiding this comment

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

I think that if it has real applications, this PR is OK, except one small thing. Please fix it and I'll merge it

@@ -83,6 +83,26 @@ class RuntimeKernelProperties(val map: Map<String, String>): ReplRuntimeProperti
}
}

data class KernelCfgFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that name KernelJupyterParams fits better. Anyway, please rename this class to something without abbreviations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good name, changed it.

@fmagin
Copy link
Contributor Author

fmagin commented Sep 28, 2020

The testcases work for me locally, both Test for Java 8 and Test for Java 11 (set Gradle JVM to v.11). Not sure why the TeamCity build fails and I can't access the details.

@ileasile ileasile merged commit da7be34 into Kotlin:master Sep 28, 2020
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.

2 participants