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

Render arbitrarily large collections in the UI. #5091

Merged
merged 9 commits into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@jmchilton
Copy link
Member

jmchilton commented Nov 29, 2017

This builds on #5038 (render arbitrarily nested lists in the UI) and fixes #3795 (which is both a usability problem with large collections and serious backend memory issue). Only around 1000 elements of a collection will be rendered now and these will be fetched much more efficiently. It'd be nice if we could paginate or progressively fetch more elements beyond that - but that should probably wait for a major rewrite and be tied with search and such to make navigation of such collection possible.

This problem is tackled along three prongs.

  1. Reduces the transfer size, number of database fetches, and number of database joins to pull out the history collection structure for rendering a collection in the UI. a475f9c
  2. Introduces the concept of "fuzzy count" limit when fetching large collections - to limit the order-of-magnitude-per-level-of-depth of the collection fetched via the API while still fetching the collection at every level of depth for nested collections. 62c3e9c
  3. Updates the UI for more consistent collection subtitles, display of element counts, and warns the user if only a subset of elements are being display.

This screenshot generated on Jenkins from this PR is of a collection that is rendering a more limited number of elements (the test case overrides the client to reduce the fuzzy count limit from 1000 to 2 to verify the limiting works).

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Nov 29, 2017

A cool, unintended upshot of this is that when the long standing issue #3782 occurs and the UI just decides not to render some collection elements - there is now a warning that this occurring. Still disturbing that it happens at all - but better there is some feedback.

Here is a screenshot from a test case that exhibits this on the Selenium server.

Prior to this PR that failure would look like this - just dropping an element without a warning.

@jmchilton jmchilton force-pushed the jmchilton:open_large_collections branch 2 times, most recently from f284c49 to 629750b Nov 30, 2017

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Nov 30, 2017

Mayhem! :)
screenshot 2017-11-30 15 21 20

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Nov 30, 2017

Meh - you say mayhem - I say if people who have "list:list:paired:paired:paired:paired:paired" data structures can now see their data 😆.

Update: For people concerns about this - it is limited to the multi-history view right? You can descend more cleanly in your own active history in the analysis view.

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Nov 30, 2017

@jmchilton your update is correct, sorry I did not clarify

jmchilton added some commits Nov 4, 2017

Optimize history collection response.
Introduce a new collection view type (between view="collection" and view="element") called "element-reference" that fetches the element structure of a collection and enough information to resolve references for leaf datasets but doesn't include details that aren't used in rendering the structure of the collection in the GUI. Prevents transmitting metadata, dataset name (since element identifier is used), info, peek, dbkey, etc....

Backbone and the rest of the infrastructure seems to just work and fills in all remaining details when datasets are actually expanded by users.

Optimization mentioned in #3795 - mitigates the issue (hopefully to large degree - but doesn't fix it).

@jmchilton jmchilton force-pushed the jmchilton:open_large_collections branch from 629750b to 94a5cc1 Nov 30, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Nov 30, 2017

Rebased.

Clarified the concept of the fuzzy_count a bit in the above description. But here is the chat I had with @martenson on it in Gitter.

Martin Cech @martenson 15:25
@jmchilton how many collection elements do you expect to see with fuzzy_count=10?

John Chilton @jmchilton 15:25
in a list?

Martin Cech @martenson 15:25
deeply nested lists
I thought the idea of fuzzy count was to limit the absolute number of elements fetched no matter the depth

John Chilton @jmchilton 15:26
like "list:list" or more deeply?

Martin Cech @martenson 15:26
10 levels of depth

John Chilton @jmchilton 15:27
you need to stop creating the data structures!

Martin Cech @martenson 15:27
hey, just testing mate :wink:

John Chilton @jmchilton 15:27
umm... I'm not sure - I guess I would expect to see just one element at every level
It isn't a hard limit for sure - it meant to be a sort of "within an order of magnitude" limit

Martin Cech @martenson 15:28
https://gist.github.com/martenson/8cda8a40ca04e6e124d991fd1cdf35c6
(the request is in the end of gist)

John Chilton @jmchilton 15:29
well with paired stuff in there - I would expect it to grab one pair from each level of list and both elements of each pair
which... it seems to be doing?

Martin Cech @martenson 15:30
I just expected less than 449 elements for fuzzy_count=10
this is probably hard cornercase though

John Chilton @jmchilton 15:32
So it is always going to grab a full pair - since 2 isn't an order or magnitude more than 1... but if it does that at every level of depth - you can see how this would happen. Is it at least just grabbing one element or the top list?

Martin Cech @martenson 15:33
yeah, it seems to be exact third of the original elements
(that are shown when there is no limit)

John Chilton @jmchilton 15:34
I wouldn't expect anyone to ever have a list:paired:paired or anything but one paired at the end of these chains - so I wouldn't expect grabbing both parts of the paired to affect the total count more by more than a factor of 2. Clearly it can though. I still think I'm doing the right thing - I just need to explain it better-er

Martin Cech @martenson 15:40
That makes sense.

John Chilton @jmchilton 15:40
Does it though? I've reworded the PR to claim it restricts the order-of-magnitude-per-level-of-depth instead of just the order of magnitude

Martin Cech @martenson 15:41
It does, until we have more structure than list and pair
then we should revisit
But that is kinbda obvious.

John Chilton @jmchilton 15:44
Also in its defense 449 seems big - but if you had 1000 elements in that top list - it would still only grab one of those crazy pairs in that large list and it would left hundreds of thousands of datasets on the server - and the 2^{number of nested pairs} would more closely approximate the small positive constant I was sort of claiming it to be.
yes - I have plans for records - but I'm probably going to just approximate them as lists for this calculation

@martenson martenson merged commit dcd7e78 into galaxyproject:dev Dec 1, 2017

7 checks passed

api test Build finished. 331 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 164 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 59 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript 1 new alert
Details
selenium test Build finished. 117 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Dec 1, 2017

Sorry to be late to the game here, but could we have the white square around the elements always go through to the right side ? That would give us more screen space to display buttons

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Dec 1, 2017

@mvdbeek It is certainly a good plan - I didn't actually touch the style sheet at all in the PR it just sort of magically worked. We should take a stab at cleaning that up a bit though - I agree. Eliminating the right padding/margin is a good idea. Maybe alternating the color of that white border as we go deeper would be a good idea also for more visual distinction between the levels.

I honestly wasn't very familiar with this view of collections at all - I've never really used them with the multi-history panel. Do you think it would be good to at least have an option to navigate collections this way in the main history panel? Like throw a + button in the view somewhere to expand them in an embedded way instead of in the digging deeper way? (Now that I've actually touched all that code it seems like that would be an easy thing to implement if it would be useful.) Not sure it is worth the added complexity / visual clutter though.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Dec 1, 2017

Do you think it would be good to at least have an option to navigate collections this way in the main history panel?

You mean expanding collections in the single-history view like we do in the multi-history view ? That would certainly make drag-and-drop of collection elements easier, but also scanning collections for elements would be a easier that way.

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Dec 1, 2017

@mvdbeek Expanding subthings in a thing that already expands needs to be very careful. There is not much canvas to use and UX might easily downgrade.

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Dec 1, 2017

@mvdbeek This is what I meant.

@martenson Is there something about the multi-history view lends it to being okay that the main history view doesn't allow?

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Dec 1, 2017

I think it is confusing it behaves differently in two very similar interfaces. IMHO the way to go is to improve on what is in multi_view and drop the full drilldown that we use in history view currently (ie using the expanding collections everywhere).

edit: now when I read this thread again I think we totally agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.