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

Create basic comm API shim for jupyter-js-services #264

Merged
merged 10 commits into from
Dec 9, 2015

Conversation

jdfreder
Copy link
Contributor

@jdfreder jdfreder commented Dec 8, 2015

Here's a simple shim that allows use of jupyter-js-services via the old comm API.

  • shim
  • test it / create an example

@parente @blink1073

@jdfreder jdfreder added this to the 5.0 milestone Dec 8, 2015
@parente
Copy link
Contributor

parente commented Dec 8, 2015

Sweet. Thanks for jumping on this. Definitely looks like what I was envisioning.

So on our end we'd have to instantiate and pass the shim to the WidgetManager, plus alias the services/kernels/comm module import here (https://github.com/ipython/ipywidgets/blob/master/ipywidgets/static/widgets/js/manager.js#L8) so that the shim gets imported. I think.

/cc @lbustelo, @dalogsdon @jhpedemonte who are all closer to widgets / dahsboards JS than I have been

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 8, 2015

So on our end we'd have to instantiate and pass the shim to the WidgetManager, plus alias the services/kernels/comm module import here (https://github.com/ipython/ipywidgets/blob/master/ipywidgets/static/widgets/js/manager.js#L8) so that the shim gets imported. I think.

/cc @lbustelo, @dalogsdon @jhpedemonte who are all closer to widgets / dahsboards JS than I have bee

Actually, that's not what I was intending. The WidgetManager you reference is intended for notebook use only, it'd surely break outside of the notebook. Instead my suggestion is took take something like examples/development/web2 as a starting point, and use jupyter-js-services to add backend support. Right now that example only has widgets without live comms.

I'll add another example to this PR clarifying what I mean and demonstrating jupyter-js-services powered widgets outside of the notebook.

@jdfreder jdfreder changed the title WIP: Create basic comm API shim for jupyter-js-services Create basic comm API shim for jupyter-js-services Dec 9, 2015
@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

This is ready to go now! See ipywidgets/examples/development/web3

cc @SylvainCorlay

@rgbkrk
Copy link
Contributor

rgbkrk commented Dec 9, 2015

yeah this is awesome

@blink1073
Copy link
Contributor

Looks like you had to poke into the private API, feel free to push back on what the API needs to look like.

@lbustelo
Copy link

lbustelo commented Dec 9, 2015

Pretty awesome indeed.

Gino B.

On Dec 8, 2015, at 7:04 PM, Kyle Kelley [email protected] wrote:

yeah this is awesome


Reply to this email directly or view it on GitHub.

@parente
Copy link
Contributor

parente commented Dec 9, 2015

@lbustelo and I poked around earlier while you were working up the new demo. Totally get it now. Implement a subclass of the base widget manager. Instantiate the shim. Override the necessary methods. Away you go. Looks great.

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

Looks like you had to poke into the private API, feel free to push back on what the API needs to look like.

Thanks, I know that ipywidgets actually uses some of the private methods of the old API too. The private methods used in the shim may not be essential. I'll have to dig deeper into the code to remember what the reasoning was for creating comms that are unregistered and unknown to the manager (the kernel, in jupyter-js-services's case). It may be that no changes are needed to the new API when I drop the old comm API completely.

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

@SylvainCorlay this PR also fixes some hiccups introduced in the examples by #261

@SylvainCorlay
Copy link
Member

@jdfreder thank you for fixing the example after #261.

If we use private APIs, this may be an indication that to expose more things in the API!

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

Actually, I just looked, we aren't using any private APIs in master right now, so we must have fixed that at some point.

@SylvainCorlay
Copy link
Member

Why moving the examples directory?

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

@blink1073 the only lines that are causing me to use private APIs ATM are these: https://github.com/ipython/ipywidgets/blob/master/ipywidgets/static/widgets/js/manager.js#L260-L262 They create a comm and don't send a comm open message. Looks like the new API supports this, as connectToComm, right?

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

Why moving the examples directory?

I had trouble with it complaining about missing dev-dependencies of ipywidgets otherwise. Basically, in order for the funky relative dependency that we have to work, the sub projects have to be inside the parent project's directory.

My theory is that npm walks outwards for node_modules folders looking for binary dependencies. I found this out because I don't have tsc installed globally on my machine, and yet it's a local installed dependency of ipywidgets. If the examples were outside of the parent folder, I'd get tsc not found errors.

@SylvainCorlay
Copy link
Member

Why moving the examples directory?
I had trouble with it complaining about missing dev-dependencies of ipywidgets otherwise. Basically, in order for the funky relative dependency that we have to work, the sub projects have to be inside the parent project's directory.

My theory is that npm walks outwards for node_modules folders looking for binary dependencies. I found this out because I don't have tsc installed globally on my machine, and yet it's a local installed dependency of ipywidgets. If the examples were outside of the parent folder, I'd get tsc not found errors.

It is weird because I did not have such issues with bqplot while requiring ipywidgets which was not in a parent project directory.

@blink1073
Copy link
Contributor

@jdfreder, connectToComm gives you a comm object, it is up to you whether to call open on it or not.

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

It is weird because I did not have such issues with bqplot while requiring ipywidgets which was not in a parent project directory.

I just double checked and it looks like I can move them back. Now I think what I was seeing earlier was caused by a separate problem, which was that I hadn't ran npm install in the widgets directory yet.

@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

@jdfreder, connectToComm gives you a comm object, it is up to you whether to call open on it or not.

Awesome, I shouldn't need any private members when I make the full transition to jupyter-js-services. Also, I just checked, and the old API methods I'm emulating, that do need private members of jupyter-js-services, aren't even being used by the example, so I went ahead and removed them. There shouldn't be any more private member usage now.

@blink1073
Copy link
Contributor

Great, glad to hear it.

if (callbacks) {
future.onReply = function(msg) {
if (callbacks.shell && callbacks.shell.reply) callbacks.shell.reply(msg);
// TODO: Handle payloads. See https://github.com/jupyter/notebook/blob/master/notebook/static/services/kernels/kernel.js#L923-L947
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should update KernelFuture to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't think so. As you know, the channels are shell, iopub, and stdin. The payload handler acts on shell messages. In the spirit of minimal message processing at the kernel level and the 'hand the full message to the handler and allow it to decide what to do with it' thinking, it makes sense to not handle this in the kernel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, that was my reasoning as well.

@SylvainCorlay
Copy link
Member

Thanks for moving examples back. I have build scripts that fetch this directory and I wanted to be sure it is not moving.

@SylvainCorlay
Copy link
Member

LGTM.

SylvainCorlay added a commit that referenced this pull request Dec 9, 2015
Create basic comm API shim for jupyter-js-services
@SylvainCorlay SylvainCorlay merged commit fc6844f into jupyter-widgets:master Dec 9, 2015
@jdfreder
Copy link
Contributor Author

jdfreder commented Dec 9, 2015

🎉

@blink1073
Copy link
Contributor

Nicely done!

@parente
Copy link
Contributor

parente commented Dec 10, 2015

💥 yay!

@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants