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

Javascript link #6454

Merged
merged 10 commits into from
Dec 10, 2014
Merged

Javascript link #6454

merged 10 commits into from
Dec 10, 2014

Conversation

jasongrout
Copy link
Member

This connects models on the javascript side instead of the python side.

This uses the destroy handler triggered in #6438.

@SylvainCorlay
Copy link
Member

I think that it is an important one because:
-it extends the base widget model and not the view - edit: it is viewless.
-it is a widget without being a DOM widget. - edit: Widget as in XObject cf gitter discussion.
-depends on the backbone model destruction PR #6438.
-Since #6151, there is no need to call display as the JavaScript model is instantiated on creation of the Python side.
-It should allow some simple interaction and links between widgets to still work without a kernel after the widget persistence PR is merged, and therefore in the nbconvert output.

@jdfreder
Copy link
Member

So I have to admit, I'm a little sad this doesn't link on both sides. If you ran the following Python code in a single cell, would it not behave as-if the link didn't work?

from IPython.html import widgets
from IPython.display import display
w1 = widgets.IntSlider(value=5)
w2 = widgets.IntSlider(value=5)
link = widgets.Link(widgets=(w1, w2))
w1.value = 6
print w2.value

prints 5 instead of 6.

Why not link on both ends and not send state changes made by the link (under the assumption the same will happen at the other end)?

@jasongrout
Copy link
Member Author

I guess what is happening here is that the python is triggering a message to the javascript, which changes w2, but then the message back to python about w2's change isn't being executed until after the python is run.

It seems a bit sketchy to me to independently trigger a value change on either side and not sync with the other. That seems to be asking for race conditions and getting things out of sync. On the other hand, I agree that your example is rather confusing.

Perhaps state updates should happen on a separate thread so that they can happen while the main python program is running. You can trigger state events on the main thread, but have actual updates between javascript and python happen on this separate thread.

@SylvainCorlay
Copy link
Member

We could have the DirectionalLink widget also do the link in the Python side using traitlets.dlink. The bidirectional link might be a bit more tricky.

@jasongrout
Copy link
Member Author

I'm "evolving" my position---the nice thing about triggering independently on the javascript and the python sides is that exactly one message has to go across the wire, which is potentially a huge speedup.

@jdfreder
Copy link
Member

I know you said you didn't like this idea, but if both sides were synced, no messages would need to be sent at all 😀

@jasongrout
Copy link
Member Author

Well, of course the initial value for the triggering widget will need to make it across the wire.

@SylvainCorlay
Copy link
Member

Rather than registering our handler with on("destroy") we should maybe use backbone's function once which un-registers the callback after the first time the event fires. once("destroy") .

Otherwise this keep a reference to the model and prevent the garbage collection to happen.

@SylvainCorlay
Copy link
Member

Linking widget attributes in both front-end and back-end can be very tricky.

For example, let say that that wid1 and wid2 are two widgets both having an attribute a initially set to 0, which we link bidirectionally

Setting wid1.a from 0 to 1 and then 2 on the Python side can lead to

  • wid1.a is set to 1 / near-immediate update of wid2.a to 1 on the backend
  • Message sent to the front-end updating wid1 to 1
  • wid1.a is set to 2 / near-immediate update of wid2.a to 2 on the backend
  • Message comes back from the front-end setting wid2.a back to 1 because of the first update
    etc.
    bidirectional link might require disconnecting one of the widgets from its front-end (for a given attribute) with some attribute_lock like or tagging the messages.
    What exactly would be the use case of linking in both frontend and backend?

@takluyver
Copy link
Member

From discussion today with @jdfreder, @SylvainCorlay, @jasongrout and @minrk , I increasingly feel that, while linking widgets in the frontend could be useful, this approach is fundamentally wrong.

Because widgets have convenient hooks for instantiation -- and with my PR #6494, loading -- by the kernel, it's very tempting to use them as a generic RPC/object framework. This is giving in to that temptation: it creates a new backbone model and new comm, neither of which are being used for their intended purpose. And in discussions today, we briefly considered adding an invisible 'view' for it as well. And when it comes to serialisation, the problem gets sharply worse: it becomes necessary to build a DAG of 'widgets' (inverted commas because our use of the term widget is going all fuzzy again) and recreate them in order based on their dependencies.

I'm not sure what the big picture solution to this is, but I think we should consider it out of scope, at least until we've had more time to step back and think about these things. Simple frontend linking might be special enough that we should build it into our widget framework, but even if that is the case, it shouldn't be done by creating a separate 'model'.

(Of course, we can't stop people doing things like this in their own code, and I wouldn't want to. But I don't think it belongs in IPython, and I definitely don't think our design of widget persistence in the notebook format should be based on this)

@SylvainCorlay
Copy link
Member

A widget with no view would not be 'displayed', and naturally the link would fall into that category. Setting the _view_name to None, making things clear rather than using the base view class. (Possible after #6532.)

I agree that Widget is probably not the best name for what it is actually. (We could probably have named Widget something like ClientObject and DOMWidget could be renamed into Widget. Things would be clearer.)

Regarding the persistence, I still believe that widgets models (in the large sense) with no views should be allowed to persist. It will allow for interactivity in the nbconverted notebook. In any case, even if you make the decision to not persist models that do not correspond to a view, it is easy to work around by doing something ugly like

def link_persist(*args):
    l = link(*args)
    l._view_name = 'LatexView'   # dynamically setting the view to something empty
    display(l)
    return l

What exactly is wrong with using the current widgets as a convenient way of instantiation (like for the button which does not have any synced state)?

@takluyver
Copy link
Member

Having thought about this overnight, I'm afraid that I'm even more convinced that this should not be enshrined in IPython. You are trying to:

  1. Assemble complex JS applications from pre-written components, without writing any new JS, by using an RPC framework.
  2. Serialise not just the state but the structure of these applications, so that you can regenerate an interactive application from a purely declarative representation. No existing system I'm aware of has this capability.

You're welcome to hack 1 together on top of the widget infrastructure, but it's not what that infrastructure is designed for, and I don't want to codify that kind of use in IPython itself. And while 2 would be a fascinating experiment, our notebook format should not be the vehicle for it.

Some of the widgets already in IPython do stretch the boundaries of the widget model, but that's not carte blanche to do arbitrary other things by instantiating a 'widget'.

@jdfreder
Copy link
Member

Serialise not just the state but the structure of these applications

Just to clarify, the structure is a piece of state and is encoded as such. If we are going to persist the state of widgets, the structure is included.

@takluyver
Copy link
Member

Document structure has to be included. The application structure - the graph of which components refer to which other components, is another question entirely.

@jdfreder
Copy link
Member

Document structure has to be included. The application structure

I think you've lost me, how do the two differ in this context? The nesting of widgets (I think this is what you are referring to as application structure, right?) vs what cells the widgets belong to? It seems that nesting of widgets is something that we do want to persist, right?

@takluyver
Copy link
Member

Nesting of widgets that you can see, e.g. a Box that contains a text box and a slider, is what I call document structure. Application structure is things like the existence of an invisible Link object that refers to other widgets - there's no obvious place to put that in a document structure.

@jdfreder
Copy link
Member

Ahh I see, but you're aware that the two are implemented in such a way that they are the same thing in the widget framework?

@takluyver
Copy link
Member

The document structure is a strict nesting, so there's an obvious way to serialise it to JSON. Application structure is a DAG, so serialising it gets much more complex.

@jdfreder
Copy link
Member

Visible or not, all widget relationships are encoded as a DAG.

There isn't a way to easily distinguish between model references that are used as visible children versus references that are used for some other purpose. The attribute names (there can be any number) of the child list or single child is a property of the widgets themselves, and the way those attributes are used are up to the widget's view. Just to be clear, the trait children, which is used in all of the widgets that we distribute with IPython, could be renamed to anything. The Box widget could call it cars and the Accordion widget could call it trucks and it would still work.

So the way I see it, we either encode all of this structure, or none of it (which simply wouldn't be useful at all).

@SylvainCorlay
Copy link
Member

I have been thinking about this again, and the reservations of @minrk and @takluyver about storing the model states in the notebook metadata.
One way to look at the issue, is that in the same way as there is input and output for cells, if the widget persistence is implemented as proposed now, the top-level metadata will contain generated content (the model states) and input-level information, like the kernel specs... Actions like clear all output and scripts like the nbstripout git hook would need to delete the generated content at the top level as well.
It could actually be a good idea to separate generated content from input in the notebook-level metadata, to make a clear separation.

@takluyver
Copy link
Member

I'm not sure that that distinction is clear. If I have typed in a text box, isn't the text that I have typed 'input'?

@SylvainCorlay
Copy link
Member

Good point, but the thing is that a new model id is generated every time you run the cell... I would see it as part of output persistence, like images.

@SylvainCorlay
Copy link
Member

Same for me, I apologize. if I said anything that offended someone in this discussion. I am +1 on a clear separation between widgets and the core of IPython.
As @jdfreder mentioned earlier, it is already well layered - although, besides the bare comm messages, one may want to use some of the higher-level mechanisms without the assumption of JQuery.

@Carreau
Copy link
Member

Carreau commented Dec 4, 2014

Dead for more than a month, closing PR as per policy and open related issue.

@Carreau Carreau closed this Dec 4, 2014
@Carreau Carreau mentioned this pull request Dec 4, 2014
@Carreau
Copy link
Member

Carreau commented Dec 4, 2014

Opened #7102 for discsussion, feel free to reopen if needde.

@ellisonbg
Copy link
Member

I don't think this PR meets our criteria for closing:

https://github.com/ipython/ipython/wiki/Dev:-Closing-pull-requests

It isn't waiting for the author to write more code and there is no deep technical issue that has to be figured out. We just need to review and decide.

Also, our policy recommends that before we close a PR we:

"Post a github message to the pull request to confirm that everyone is fine with closing the pull request. This message should cite this policy."

If it is super obvious to all that a PR should be closed we sometimes omit this step, but I think it is good practice to try and remember to do this.

Reopening and tagging as Needs Review and Needs Decision...

@fperez
Copy link
Member

fperez commented Dec 4, 2014

I'm approving merge with BDFL privilege on this one, see youtube discussion for details. I will leave the actual merge to others in case there's any last-minute technical work to be done.

It does need a rebase, @jasongrout will do that.

Conflicts:
	examples/Interactive Widgets/Widget Events.ipynb
@jasongrout
Copy link
Member Author

Don't merge this yet. It's currently broken. I think it relies on a comm closing to delete the model on the javascript side, but recent changes make it so that closing a comm will not delete the model on the javascript side.

@SylvainCorlay - can you look at this? It appears that if you have a javascript link l, l.close() no longer severs the connection.

@jdfreder
Copy link
Member

jdfreder commented Dec 4, 2014

@jasongrout the persistence PR also introduces some fixes for front-end widget lifetime related bugs. It's possible the work in there may help...

@SylvainCorlay
Copy link
Member

In html/static/services/kernel/comm.js, line 111
replace this.unregister_comm(comm); with that.unregister_comm(comm); (and define that).
Then it works.

@jdfreder
Copy link
Member

jdfreder commented Dec 9, 2014

🍰 for @SylvainCorlay . @jasongrout can you make this change?

@jasongrout
Copy link
Member Author

Done. Can you look at it, @jdfreder and @SylvainCorlay?

@jdfreder
Copy link
Member

jdfreder commented Dec 9, 2014

Sure, I'll give it a test too.

@jdfreder
Copy link
Member

jdfreder commented Dec 9, 2014

from IPython.html.widgets import *
from IPython.display import display
a = IntSlider()
b = FloatSlider()
display(a, b)
Link([(a, 'value'), (b, 'value')])

Works

Edit: Fixed error in my test, thanks @SylvainCorlay .

@SylvainCorlay
Copy link
Member

The syntax would be link((a, 'value'), (b, 'value')) right?

@jdfreder
Copy link
Member

jdfreder commented Dec 9, 2014

@SylvainCorlay whoops you're right.

@jdfreder
Copy link
Member

I reviewed this looks good and verified with @SylvainCorlay that it was okay to merge this.

jdfreder added a commit that referenced this pull request Dec 10, 2014
@jdfreder jdfreder merged commit ce5036c into ipython:master Dec 10, 2014
@jdfreder
Copy link
Member

Thanks @jasongrout !

@minrk minrk modified the milestone: 3.0 Jan 29, 2015
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.

None yet

9 participants