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

Deployment Improvements #3

Merged
merged 17 commits into from
Jan 11, 2017
Merged

Deployment Improvements #3

merged 17 commits into from
Jan 11, 2017

Conversation

chaosphere2112
Copy link
Contributor

This pull request puts vcs.js about where we need it for VCDAT. There's a bit here, so I'll break it down.

Simple changes:

I created a script "vcs-server" that launches the server automatically. I also converted the backend to be installable as a package using setup.py; this also builds the javascript and bundles it in the package, and the server provides a version of the script that is already pointing at the correct URLs. All a user needs to do to use VCS.js is add <script src="http://server.com/scripts/vcs.js"></script> to their page, and it'll be available in the vcs global variable.

Bigger changes:

So, this is the controversial change: I dropped ParaViewWeb, and reimplemented the subset that we need using Tornado.

I did this for a few reasons. A fairly straightforward one is that we have a need for HTTP as well as WebSocket communications (I spent a fair bit of time trying to see if there was any support for running both on a single server in PVW, but couldn't find anything). The reason I believe we need HTTP as well as WebSockets is for client side rendering. It seems like massive overkill to use a WebSocket to make a one-time transfer of data to the client side, and transferring both the array data and the metadata through the same protocol leads to a number of complications (we could serialize the entire array in JSON, but that's extremely bandwidth intensive, re: the very slow client side demo packaged with the previous version). HTTP is a pretty natural fit for solving these; the body of the response is the binary array data (which the browser interprets as an ArrayBuffer), and I used a series of X-Cdms-* headers to communicate the rest of the metadata about the array (shape, datatype, axis information). The result is that the client-side render is actually substantially faster than any of the other rendering methods:

Package Transfer Mechanism Time till image in browser
Plot.ly JSON-encoded Data ~16.5 seconds (mostly spent download clt.json from kitware's servers)
Plot.ly ArrayBuffer Encoded < .5 seconds
VTKWeb WebSocket ~1.5 sec
Tornado Socket WebSocket ~1.2 sec

There also appears to be some kind of race condition that's messing things up in PVW. These GIFs show roughly the difference in my experience between PVW and my Tornado server; generally speaking, on my computer PVW will only render a random subset of the 3 plots that it's supposed to be rendering. Every once in a blue moon it will actually render all three.

ParaviewWeb:

pvw

Tornado:

tornado

There's also this message:

screen shot 2016-10-20 at 2 09 48 pm

This is the server informing you that after 5 minutes of inactivity, it's going to die. I tried setting it to -1 (AssertionError: -1 is not greater than or equal to 0 seconds), to 0 (dies immediately), and to 1e20 (vcs-server: error: argument -t/--timeout: invalid int value: '1e20'). As far as I can tell, there's no way to not have your server commit suicide at some point.

Also, for whatever reason, the PVW click code was dysfunctional:

screen shot 2016-10-20 at 2 13 04 pm

I reimplemented the basics of it using Tornado and JS mouse events, proxied over to the vtkRenderWindowInteractor:

tornado_click

@danlipsa
Copy link
Contributor

There also appears to be some kind of race condition that's messing things up in PVW. These
GIFs show roughly the difference in my experience between PVW and my Tornado server;
generally speaking, on my computer PVW will only render a random subset of the 3 plots that
it's supposed to be rendering. Every once in a blue moon it will actually render all three.

I have seen the same thing.

Regarding ParaViewWeb vs Tornado: it seems that you already have more functionality than we had in the demo - we didn't have the click implemented. One thing that we'll need in the future is passing mouse moves to zoom into datasets. This is already implemented in ParaView web - we'll have to reimplement that using tornado.
Besides that, I don't see other problems with this. @aashish24 @jbeezley any thoughts?

An additional question: You seems to have only two vtkweb examples, and the projection for the isofill is changed. Why is that? Note the info should work for any projection.

@chaosphere2112
Copy link
Contributor Author

@danlipsa There are 3 VTK examples (the 3D one in the bottom right is also VTK); the top left one is a boxfill, because that was the first object I dumped to JSON in the demo (this is actually true already in the demo.js). It's pretty easy to convert back to isofill with a projection; just need to change the g_name and projection in the JSON.

@jbeezley
Copy link
Contributor

We had an issue before with plots not appearing that was fixed by Kitware/VTK@1ff973c. I don't know if this is related.

The pv server shutting down after a few minutes is intentional and necessary. For simplicity, I didn't include the session launcher in this example. In a real deployment, you have to do something more like what was done in the old cdatweb. Each user spawns a new server instance that must be cleaned up when the session ends. Unfortunately, there is no 100% reliable way to determine when a user disconnects, so the server has to kill itself eventually. For development purposes, it would be useful to be able to create a server that stays open. The timeout logic is here if anyone cares enough to make a PR.

@chaosphere2112
Copy link
Contributor Author

@aashish24
Copy link

@chaosphere2112 can we talk about it tuesday of next week?

Few questions:

  1. Looking at the code it seems like animation support is missing.
  2. I think picking is working but can we move slice planes - Just give it a try, it may work
  3. Also, try with some larget dataset (something real and not clt) to compare the performance.

@aashish24
Copy link

@chaosphere2112 I like it but I have some concerns specially dealing with deployment, security and moving this code from demo to a production quality. Let's talk next week.

Thanks,

@chaosphere2112
Copy link
Contributor Author

@aashish24 I'm going to be kind of crazy next week; can we book something for after the ACME meeting (the week of the 14th)? I'm not in any rush to get this merged; I definitely want to allay any concerns you or anyone else may have about it.

@aashish24
Copy link

@aashish24 I'm going to be kind of crazy next week; can we book something for after the ACME meeting (the week of the 14th)?

Sure, thanks, if we are not in hurry to merge this one in, we can meet in the week of 14th. I can setup a gotomeeting. Thanks.

@danlipsa
Copy link
Contributor

danlipsa commented Nov 2, 2016

@chaosphere2112 Do your plot windows get closed in vcs-js / vcdat? See CDAT/vcs#21. This issue has to do with the interactor set on the window - but you may have a different way of rewriting the script in the issue so that the windows close.

@chaosphere2112
Copy link
Contributor Author

Just using canvas.close() seems to work for me; there must have been a reference held onto somewhere inside our use of PVW. Or it could be a windowing environment thing (since I'm testing on Mac).

@danlipsa
Copy link
Contributor

danlipsa commented Nov 2, 2016

@chaosphere2112 I remember when I run on my laptop windows do not show up for the server, the way they do on linux. So, then how do you know they are really closed?

It may be also a windowing thing. Our interactor code is also very different on mac.

@chaosphere2112
Copy link
Contributor Author

chaosphere2112 commented Nov 2, 2016

I’ve done some work on making .close() work properly in the past; as I recall, it was due to references to the canvas itself being stuck around. In reality, we should probably just get rid of the reference to the RenderWindow when we call close(); that will also make it a lot easier to do things like reopen a closed canvas, which just totally fails right now.

@danlipsa
Copy link
Contributor

danlipsa commented Nov 2, 2016

@chaosphere2112 This is related. The interactor makes the render window stay around. close() does remove the reference to render window.

@aashish24
Copy link

@danlipsa @chaosphere2112 as I mentioned earlier, lets get this branch merged. I looked at the changes and apart from minor concerns (which we can address in future PR's).

@danlipsa if you don't have any major concerns, okay if @chaosphere2112 merges this branch?

@danlipsa
Copy link
Contributor

@aashish24 @chaosphere2112 Yes, lets merge this branch.

@chaosphere2112 chaosphere2112 merged commit 592d30a into master Jan 11, 2017
danlipsa added a commit that referenced this pull request Aug 17, 2017
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.

4 participants