-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Connect to synchronized kernel #77
Conversation
Hi, thanks a lot for the PR! This looks awesome.
|
Thank you for your suggestion. I will start making the changes immediately. The following points may require further input:
|
If you don't have Windows at all, I'll do some simple tests once all implementation is done. But do let me know what kind of example (complicated) shell commands should be executed, so I can see if they all work well. |
OK, I will use the native terminal as default and expose the kernel id, so the user can define their own terminal interface if they want. As for Windows, the commands I am currently using are relatively simple, so they should be able to run without issue. However, I will keep in mind that I may need to use more complex commands in the future. |
Let's first see how many people uses this feature. I think it's a niche functionality only a few people may use (but still very useful, thanks!), so if they ever wanted to customise terminal interface we can later add some more options without a dependency. First of all, is there any requirement for you or is horizontal split enough most of the time? And the kernel ID is able to be achieved with the Lua API with one line of javascript. We don't need to really expose it I think. |
What I meant was to add a function similar to Or do we already have something like |
This can be done with |
In the README, I think instead of long description you can just add in the Vim Commands section, add one line of description would be enough? I appreciate it though! |
Good for me, I also feel that the long description now looks a bit inconsistent. |
f1597a5
to
cc86ac8
Compare
src/jupynium/events_control.py
Outdated
@@ -522,6 +522,12 @@ def process_request_event(nvim_info: NvimInfo, driver, event): | |||
event[3].send(ret_obj) | |||
return True, None | |||
|
|||
elif event[1] == "kernel_connect_info": | |||
driver.switch_to.window(nvim_info.window_handles[bufnr]) | |||
kernel_id = driver.execute_script("Jupyter.notebook.kernel.id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you also get this error?
Error executing Lua callback: [string "<nvim>"]:718: attempt to concatenate local 'kernel_id' (a userdata value)
stack traceback:
[string "<nvim>"]:718: in function 'Jupynium_get_kernel_connect_shcmd'
[string "<nvim>"]:726: in function <[string "<nvim>"]:723>
I think the kernel_id value is wrong. You should add return
.
kernel_id = driver.execute_script("return Jupyter.notebook.kernel.id")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I only test no-sync cases (connect to latest kernel) today since it's just implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's okay. I think it works alright now. Thanks a lot for your work. The only things hasn't been done are
- Test on Windows
- Mention
pip install jupyter-console
dependency in README
# jupyter-console is optional and used for `:JupyniumKernelOpenInTerminal`
pip install notebook jupyter-console
I think you can add that command in the README Jupyter Notebook version dependency area. Then we're good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have requested someone else to do the testing work for Windows. If they lack the time, since this plugin does not require any additional installation, I should be able to find an opportunity to complete the testing on Windows.
And as for the document, it's suitable to mention an installation command in the requirements area? But I also don't find other where to place that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can do that, that'd be helpful. He's actually using it on Windows so I thought it would be good to have it from an actual user. Once we make sure it runs on Windows it's good to go.
@sho-87 Hi, it would be really appreciated if you can test this feature on Windows. It runs a shell command so I think potentially it will be buggy on Windows shell. Only if you care about this feature though! You can just test |
b08a36b
to
29efdd2
Compare
A couple of things:
Isn't this basically a REPL window that is connected to the notebook kernel? Calling it a REPL might make it more obvious what this feature does |
Basically, yes. But it would be a bit weird to use it as REPL since we don't know which kernel to connect without sync with jupynium. And jupynium already work as a better REPL. Or we can let |
|
yup, I was just confused about the correct way to test this, but the
tbh I'd be ok with it either way. I was just trying to create a mental model for myself regarding what the feature would be for and REPL was the first thing that came to mind |
To make it a real REPL we should also implement a method to send code cell to the terminal, currently it's just a plain terminal |
I managed to implement an MVP for my feature request on #76. The implementation follows your suggested approach:
Jupyter.notebook.kernel.id
jupyter console --existing
.This implementation supports connecting to a remote kernel by explicitly passing a hostname parameter and a naive approach(
ssh host -t
), and this require https://github.com/akinsho/toggleterm.nvim as a dependencyYou can view the workflow here:
output.mp4
In my opinion, to improve this functionality, we may need to:
Long-term plan
As a beginner in JavaScript and Neovim plugin development, I would be grateful if you could share any additional ideas you may have to further improve this functionality. Additionally, I would appreciate your guidance on what else I need to do to merge this pull request.