Skip to content

data-loader-behavior: add fine-grained batching support #4045

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.

Sign up for GitHub

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

Merged
wchargin merged 2 commits into from
Aug 19, 2020
Merged

data-loader-behavior: add fine-grained batching support #4045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
065d7ca
data-loader-behavior: add fine-grained batching support
wchargin Aug 18, 2020
e8bf2fe
[update patch]
wchargin Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions tensorboard/components_polymer3/tf_backend/canceller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

export interface CancelResult<T> {
value: T;
cancelled: boolean;
}

/**
* A class that allows marking promises as cancelled.
*
Expand Down Expand Up @@ -45,9 +50,7 @@ export class Canceller {
* a `cancelled` argument. This argument will be `false` unless and
* until `cancelAll` is invoked after the creation of this task.
*/
public cancellable<T, U>(
f: (result: {value: T; cancelled: boolean}) => U
): (T) => U {
public cancellable<T, U>(f: (result: CancelResult<T>) => U): (T) => U {
const originalCancellationCount = this.cancellationCount;
return (value) => {
const cancelled = this.cancellationCount !== originalCancellationCount;
Expand Down
147 changes: 68 additions & 79 deletions tensorboard/components_polymer3/tf_dashboard_common/data-loader-behavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ limitations under the License.
import {PolymerElement} from '@polymer/polymer';
import * as _ from 'lodash';

import {Canceller} from '../tf_backend/canceller';
import {CancelResult, Canceller} from '../tf_backend/canceller';
import {RequestManager} from '../tf_backend/requestManager';

type CacheKey = string;
Expand All @@ -34,6 +34,23 @@ export interface DataLoaderBehaviorInterface<Item, Data>
dataToLoad: Item[];
}

// A function that takes a list of items and asynchronously fetches the
// data for those items. As each item loads, it should invoke the
// `onLoad` callback with an `{item, data}` pair to update the cache.
// After all items have finished loading, it should invoke the
// `onFinish` callback. Conceptually, that this function accepts
// `onLoad` and `onFinish` as arguments is as if it returned an
// Observable-style stream of `{item, data}`-pairs, CPS-transformed.
//
// Used in `DataLoaderBehavior.requestData`.
export interface RequestDataCallback<Item, Data> {
(
items: Item[],
onLoad: (kv: {item: Item; data: Data}) => void,
onFinish: () => void
): void;
}

export function DataLoaderBehavior<Item, Data>(
superClass: new () => PolymerElement
): new () => DataLoaderBehaviorInterface<Item, Data> {
Expand All @@ -47,16 +64,16 @@ export function DataLoaderBehavior<Item, Data>(
*/
loadKey = '';

// List of data to be loaded. By default, a datum is passed to
// List of items to be loaded. By default, items are passed to
// `requestData` to fetch data. When the request resolves, invokes
// `loadDataCallback` with the datum and its response.
dataToLoad: Item[] = [];

/**
* A function that takes a datum as an input and returns a unique
* A function that takes an item as an input and returns a unique
* identifiable string. Used for caching purposes.
*/
getDataLoadName = (datum: Item): CacheKey => String(datum);
getDataLoadName = (item: Item): CacheKey => String(item);

/**
* A function that takes as inputs:
Expand All @@ -66,28 +83,11 @@ export function DataLoaderBehavior<Item, Data>(
* This function will be called when a response from a request to that
* data URL is successfully received.
*/
loadDataCallback!: (component: this, datum: Item, data: Data) => void;

public requestManager!: RequestManager;

// A function that takes a datum as argument and makes the HTTP
// request to fetch the data associated with the datum. It should return
// a promise that either fullfills with the data or rejects with an error.
// If the function doesn't bind 'this', then it will reference the element
// that includes this behavior.
// The default implementation calls this.requestManager.request with
// the value returned by this.getDataLoadUrl(datum) (see below).
// The only place getDataLoadUrl() is called is in the default
// implementation of this method. So if you override this method with
// an implementation that doesn't call getDataLoadUrl, it need not be
// provided.
requestData = (datum: Item) => {
return this.requestManager.request(this.getDataLoadUrl(datum));
};

// A function that takes a datum and returns a string URL for fetching
// data.
getDataLoadUrl!: (datum: Item) => string;
loadDataCallback!: (component: this, item: Item, data: Data) => void;
Copy link
Contributor

@stephanwlee stephanwlee Aug 18, 2020

Choose a reason for hiding this comment

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

Do you think there is a benefit in "batching" this API, too? i.e., call the callback once per all items.

Copy link
Contributor Author

@wchargin wchargin Aug 18, 2020

Choose a reason for hiding this comment

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

Good question. I considered it, and thought that the commitChanges
batching that we already do would be sufficient (for scalars, and it’s a
non-issue for histograms/distributions). But on further inspection I see
that the data load-data callback actually calls commitChanges every
time. I feel like I’m missing something; what’s the point of the
batching then?

I’m happy to consider adding batching here, but by default I’ll probably
opt to defer that into another PR if we decide that it’s worth doing.
Could be convinced otherwise, though.

Copy link
Contributor

@stephanwlee stephanwlee Aug 19, 2020

Choose a reason for hiding this comment

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

Ah, I think I made a mistake. I definitely meant to only commit the changes on onLoadFinish. I will send a separate PR shortly.

Copy link
Contributor Author

@wchargin wchargin Aug 19, 2020

Choose a reason for hiding this comment

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

Forgot to comment here: I did play around with this and everything still
works, and I see a significant improvement in paint time.

It’d be slightly easier for me if I could send that PR after these two
land, just so that I don’t have to futz with conflicts; is that okay
with you?

Copy link
Contributor

@stephanwlee stephanwlee Aug 19, 2020

Choose a reason for hiding this comment

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

Awesome. It is awesome that it improves the paint time, too.

Please feel free to work on it.


// Function that actually loads data from the network. See docs on
// `RequestDataCallback` for details.
requestData: RequestDataCallback<Item, Data>;

dataLoading = false;

Expand Down Expand Up @@ -192,62 +192,51 @@ export function DataLoaderBehavior<Item, Data>(
if (result.cancelled) {
return;
}
// Read-only property have a special setter.
this.dataLoading = true;
// Promises return cacheKeys of the data that were fetched.
const promises = this.dataToLoad
.filter((datum) => {
const cacheKey = this.getDataLoadName(datum);
return !this._dataLoadState.has(cacheKey);
})
.map((datum) => {
const cacheKey = this.getDataLoadName(datum);
this._dataLoadState.set(cacheKey, LoadState.LOADING);
return this.requestData(datum).then(
this._canceller.cancellable((result) => {
// It was resetted. Do not notify of the response.
if (!result.cancelled) {
this._dataLoadState.set(cacheKey, LoadState.LOADED);
this.loadDataCallback(this, datum, result.value as any);
}
return cacheKey;
})
);
});
return Promise.all(promises)
.then(
this._canceller.cancellable((result) => {
// It was resetted. Do not notify of the data load.
if (!result.cancelled) {
const keysFetched = result.value as any;
const fetched = new Set(keysFetched);
const shouldNotify = this.dataToLoad.some((datum) =>
fetched.has(this.getDataLoadName(datum))
);
if (shouldNotify) {
this.onLoadFinish();
}
}
const isDataFetchPending = Array.from(
this._dataLoadState.values()
).some((loadState) => loadState === LoadState.LOADING);
if (!isDataFetchPending) {
// Read-only property have a special setter.
this.dataLoading = false;
}
}),
// TODO(stephanwlee): remove me when we can use
// Promise.prototype.finally instead
() => {}
)
.then(
this._canceller.cancellable(({cancelled}) => {
if (cancelled) {
return;
const dirtyItems = this.dataToLoad.filter((datum) => {
Copy link
Contributor

@stephanwlee stephanwlee Aug 18, 2020

Choose a reason for hiding this comment

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

Sorry about pre-existing naming I created. Can we, in a follow up, rename these (or create an alias) to say item instead of data? i.e., we use both right now with dataToLoad, getDataLoadName, and dataLoadState but use item in requestData`.

Copy link
Contributor Author

@wchargin wchargin Aug 18, 2020

Choose a reason for hiding this comment

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

np :-) Yeah, I was planning to rename them to just K and V
throughout (with a slight potential point of confusion around the cache
keys, but I think that (K) => string is a pretty self-explanatory
signature). In this PR, I cleaned up a couple that I was touching
anyway, but tried to avoid changing the load-bearing API names.
Definitely happy to follow up. Could also do that as pre-work, though
I’m not super inclined to surgere that at this point.

Copy link
Contributor

@stephanwlee stephanwlee Aug 18, 2020

Choose a reason for hiding this comment

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

I do not remember your plan so I will ask here: what is our plan with the maximum number of runs we batch?

On the reload, we expunge the cache (maybe we should only expunge already loaded so we do not double fetch) and invoke _loadData. If user has thousands of runs, one batch request can ask for thousand entries which I don't think will scale nicely (if you want some anecdote, please ask offline). Do we plan on breaking the request down into chunks even in this model?

Copy link
Contributor Author

@wchargin wchargin Aug 18, 2020

Choose a reason for hiding this comment

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

Yeah, fair question. The data loader behavior shouldn’t impose a max
batch size, but I was considering adding a batch size of ~100 in the
requestData implementation that calls /scalars_multirun. This is
just based off some napkin math: per time series, 1000 points, with
JSON-encoded wall time (~18 bytes), step (~5 bytes), value (~18 bytes),
and overhead (~8 bytes), for about 48 bytes per datum and thus 4.8 KB
per time series. A response size of 500 KB seems entirely reasonable,
and capping it somewhere seems wise to avoid unbounded server load.

Do we plan on breaking the request down into chunks even in this
model?

I think that it’s fine to call requestData with all the items and let
requestData decide if/how to break that up into smaller requests.

Copy link
Contributor

@stephanwlee stephanwlee Aug 19, 2020

Choose a reason for hiding this comment

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

That plan sounds good. Thanks for sharing your plans.

const cacheKey = this.getDataLoadName(datum);
return !this._dataLoadState.has(cacheKey);
});
for (const item of dirtyItems) {
const cacheKey = this.getDataLoadName(item);
this._dataLoadState.set(cacheKey, LoadState.LOADING);
}
const onLoad = this._canceller.cancellable(
(result: CancelResult<{item: Item; data: Data}>) => {
if (result.cancelled) {
return;
}
const {item, data} = result.value;
const cacheKey = this.getDataLoadName(item);
this._dataLoadState.set(cacheKey, LoadState.LOADED);
this.loadDataCallback(this, item, data);
}
);
const onFinish = this._canceller.cancellable(
(result: CancelResult<void>) => {
// Only notify of data load if the load was not cancelled.
if (!result.cancelled) {
const keysFetched = result.value as any;
const fetched = new Set(
dirtyItems.map((item) => this.getDataLoadName(item))
);
const shouldNotify = this.dataToLoad.some((datum) =>
fetched.has(this.getDataLoadName(datum))
);
if (shouldNotify) {
this.onLoadFinish();
}
this._loadDataAsync = null;
})
);
}
const isDataFetchPending = Array.from(
this._dataLoadState.values()
).includes(LoadState.LOADING);
if (!isDataFetchPending) {
this.dataLoading = false;
}
}
);
this.requestData(dirtyItems, onLoad, () => onFinish(undefined));
})
);
}
Expand Down
36 changes: 28 additions & 8 deletions ...s/custom_scalar/polymer3/tf_custom_scalar_dashboard/tf-custom-scalar-margin-chart-card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ import * as _ from 'lodash';
import {DomRepeat} from '../../../../components_polymer3/polymer/dom-repeat';
import '../../../../components_polymer3/polymer/irons_and_papers';
import {LegacyElementMixin} from '../../../../components_polymer3/polymer/legacy_element_mixin';
import {RequestManager} from '../../../../components_polymer3/tf_backend/requestManager';
import {getRouter} from '../../../../components_polymer3/tf_backend/router';
import {addParams} from '../../../../components_polymer3/tf_backend/urlPathHelpers';
import '../../../../components_polymer3/tf_card_heading/tf-card-heading';
import {RequestDataCallback} from '../../../../components_polymer3/tf_dashboard_common/data-loader-behavior';
import {runsColorScale} from '../../../../components_polymer3/tf_color_scale/colorScale';
import '../../../../components_polymer3/tf_line_chart_data_loader/tf-line-chart-data-loader';
import {TfLineChartDataLoader} from '../../../../components_polymer3/tf_line_chart_data_loader/tf-line-chart-data-loader';
import {
SYMBOLS_LIST,
ScalarDatum,
Y_TOOLTIP_FORMATTER_PRECISION,
multiscaleFormatter,
relativeAccessor,
Expand Down Expand Up @@ -57,6 +60,12 @@ interface StepsMismatch {
seriesObject: MarginChartSeries;
}

type RunItem = string;
type CustomScalarsDatum = {
regex_valid: boolean;
tag_to_events: Record<string, ScalarDatum[]>;
};

export interface TfCustomScalarMarginChartCard extends HTMLElement {
reload(): void;
}
Expand All @@ -72,11 +81,11 @@ class _TfCustomScalarMarginChartCard extends LegacyElementMixin(PolymerElement)
active="[[active]]"
color-scale="[[_colorScale]]"
data-series="[[_seriesNames]]"
get-data-load-url="[[_dataUrl]]"
fill-area="[[_fillArea]]"
ignore-y-outliers="[[ignoreYOutliers]]"
load-key="[[_tagFilter]]"
data-to-load="[[runs]]"
request-data="[[_requestData]]"
log-scale-active="[[_logScaleActive]]"
load-data-callback="[[_createProcessDataFunction(marginChartSeries)]]"
request-manager="[[requestManager]]"
Expand Down Expand Up @@ -315,7 +324,7 @@ class _TfCustomScalarMarginChartCard extends LegacyElementMixin(PolymerElement)
ignoreYOutliers: boolean;

@property({type: Object})
requestManager: object;
requestManager: RequestManager;

@property({type: Boolean})
showDownloadLinks: boolean;
Expand Down Expand Up @@ -347,12 +356,23 @@ class _TfCustomScalarMarginChartCard extends LegacyElementMixin(PolymerElement)
_logScaleActive: boolean;

@property({type: Object})
_dataUrl: (run: string) => string = (run) => {
const tag = this._tagFilter;
return addParams(getRouter().pluginRoute('custom_scalars', '/scalars'), {
tag,
run,
});
_requestData: RequestDataCallback<RunItem, CustomScalarsDatum> = (
items,
onLoad,
onFinish
) => {
const router = getRouter();
const baseUrl = router.pluginRoute('custom_scalars', '/scalars');
Promise.all(
items.map((item) => {
const run = item;
const tag = this._tagFilter;
const url = addParams(baseUrl, {tag, run});
return this.requestManager
.request(url)
.then((data) => void onLoad({item, data}));
})
).finally(() => void onFinish());
};

@property({type: Object})
Expand Down
37 changes: 29 additions & 8 deletions ...stom_scalar/polymer3/tf_custom_scalar_dashboard/tf-custom-scalar-multi-line-chart-card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,14 @@ import {getRouter} from '../../../../components_polymer3/tf_backend/router';
import '../../../../components_polymer3/tf_backend/tf-backend';
import {addParams} from '../../../../components_polymer3/tf_backend/urlPathHelpers';
import '../../../../components_polymer3/tf_card_heading/tf-card-heading';
import {RequestDataCallback} from '../../../../components_polymer3/tf_dashboard_common/data-loader-behavior';
import {runsColorScale} from '../../../../components_polymer3/tf_color_scale/colorScale';
import '../../../../components_polymer3/tf_line_chart_data_loader/tf-line-chart-data-loader';
import {TfLineChartDataLoader} from '../../../../components_polymer3/tf_line_chart_data_loader/tf-line-chart-data-loader';
import {SYMBOLS_LIST} from '../../../../components_polymer3/vz_chart_helpers/vz-chart-helpers';
import {
SYMBOLS_LIST,
ScalarDatum,
} from '../../../../components_polymer3/vz_chart_helpers/vz-chart-helpers';
import './tf-custom-scalar-card-style';
import {
DataSeries,
Expand All @@ -40,6 +44,12 @@ export interface TfCustomScalarMultiLineChartCard extends HTMLElement {
reload(): void;
}

type RunItem = string;
type CustomScalarsDatum = {
regex_valid: boolean;
tag_to_events: Record<string, ScalarDatum[]>;
};

@customElement('tf-custom-scalar-multi-line-chart-card')
class _TfCustomScalarMultiLineChartCard
extends LegacyElementMixin(PolymerElement)
Expand All @@ -52,10 +62,10 @@ class _TfCustomScalarMultiLineChartCard
active="[[active]]"
color-scale="[[_colorScale]]"
data-series="[[_seriesNames]]"
get-data-load-url="[[_dataUrl]]"
ignore-y-outliers="[[ignoreYOutliers]]"
load-key="[[_tagFilter]]"
data-to-load="[[runs]]"
request-data="[[_requestData]]"
log-scale-active="[[_logScaleActive]]"
load-data-callback="[[_createProcessDataFunction()]]"
request-manager="[[requestManager]]"
Expand Down Expand Up @@ -235,12 +245,23 @@ class _TfCustomScalarMultiLineChartCard
_logScaleActive: boolean;

@property({type: Object})
_dataUrl: (run: string) => string = (run) => {
const tag = this._tagFilter;
return addParams(getRouter().pluginRoute('custom_scalars', '/scalars'), {
tag,
run,
});
_requestData: RequestDataCallback<RunItem, CustomScalarsDatum> = (
items,
onLoad,
onFinish
) => {
const router = getRouter();
const baseUrl = router.pluginRoute('custom_scalars', '/scalars');
Promise.all(
items.map((item) => {
const run = item;
const tag = this._tagFilter;
const url = addParams(baseUrl, {tag, run});
return this.requestManager
.request(url)
.then((data) => void onLoad({item, data}));
})
).finally(() => void onFinish());
};

@property({type: Object})
Expand Down
Loading