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 a kernel that priorly links to a existing matlab engine (besides the original one) #159

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

RibomBalt
Copy link
Contributor

Hello, this jupyter kernel for MATLAB really helps me a lot.

In short, I implemented another kernel (named matlab_connect) that first attempts to connect to a existing MATLAB engine rather than opening a new one. The motivation is that I need to use the same MATLAB session both locally (at my office computer) and remotely (at my home computer). While in the source code start_matlab is called first and connect_matlab is called only when the former one raise an exception, which usually won't happen (see kernel.py 82-85 lines in last version). My contribution is to add an option to connect to an existing engine first. So if you execute matlab.engine.shareEngine in a MATLAB session, then this kernel would directly connect to this session, with all the workspace variables preserved.

The original kernel should be available as well. That means this kernel.py should be able to act as two kernels at the same time. I tried to add another argument in argv of kernel.json (namely "--connect-to-existing-kernel", see setup.py 24-32 lines in new version). However, during my tests it seems that the jupyter server would try to recognize this additional argument and raise errors. To work around that, in __main__.py I added some code to remove the argument and add a environment variable to tell the kernel.py which kernel to use. I know it's kind of ugly but currently I don't know anything better than that.


And there's another small update:
I noticed that in kernel.json the argv[0] is directly given as python. This part won't work for me because my jupyter lab and matlab engine are installed on different python environments (it sounds kinda wield, but I have no choice since my MATLAB R2018a supports up to Python 3.6 but jupyter lab is only available on higher versions). I noticed that jupyter can use kernels in another python environments as long as the correct path to python binary is given to kernel.json. To do so, I have to dynamically generate kernel.json files in setup.py. The result kernel.json now contains the absolute path of python binaries. For normal users who installed matlab engine and jupyter under the same python environment, this would change nothing to them. Also, this dynamic generation makes it easy for me to create 2 kernels at the same time.

I also noticed the 44th line in get_kernel_json fuction in kernel.py:

data['argv'][0] = sys.executable

This line actually replace the python in kernel.json with the absolute path of python file. With the changes above, this part seems to be unnecessary, so I commented it off.


This commit works perfectly on my computer. I would be appreciated if I'd be able to contribute to this great project.

@blink1073
Copy link
Contributor

Thanks @RibomBalt, I like this idea! You can avoid the argv workarounds by using the env part of the kernelspec directly, see https://jupyter-client.readthedocs.io/en/stable/kernels.html#kernel-specs.

@RibomBalt
Copy link
Contributor Author

Thanks @blink1073, I didn't know that env can be directly set in kernel.json. I just created a new commit and got rid of those workarounds.

@@ -9,12 +12,35 @@
break

DISTNAME = 'matlab_kernel'

# generating kernel.json for both kernels
os.makedirs(os.path.join(DISTNAME, 'matlab'), exist_ok=True)
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 we need to add these paths to .gitignore so we don't accidentally add them to source control at some point. We can also rename the existing kernel.json to kernel_template.json to avoid confusion.

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's a good point. It's fixed in a new commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please update MANIFEST.in as well to make sure the template ends up in the sdist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm not familiar with MANIFEST.in, hope I did it right. Also I pushed a rebase to squash the 4 small commits into one.

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.

Looks good, thanks! I'll make a release with this change tomorrow.

@blink1073 blink1073 merged commit 96c3ba0 into Calysto:master Apr 21, 2022
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.

2 participants