FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Javascript link by jasongrout · Pull Request #6454 · ipython/ipython · GitHub

Javascript link - #6454

Merged
jdfreder merged 10 commits into
ipython:masterfrom
jasongrout:links
Dec 10, 2014
Merged

Javascript link #6454
jdfreder merged 10 commits into
ipython:masterfrom
jasongrout:links

Conversation

Copy link
Copy Markdown
Member

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

This uses the destroy handler triggered in #6438.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

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)?

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

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 😀

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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?

Copy link
Copy Markdown
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)

Copy link
Copy Markdown
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)?

Copy link
Copy Markdown
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'.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

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).

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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'?

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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 commented Dec 4, 2014

Copy link
Copy Markdown
Member

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

Carreau closed this Dec 4, 2014
Carreau mentioned this pull request Dec 4, 2014

Carreau commented Dec 4, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented Dec 4, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented Dec 4, 2014

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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 commented Dec 9, 2014

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member Author

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

jdfreder commented Dec 9, 2014

Copy link
Copy Markdown
Contributor

Sure, I'll give it a test too.

jdfreder commented Dec 9, 2014

Copy link
Copy Markdown
Contributor
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 .

Copy link
Copy Markdown
Member

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

jdfreder commented Dec 9, 2014

Copy link
Copy Markdown
Contributor

@SylvainCorlay whoops you're right.

Copy link
Copy Markdown
Contributor

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 merged commit ce5036c into ipython:master Dec 10, 2014

Copy link
Copy Markdown
Contributor

Thanks @jasongrout !

minrk modified the milestone: 3.0 Jan 29, 2015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

9 participants


Back | FazBrowse Home | New Git URL