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

RFC: Synchronous updates by default #367

Closed
wants to merge 16 commits into from
Closed

RFC: Synchronous updates by default #367

wants to merge 16 commits into from

Conversation

shashi
Copy link
Member

@shashi shashi commented Dec 16, 2019

The Condition, wait, notify served a purpose which can easily be served by a vector. This should also make #343 go away? Interact works for me without trouble with this patch, but I want to ask you all if I'm missing something! @travigd @piever @rdeits

Unrelated to this PR:

I got here trying to make

data = Iterators.repeated((), 100)
opt = ADAM(0.1)
using Interact
using .Threads

current_plot = Observable{Any}(nothing)
cb = function () #callback function to observe training
  display(loss_adjoint())
  # using `remake` to re-create our `prob` with current parameters `p`
  current_plot[] = plot(solve(remake(prob,p=Flux.data(p)),Tsit5(),saveat=0.0:0.1:10.0),ylim=(0,6))
end

# Display the ODE with the initial parameter values.
cb()

display(current_plot)

Flux.train!(loss_adjoint, params, data, opt, cb = cb)

actually show me a plot that updates in real time, but it still accumulates updates till the computation is done. I think this might be because of IJulia.

I see that the only other place that uses async now is send_request in messaging.jl, not that it's a bad thing.

@shashi
Copy link
Member Author

shashi commented Dec 16, 2019

Just for context if anyone's wondering/forgotten why we need outbox, it's because some Scope code may update observables before they are rendered (e.g. evaljs) so we need to queue them up.

We can also go back on that and say evaljs can only be called after a Scope is displayed, which intuitively also makes sense, and probably promotes modularity, but the current design is modular too. I used to use evaljs for onimport, but that has changed.

@twavv
Copy link
Member

twavv commented Dec 16, 2019

I haven't looked to closely but one concern with this is that we still need an outbox limit to avoid the memory leak issues when scopes go away.

We should also garbage collect / reap old scopes(?) but that's a whole other project.

@shashi
Copy link
Member Author

shashi commented Dec 16, 2019

The outbox is only useful until the Scope is first displayed. We should just error if further updates are received to a scope that has 0 connections. Easy to do this!

@twavv
Copy link
Member

twavv commented Dec 17, 2019

The outbox is only useful until the Scope is first displayed. We should just error if further updates are received to a scope that has 0 connections. Easy to do this!

This sounds good, but we also shouldn't buffer messages forever if the scope is never displayed.

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Yeah hmm, if we get rid of this buffer too, then that won't happen, any message that needs to be sent without a display will cause an error on the Julia side. Which seems fair to me.

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

But there may be code that will silently drop messages though:

obs = Observable{Any}(nothing)

@async for i=1:10
    obs[] = i
    sleep(1)
end
obs

for example will not send the first message because the observable is not yet displayed.

But then that code will do the same right now as well... :-p

Edit: oh wait, it actually doesn't even drop the first update because the observable is rendered within the first second

@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #367 into next will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next     #367      +/-   ##
=========================================
+ Coverage   62.1%   62.13%   +0.03%     
=========================================
  Files         16       16              
  Lines        599      589      -10     
=========================================
- Hits         372      366       -6     
+ Misses       227      223       -4
Impacted Files Coverage Δ
src/connection.jl 89.47% <100%> (+7.33%) ⬆️
src/scope.jl 66.26% <100%> (+0.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4748f0...e78cefc. Read the comment docs.

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Ok how about this to detect if an element is still attached...

https://stackoverflow.com/questions/5649534/html-element-is-attached-to-a-document ?

function is_element_in_document ( element ) {
    if (element === document) {
        return true;
    }
    element = element.parentNode;
    if (element) {
        return is_element_in_document ( element );
    }
    return false;
}

But I can see how this might be a problem if the scope ends up in some container (like tabs or switchy stuff) that may remove and bring back things.... In general it seems hard to figure out how to GC a scope because of this possibility?

function Sockets.send(pool::ConnectionPool, msg)
push!(pool.outbox, msg)
process_messages(pool)
function sendall(pool::Set, msg)
Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of partial to leaving ConnectionPool as a struct, even if we remove the outbox, and possibly just defining Sockets.send(::ConnectionPool, ...).

Minor thing though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Naahhh, if it's a set of connections, why not just Set{AbstractConnection}? Least number of names is the best number of names.

@twavv
Copy link
Member

twavv commented Dec 17, 2019

We can also go back on that and say evaljs can only be called after a Scope is displayed, which intuitively also makes sense, and probably promotes modularity, but the current design is modular too. I used to use evaljs for onimport, but that has changed.

Oh, are you saying that we don't need evaljs because we can just use onimport/onmount? I'm totally on board with that.

The one thing I might be in favor of adding though (if we go down that route) is the ability to wait scopes (and have it return when there is a connection available).

@twavv
Copy link
Member

twavv commented Dec 17, 2019

While we're doing all of this anyway, I'd really love to finally implement observables that pull their first value (instead of being rendered with the initial value embedded in the JS/JSON).

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Why do you need that?

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

I see why you'd want a way to wait on the Scope. I can add that in.

@twavv
Copy link
Member

twavv commented Dec 17, 2019

Thinking about this a bit more, I think this definitely needs to go in tandem with eagerly pulling observables on render (otherwise there's a race condition where if you display an observable, update it before it gets a chance to render in the frontend, it uses the stale value).

Edit: but that can be a separate PR

@twavv
Copy link
Member

twavv commented Dec 17, 2019

Also, I think the waiting would be really easy to implement if you left ConnectionPool as a struct and added a Condition to it... :^)

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Yeahhh well... I'll add it back. :)

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Thinking about this a bit more, I think this definitely needs to go in tandem with eagerly pulling observables on render (otherwise there's a race condition where if you display an observable, update it before it gets a chance to render in the frontend, it uses the stale value).
Suree....

x = Observable{Any}(1)
display(x)
x[]=2

does reproduce that! (in a notebook)

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

Buut... It goes away if I roll back the last commit.

But what about FOUC when you pull?? And I feel like it's a lot of complexity to justify.

@twavv
Copy link
Member

twavv commented Dec 17, 2019

But what about FOUC when you pull??

what

And I feel like it's a lot of complexity to justify.

Assuming you mean the pull-on-render thing, one big justification for it is that if you refresh a notebook, you get stale values (since it's not updated until a new value is pushed).

@shashi
Copy link
Member Author

shashi commented Dec 17, 2019

what

Flash of unstyled content haha

big justification

👍

@twavv
Copy link
Member

twavv commented Dec 17, 2019

Re: FOUC

We could include the initial (on-render) value with the payload anyway because all of the JS code assumes that observables can be read synchronously (we don't want to just return null if we don't have the values yet since that would break things). This has a FOSC problem (flash of stale content) though.

The other option might be to pull the values of every observable associated with a scope before rendering that scope (again we have to wait to render the scope because we expect to read observable values synchronously).

@twavv
Copy link
Member

twavv commented Dec 17, 2019

But I can see how this might be a problem if the scope ends up in some container (like tabs or switchy stuff) that may remove and bring back things.... In general it seems hard to figure out how to GC a scope because of this possibility?

Whoops, didn't see this. This is why I like to use the word "reap" instead of GC.

Basically, any frontend can request any scope. Suppose I have notebooks A and B and a single scope S. I display S in both A and B, at which point S is both "live" and "connected." In A, I change the code cell that rendered S to be something else and run it, thus clearing the scope from the notebook (so the notebook can never request it again, unless the scope is held as a variable in the Julia process memory). I also close the page for B. At this point, S is "live" but not "connected." It's live because I can re-open B (e.g. ctrl+shift+tab) but not connected because there are no active WebSockets.

So I think an ideal strategy would be ref-counting the frontends that "know" about a scope and holding only weakrefs otherwise. So in my example above,

  • S is created in Julia
  • S is rendered in A (A tells Julia that it has a reference to A and we increase the refcount)
  • S is rendered in B (B tells Julia...)
  • S now has a refcount of 2
  • We clear the cell that contains S in A (we can hook into the Jupyter notebook machinery here to detect this or maybe just add an event listener for when the mountpoint* DOM element is removed) and A tells Julia to decrement the refcount.
  • B is closed (this does not send the refcount decrement).

Since B could be re-opened, we still hold a strong reference to S.

Later, we might have

  • B re-openes and re-connects to S
  • B clears the cell that contains S and decrements S's refcount
  • In Julia, we no longer hold a strong ref to S
  • S can still be re-rendered if the user has a strong reference to S (e.g. if it's stored as a variable in the REPL); otherwise, we let Julia's GC run its course.

*note about the mointpoint: listening for the mointpoint's removal from the DOM gets around the tabs issue that you described.

@shashi
Copy link
Member Author

shashi commented Dec 18, 2019

Everything sounds good!

I was thinking of having a buffer which saves at most 1 latest value per observable per scope (basically channel with bufsize 1, but it doesn't block but overwrites.) I thought this is exactly the same as pulling and doesn't have the double transfer problem (once to render, another pull to make sure it's correct). But I see it won't work once you have 2 front ends and one of them goes down temporarily.

Another problem that I think we can definitely punt on is:

Say a UI element is actually accumulating some observable value instead of straight up using the latest value, then we need to queue all undelivered updates to it for that connection -- nightmare (in both pull or push regimes).

We can just say WebIO is not that sophisticated sorry.

But this got me thinking maybe there should be a way for users to make sure an update got delivered. In which case we can delegate the responsibility of correctness to the user. So users can listen to any connection on the Scope, using some kind of channel = watchconnections(scope), and @ensure connections foo[] = 3 which blocks until the connections have received the value. But it's wayy complicated, and probably no one will use it ever, so unless this becomes a common problem, we can do it.

@twavv
Copy link
Member

twavv commented Dec 18, 2019

Say a UI element is actually accumulating some observable value instead of straight up using the latest value, then we need to queue all undelivered updates to it for that connection -- nightmare (in both pull or push regimes).

This is exactly what MeshCat does.

I don't think the change from push to pull would really impact MeshCat though, but I'm not sure how it's architected (@rdeits?). The current implementation waits for connections on the scope before starting to send updates so it should be fine anyway.

We could also implement a simple messaging protocol.

onconnection(scope) do conn::AbstractConnection
  WebIO.command(conn, "update-foo", value=new_value, ...)
  browser = WebIO.request(conn, "get-browser")
  browser = browser_resp["browser"]
end

and on the JS side you might do

// in setup code
_webIOScope.onCommand("update-foo", (payload) => {
  setFoo(payload.value);
});

_webIOScope.onRequest("get-browser", () => {
  return window.navigator.browser;
});

As an aside, to make the developer experience better, we could allow scopes to be interpolable (so in code with the @js or js"..." macros can just use $scope.onCommand(...)).

@twavv
Copy link
Member

twavv commented Dec 18, 2019

I just merged #365. It fixes the CI issues (in addition to the actual point of the PR). You might want to try rebasing on master now. :^)

@twavv twavv mentioned this pull request Dec 21, 2019
9 tasks
@twavv twavv changed the base branch from master to next December 22, 2019 02:50
@shashi
Copy link
Member Author

shashi commented Dec 22, 2019

Oh did you just attempt release this?

My latest commit doesn't work, it says

src/WebIO.ts(132,25): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
src/WebIO.ts(133,74): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
src/WebIO.ts(150,30): error TS2345: Argument of type '{ type: "response"; request: WebIORequestType; requestId: string; result: unknown; }' is not assignable to parameter of type 'WebIOMessage'.
  Type '{ type: "response"; request: WebIORequestType; requestId: string; result: unknown; }' is not assignable to type 'RPCOkayResponse'.
    Types of property 'request' are incompatible.
      Type 'WebIORequestType' is not assignable to type 'WebIORequestType.RPC'.

How do I make it go away? I tried to make requestId: string -> [requestId]:string but then it complains that RPCRequest type doesn't have rpcId field, what even is RPCRequest and why does it have an rpcId field which appears nowhere in the Julia code?

@twavv
Copy link
Member

twavv commented Dec 22, 2019

Oh did you just attempt release this?

No, those were commits on the master branch that didn't have this code. Idk why Github is being weird? I commented on the commits themselves. Maybe because I rebased this pull on those.

@twavv twavv mentioned this pull request Dec 23, 2019
4 tasks
@twavv
Copy link
Member

twavv commented Dec 26, 2019

Closing in favor of #371/#372.

@twavv twavv closed this Dec 26, 2019
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.

3 participants