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

@manipulate sometimes doesn't create the Comm link successfully #115

Closed
ssfrr opened this issue Oct 27, 2016 · 22 comments
Closed

@manipulate sometimes doesn't create the Comm link successfully #115

ssfrr opened this issue Oct 27, 2016 · 22 comments

Comments

@ssfrr
Copy link

ssfrr commented Oct 27, 2016

I have a notebook with one cell:

using Interact
using PlotlyJS

and another:

p = plot(scatter())

@manipulate for phase=0.0:0.01:2pi
    e = sin(linspace(0.0, 2pi, 50)+phase)
    restyle!(p, y=[e])
end

p

I think in the past this used to work, but right now when I run both cells the first time, the sliders don't update the plot. After I refresh the page and re-run the 2nd cell, I get a bunch of this error in the javascript console:

comm.js:127 Comm promise not found for comm id 6837a751-a367-4e5f-a482-828a1d607712

If I use display(p) before @manipulate (instead of returning p from the cell) it seems to work.

In talking to @spencerlyon2 one theory is that it's trying to create a Comm link before the plot has been rendered. I think it's OK if this isn't supported and the user should always display the plot first, but if possible @manipulate should throw an error telling the user that it couldn't talk to the plot, and maybe guiding them to display it first.

@JobJob
Copy link
Member

JobJob commented Oct 28, 2016

Try this in the second cell:

p = plot(scatter())

@manipulate for phase=0.0:0.01:2pi
    e = sin(linspace(0.0, 2pi, 50)+phase)
    restyle!(p, y=[e])
    p
end

Basically, the @manipulate block returns a signal whose value is the last thing in the block.

If the @manipulate is the last thing in the cell, i.e. the result of the cell, then IJulia displays it by default. The restyle! apparently returns nothing, so having that at the end of the @manipulate block creates a signal with value nothing.

Changing the @manipulate block as above makes it return a signal whose value is the plot. IJulia displays this plot (signal) and Interact then ensures the display of that plot (signal) is updated as the slider/plot (signal) value is changed.

In your code, the result of the cell was just a plot (p), not a plot signal, so IJulia just displayed it as a regular static plot, and Interact wasn't involved in updating that display (since it wasn't a signal).

@JobJob
Copy link
Member

JobJob commented Oct 30, 2016

Sorry, I didn't realise PlotlyJS handles its own comms and syncing. I assumed it was relying on Interact for that. My code above (where Interact basically outputs an entirely new plot each time the slider is changed) does actually work both before and after refresh, but it's less smooth than when you use PlotlyJS's own sync. In any case, this warrants further investigation.

There seem to be two possibly related issues here:

  1. When using @manipulate the plot doesn't update when p is the result of the cell (it seems to work when display is called before or after the @manipulate).

N.b. Re-running the cell in your code with the p at the end, works the second time.

Also, having the @manipulate call in the cell after also works, like so:

Cell 2:

p = plot(scatter())

Cell 3:

@manipulate for phase=0.0:0.01:2pi
    e = sin(linspace(0.0, 2pi, 50)+phase)
    restyle!(p, y=[e])
end

Curiously, this also works:

p = plot(scatter())
phase_slider = slider(0:0.01:2pi)
restyle_it! = map(signal(phase_slider)) do phase
    e = sin(linspace(0.0, 2pi, 50)+phase)
    restyle!(p, y=[e])
end
display(phase_slider)
p
  1. PlotlyJS's sync after a page refresh doesn't work (there is Comm promise ... comm id) error, regardless of whether you use display or p is the result of the cell.

Seems to me they're both more likely to be bugs in PlotlyJS than in Interact, since @manipulate is only modifying p and is not involved in sending the comms to sync the plot in the cases that don't work.

HTH, sorry again for the earlier misunderstanding.

@JobJob
Copy link
Member

JobJob commented Oct 30, 2016

One other thing, this is basically what the @manipulate expands to:

p = plot(scatter())
let phase_slider = slider(0:0.01:2pi)
    display(phase_slider)
    restyle_it! = map(signal(phase_slider)) do phase
        e = sin(linspace(0.0, 2pi, 50)+phase)
        restyle!(p, y=[e])
    end
end
p

It doesn't work, but without the let, it does.

Edit (after garbage collection thoughts, see comment below): actually it doesn't work for me without the restyle_it! =, presumably because keeping a reference to the signal created by map stops it from getting garbage collected.

Also, similar story for this (doesn't work with let, does without):
restylein(n) = begin
sleep(n)
restyle!(p, y=[e])
println("done")
end
let
e = sin(linspace(0.0, 2pi, 50))
@schedule restylein(1)
end
How is let affecting the restyle! call?

@JobJob
Copy link
Member

JobJob commented Oct 30, 2016

Thought about issue 1) some more, and (ignoring the @schedule stuff for now) it seems that issue is with Interact after all. I think what's happening is that the signal/map created by your manipulate block is being garbage collected, so it doesn't run later. This is because, unlike the usual case, it's not connected to any other (e.g. downstream) signals - since you're only running it for its side effect (the call to restyle!) and the output isn't displayed - so no Nodes/Signals have references to it. Search for preserve on this page. This is why the let block makes a difference, because without it, there's a reference to the signal created by @manipulate in global scope and it doesn't get GC'd.

There are lots of ways to work around this issue, you could

  1. call preserve on the whole manipulate block: preserve(@manipulate ... end) (ugly)
  2. hold a reference to the signal returned by manipulate: hold = @manipulate ... end (might need to use a different variable name for each manipulate block - not 100% sure)
  3. maybe the neatest solution (not tested much) is to redefine @manipulate like so:
macro manipulate(expr)
    :(preserve(@Interact.manipulate $expr)) |> esc
end

Having said that I think there's a strong argument to be made to make @manipulate preserve its signal by default, if only because @manipulate effectively hides the complexity of doing the great stuff Interact allows so well, so we might as well continue to try do that.

@manipulate is, I assume, many new users first exposure to Interact/Reactive and IMO it's best to make it as painless as possible for those people, even if they may possibly run into memory usage issues (which I guess prob could be handled with a note in the docs).

@sglyon
Copy link

sglyon commented Oct 31, 2016

Hey @JobJob that makes more sense to me.

I believe the issues @ssfrr talked about were with Comms whose id was a UUID4. The Comm I'm using in PlotlyJS.jl is named :plotlyjs_eval.

What I've always done that seems to make it work is:

p = plot(...)
display(p)

@manipulate for ...
end

The key is to display the plot after making it, but before the @manipulate block.

I've never had an issue that way -- not sure if that fits in well with your diagnosis or not.

@JobJob
Copy link
Member

JobJob commented Nov 1, 2016

Yeah, I think the reason that's working is only because the @manipulate happens to be at the end of the cell, so the Signal it creates gets displayed as the result of the cell. When Interactdisplays a Signal, it creates a new "downstream" Signal to handle syncing of values to the front-end. This downstream signal then holds a reference to the Signal created by @manipulate so it doesn't get GC'd.

Try this:

p = plot(...)
display(p)

@manipulate for ...
end

gc() #even without this it doesn't work for me, but included just to be sure
nothing

Also, I've realised that some other things I thought were working earlier mentioned above, either don't work every time, or break when you call gc() afterward, namely, running the cell twice, and putting the display(p) after the @manipulate.

So yeah, seems that GC is the problem here with issue 1.

I think the issue with the comms on refresh is to do with the global Comm manager in PlotlyJS. The bug happens if you refresh the page after using PlotlyJS and before calling display. Seems Interact sets up a new Comm manager for each Signal it displays see here. I'm a little vague on the details but when the page is refreshed, the front end seems to lose the reference to the _ijulia_eval_comm.id. Maybe you need to create a new Comm manager on each call to display a SyncPlot? Ok, looked into it, and submitted a PR to PlotlyJS which seems to fix this, not sure it's the best way to do it, but it works: JuliaPlots/PlotlyJS.jl#91

@sglyon
Copy link

sglyon commented Nov 1, 2016

Thanks or the PR!

I'm not totally sure I understand how that PR is different in practice than what I had.

It seems to me that what I had created one Comm when the julia code using PlotlyJS or PlotlyJS.init_notebook(true) is run.

I believe that what is in that PR creates a Comm whenever the Julia code plot(...) or JupyterPlot(...) is run.

Either way, the Comm is only created after the user asks the Julia kernel to execute some code.

Does interact on its own (i.e. just interacting with a julia expression) work between page refreshes?

I would think that the behavior or "pure" Interact.jl Comms or the PlotlyJS.jl Comm(s) should be identical and described by: whenever the gc runs to clear out Julia-side Comms kept in the kernel -- e.g. maybe (? not sure) on page refresh -- the user needs to re-execute the code that created the Comm.

JobJob added a commit to JobJob/PlotlyJS.jl that referenced this issue Nov 1, 2016
JobJob added a commit to JobJob/PlotlyJS.jl that referenced this issue Nov 1, 2016
@JobJob
Copy link
Member

JobJob commented Nov 1, 2016

Firstly, just to clarify, the use case that the (new and old) PRs fix is:
using Interact, PlotlyJS
Then run

p = plot(...)
display(p)

@manipulate for ...
end

refresh page, then run cell again.
Previous behaviour: Doesn't work, JS console displays: Comm promise not found for comm id ...
New behaviour: works fine

This issue is not related to issue 1) above and GC. It is simply caused by the fact that on page refresh the Jupyter front end clears all registered comms, so the _ijulia_eval_comm held by PlotlyJS becomes stale.

The original PR was slightly more complicated because I had an error in the test case I was using. I thought just updating the global Comm on each call to JupiterDisplay wasn't working with multiple plots, so I made it more like the Interact code in that Interact stores a mapping between Signals (equivalent of a plot)->Comms, but in fact this wasn't necessary in the end because of the way PlotlyJS uses the plot's divid.

Anyway, have made a new PR, much simpler. It just creates a new global Comm every time JupiterDisplay is called, this stops the old comm reference from being stale on page refresh.

@shashi
Copy link
Member

shashi commented Nov 1, 2016

I'm glad you have seemed to figured out a fix.

FWIW I can't reproduce Spencer's initial failure on Jupyter 4.2.3 + latest Interact

@JobJob
Copy link
Member

JobJob commented Nov 1, 2016

does this work for you when you move the slider?:

p = plot(scatter())

@manipulate for phase=0.0:0.01:2pi
    e = sin(linspace(0.0, 2pi, 50)+phase)
    restyle!(p, y=[e])
end
gc()
p

@shashi
Copy link
Member

shashi commented Nov 1, 2016

Yes, just fine.

sglyon pushed a commit to JuliaPlots/PlotlyJS.jl that referenced this issue Nov 2, 2016
@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

Even with the gc()?

Does it work for you @spencerlyon2 ? I'm also on Jupyter 4.2.3 and Interact master with Julia 0.5.0. I was pretty sure that the signal created by the @manipulate would have no references to it and get GC'd, but maybe I'm missing something?

@shashi
Copy link
Member

shashi commented Nov 2, 2016

your guess should be correct. I did copy-paste the same code you posted and it works...

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

It works, and the plot updates when you move the slider?

@shashi
Copy link
Member

shashi commented Nov 2, 2016

yes

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

wow, ok... confusing

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

julia 0.5.0 ?

@shashi
Copy link
Member

shashi commented Nov 2, 2016

Eek, wait. I'm so sorry I was trying it without gc. With gc(), sure enough it doesn't work.

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

whew :)

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

@shashi How do you feel about changing @manipulate to either:

  1. use preserve (or foreach instead of map if it works)?
  2. adding the manipulate created signals to a vector that you could clear with say Interact.freemanipulates() or something

That way people using @manipulate for side effects won't run into the OP's problem - with their non-displayed signal getting GC'd

@shashi
Copy link
Member

shashi commented Nov 2, 2016

So, I think it's reasonable to make @manipulate call preserve on its output by default. Because

  1. I agree that it is not easy to think of GC when there's a problem like this
  2. Since @manipulate blocks are usually self-contained, they will get gc'd once all the inputs go out of scope anyway

Only problematic situation is when you have something like

ticks = fps(60)

in one cell

@manipulate for t=ticks, x=1:100
    ...
end

in another. As opposed to

@manipulate for t=fps(60), x=1:100
    ...
end

But this is almost never used

@JobJob
Copy link
Member

JobJob commented Nov 2, 2016

Right, you're saying that won't get gc'd because of the reference to ticks being held. Yeah I agree that's probably not a major problem, people who are doing that are more likely to make their own signal chain without using manipulate, I would think.

JobJob added a commit to JobJob/Interact.jl that referenced this issue Nov 4, 2016
Stops issues with manipulate signals being garbage collected when they go out of scope.
Fixes JuliaGizmos#115
JobJob added a commit to JobJob/Interact.jl that referenced this issue Nov 9, 2016
Stops issues with manipulate signals being garbage collected when they go out of scope.
Fixes JuliaGizmos#115
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

No branches or pull requests

4 participants