Skip to content

Commit

Permalink
Improve performance bugs and misleading summary info in history list.
Browse files Browse the repository at this point in the history
Prefetch shared user information as counts instead of objects and do so in initial query to eliminate an extra 15 SQL queries per page and load less data related to sharing.

Replace columns that would cause HDA information to be joined into the query (history size and HDA state counts) with a spinner that will be fetched in subsequent queries on the client end. There can hundreds of thousands of datasets per history - this information shouldn't be summarized to get the initial page to render - it can be fetched one history at a time once the page is rendered.

This commit also adds a new column "Items" that corresponds to the next HID - and that I think is a good summary of the "history size" before the dataset state information is loaded and even gives additional information because that count includes collections. This also renders state information for deleted and hidden datasets that was previously missing and could cause confusion. That said I don't like the dataset summaries - I'd rather just have the item count and then job state summaries.
  • Loading branch information
jmchilton committed Feb 14, 2018
1 parent c20b68a commit 283ab04
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 31 deletions.
51 changes: 50 additions & 1 deletion client/galaxy/scripts/mvc/history/history-list.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,60 @@
import _l from "utils/localization";
/** This class renders the grid list. */
import AjaxQueue from "utils/ajax-queue";
import Utils from "utils/utils";
/** This class renders the grid list. */
import GridView from "mvc/grid/grid-view";
import HistoryModel from "mvc/history/history-model";
import historyCopyDialog from "mvc/history/copy-dialog";

var HistoryGridView = GridView.extend({
initialize: function(grid_config) {
this.ajaxQueue = new AjaxQueue.AjaxQueue();
GridView.prototype.initialize.call(this, grid_config);
},

init_grid_elements: function() {
const ajaxQueue = this.ajaxQueue;
ajaxQueue.stop();
GridView.prototype.init_grid_elements.call(this);
const fetchDetails = $.makeArray(
this.$el.find(".delayed-value-datasets_by_state").map((i, el) => {
return () => {
const historyId = $(el).data("history-id");
const url = `${
Galaxy.root
}api/histories/${historyId}?keys=nice_size,contents_active,contents_states`;
const options = {};
options.url = url;
options.type = "GET";
options.success = req => {
const contentsStates = req["contents_states"];
let stateHtml = "";
for (let state of ["ok", "running", "queued", "new", "error"]) {
const stateCount = contentsStates[state];
if (stateCount) {
stateHtml += `<div class="count-box state-color-${state}">${stateCount}</div> `;
}
}
const contentsActive = req["contents_active"];
const deleted = contentsActive["deleted"];
if (deleted) {
stateHtml += `<div class="count-box state-color-deleted" title="Number of datasets that are deleted.">${deleted}</div> `;
}
const hidden = contentsActive["hidden"];
if (hidden) {
stateHtml += `<div class="count-box state-color-hidden" title="Number of datasets that are marked as hidden.">${hidden}</div> `;
}
$(`.delayed-value-datasets_by_state[data-history-id='${historyId}']`).html(stateHtml);
$(`.delayed-value-disk_size[data-history-id='${historyId}']`).html(req["nice_size"]);
};
var xhr = jQuery.ajax(options);
return xhr;
};
})
);
fetchDetails.forEach(fn => ajaxQueue.add(fn));
ajaxQueue.start();
},
_showCopyDialog: function(id) {
var history = new HistoryModel.History({ id: id });
history
Expand Down
6 changes: 6 additions & 0 deletions client/galaxy/style/less/base.less
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,12 @@ ul.manage-table-actions li {
background: @state-deleted-bg;
}

.state-color-hidden {
border-color: @state-default-border;
border-style: dotted;
background: @state-default-bg;
}

.state-fg-new {
color: #FFB030;
}
Expand Down
4 changes: 4 additions & 0 deletions lib/galaxy/model/mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -1574,6 +1574,10 @@ def simple_mapping(model, **kwds):
average_rating=column_property(
select([func.avg(model.HistoryRatingAssociation.table.c.rating)]).where(model.HistoryRatingAssociation.table.c.history_id == model.History.table.c.id),
deferred=True
),
users_shared_with_count=column_property(
select([func.count(model.HistoryUserShareAssociation.table.c.id)]).where(model.History.table.c.id == model.HistoryUserShareAssociation.table.c.history_id),
deferred=True
)
))

Expand Down
16 changes: 14 additions & 2 deletions lib/galaxy/web/framework/helpers/grids.py
Original file line number Diff line number Diff line change
Expand Up @@ -822,22 +822,34 @@ def get_accepted_filters(self):
class SharingStatusColumn(GridColumn):
""" Grid column to indicate sharing status. """

def __init__(self, *args, **kwargs):
self.use_shared_with_count = kwargs.pop("use_shared_with_count", False)
super(SharingStatusColumn, self).__init__(*args, **kwargs)

def get_value(self, trans, grid, item):
# Delete items cannot be shared.
if item.deleted:
return ""
# Build a list of sharing for this item.
sharing_statuses = []
if item.users_shared_with:
if self._is_shared(item):
sharing_statuses.append("Shared")
if item.importable:
sharing_statuses.append("Accessible")
if item.published:
sharing_statuses.append("Published")
return ", ".join(sharing_statuses)

def _is_shared(self, item):
if self.use_shared_with_count:
# optimization to skip join for users_shared_with and loading in that data.
return item.users_shared_with_count > 0

return item.users_shared_with

def get_link(self, trans, grid, item):
if not item.deleted and (item.users_shared_with or item.importable or item.published):
is_shared = self._is_shared(item)
if not item.deleted and (is_shared or item.importable or item.published):
return dict(operation="share or publish", id=item.id)
return None

Expand Down
46 changes: 19 additions & 27 deletions lib/galaxy/webapps/galaxy/controllers/history.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from markupsafe import escape
from six import string_types
from six.moves.urllib.parse import unquote_plus
from sqlalchemy import and_, false, func, null, true
from sqlalchemy import and_, false, null, true
from sqlalchemy.orm import eagerload, eagerload_all, undefer

import galaxy.util
Expand All @@ -16,7 +16,7 @@
UsesAnnotations,
UsesItemRatings
)
from galaxy.util import listify, nice_size, Params, parse_int, sanitize_text
from galaxy.util import listify, Params, parse_int, sanitize_text
from galaxy.util.odict import odict
from galaxy.util.sanitize_html import sanitize_html
from galaxy.web import url_for
Expand Down Expand Up @@ -44,28 +44,13 @@ def get_value(self, trans, grid, history):
class HistoryListGrid(grids.Grid):

# Custom column types
class DatasetsByStateColumn(grids.GridColumn):
class DelayedValueColumn(grids.GridColumn):
def get_value(self, trans, grid, history):
# States to show in column.
states_to_show = ('ok', 'running', 'queued', 'new', 'error')

# Get dataset counts for each state in a state-count dictionary.
state_counts = dict((state, count) for state, count in
trans.sa_session.query(model.Dataset.state, func.count(model.Dataset.state))
.join(model.HistoryDatasetAssociation)
.group_by(model.Dataset.state)
.filter(model.HistoryDatasetAssociation.history_id == history.id,
model.HistoryDatasetAssociation.visible == true(),
model.HistoryDatasetAssociation.deleted == false(),
model.Dataset.state.in_(states_to_show)))

# Create HTML.
rval = ''
for state in states_to_show:
count = state_counts.get(state)
if count:
rval += '<div class="count-box state-color-%s">%s</div> ' % (state, count)
return rval
return '<div class="delayed-value-%s" data-history-id="%s"><span class="fa fa-spinner fa-spin"></span></div>' % (self.key, trans.security.encode_id(history.id))

class ItemCountColumn(grids.GridColumn):
def get_value(self, trans, grid, history):
return str(history.hid_counter - 1)

class HistoryListNameColumn(NameColumn):
def get_link(self, trans, grid, history):
Expand All @@ -91,25 +76,32 @@ def sort(self, trans, query, ascending, column_name=None):
query = query.order_by(self.model_class.table.c.purged.desc(), self.model_class.table.c.update_time.desc())
return query

def build_initial_query(self, trans, **kwargs):
# Override to preload sharing information used when fetching data for grid.
query = super(HistoryListGrid, self).build_initial_query(trans, **kwargs)
query = query.options(undefer("users_shared_with_count"))
return query

# Grid definition
title = "Saved Histories"
model_class = model.History
default_sort_key = "-update_time"
columns = [
HistoryListNameColumn("Name", key="name", attach_popup=True, filterable="advanced"),
DatasetsByStateColumn("Datasets", key="datasets_by_state", sortable=False, nowrap=True),
ItemCountColumn("Items", key="item_count", sortable=False),
DelayedValueColumn("Datasets", key="datasets_by_state", sortable=False, nowrap=True),
grids.IndividualTagsColumn("Tags", key="tags", model_tag_association_class=model.HistoryTagAssociation,
filterable="advanced", grid_name="HistoryListGrid"),
grids.SharingStatusColumn("Sharing", key="sharing", filterable="advanced", sortable=False),
grids.GridColumn("Size on Disk", key="disk_size", format=nice_size, sortable=False),
grids.SharingStatusColumn("Sharing", key="sharing", filterable="advanced", sortable=False, use_shared_with_count=True),
DelayedValueColumn("Size on Disk", key="disk_size", sortable=False),
grids.GridColumn("Created", key="create_time", format=time_ago),
grids.GridColumn("Last Updated", key="update_time", format=time_ago),
DeletedColumn("Status", key="deleted", filterable="advanced")
]
columns.append(
grids.MulticolFilterColumn(
"search history names and tags",
cols_to_filter=[columns[0], columns[2]],
cols_to_filter=[columns[0], columns[3]],
key="free-text-search", visible=False, filterable="standard")
)
operations = [
Expand Down
2 changes: 1 addition & 1 deletion test/selenium_tests/test_saved_histories.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ def get_history_tags_cell(self, history_name):
for row in grid.find_elements_by_tag_name('tr'):
td = row.find_elements_by_tag_name('td')
if td[1].text == history_name:
tags_cell = td[3]
tags_cell = td[4]
break

if tags_cell is None:
Expand Down

0 comments on commit 283ab04

Please sign in to comment.