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

Time Series: preserve run regex filter string in URL #5412

Merged
merged 5 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
12 changes: 12 additions & 0 deletions tensorboard/webapp/routes/dashboard_deeplink_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
RUN_COLOR_GROUP_KEY,
DeserializedState,
PINNED_CARDS_KEY,
RUN_REGEX_FILTER_KEY,
SMOOTHING_KEY,
TAG_FILTER_KEY,
} from './dashboard_deeplink_provider_types';
Expand Down Expand Up @@ -162,6 +163,12 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
return [{key: RUN_COLOR_GROUP_KEY, value}];
})
),
store.select(selectors.getRunSelectorRegexFilter).pipe(
map((value) => {
if (!value) return [];
return [{key: RUN_REGEX_FILTER_KEY, value}];
})
),
]).pipe(
map((queryParamList) => {
return queryParamList.flat();
Expand All @@ -176,6 +183,7 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
let smoothing = null;
let tagFilter = null;
let groupBy: GroupBy | null = null;
let runRegexFilter = null;

for (const {key, value} of queryParams) {
switch (key) {
Expand Down Expand Up @@ -206,6 +214,9 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
case TAG_FILTER_KEY:
tagFilter = value;
break;
case RUN_REGEX_FILTER_KEY:
runRegexFilter = value;
break;
}
}

Expand All @@ -217,6 +228,7 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
},
runs: {
groupBy,
regexFilter: runRegexFilter,
},
};
}
Expand Down
133 changes: 84 additions & 49 deletions tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ describe('core deeplink provider', () => {
store.overrideSelector(selectors.getOverriddenFeatureFlags, {});
store.overrideSelector(selectors.getMetricsSettingOverrides, {});
store.overrideSelector(selectors.getRunUserSetGroupBy, null);
store.overrideSelector(selectors.getRunSelectorRegexFilter, '');

queryParamsSerialized = [];

Expand Down Expand Up @@ -419,67 +420,101 @@ describe('core deeplink provider', () => {
});

describe('runs', () => {
it('does not put state in the URL when user set color group is null', () => {
// Setting from `null` to `null` does not actually trigger the provider so
// we have to set it: `null` -> something else -> `null` to test this
// case.
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.EXPERIMENT,
describe('color group', () => {
it('does not put state in the URL when user set color group is null', () => {
// Setting from `null` to `null` does not actually trigger the provider so
// we have to set it: `null` -> something else -> `null` to test this
// case.
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.EXPERIMENT,
});
store.refreshState();

store.overrideSelector(selectors.getRunUserSetGroupBy, null);
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[]
);
});
store.refreshState();

store.overrideSelector(selectors.getRunUserSetGroupBy, null);
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[]
);
});
it('serializes user set color group settings', () => {
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.EXPERIMENT,
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'experiment'}]
);

it('serializes user set color group settings', () => {
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.EXPERIMENT,
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'runColorGroup', value: 'experiment'},
]);
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.RUN,
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'run'}]
);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.RUN,
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: 'hello:world',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex:hello:world'}]
);
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'runColorGroup', value: 'run'},
]);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: 'hello:world',
it('serializes interesting regex strings', () => {
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: '',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex:'}]
);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: 'hello/(world):goodbye',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex:hello/(world):goodbye'}]
);
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'runColorGroup', value: 'regex:hello:world'},
]);
});

it('serializes interesting regex strings', () => {
store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: '',
describe('regex filter', () => {
it('does not serialize an empty string', () => {
store.overrideSelector(selectors.getRunSelectorRegexFilter, '');
store.refreshState();

expect(queryParamsSerialized).toEqual([]);
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'runColorGroup', value: 'regex:'},
]);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX,
regexString: 'hello/(world):goodbye',
it('serializes runRegexFilter state', () => {
store.overrideSelector(selectors.getRunSelectorRegexFilter, 'hello');
store.refreshState();

expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runRegexFilter', value: 'hello'}]
);

store.overrideSelector(selectors.getRunSelectorRegexFilter, 'hello:');
store.refreshState();

expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runRegexFilter', value: 'hello:'}]
);

store.overrideSelector(selectors.getRunSelectorRegexFilter, 'hello:.*');
store.refreshState();

expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runRegexFilter', value: 'hello:.*'}]
);
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'runColorGroup', value: 'regex:hello/(world):goodbye'},
]);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,5 @@ export const PINNED_CARDS_KEY = 'pinnedCards';
export const RUN_COLOR_GROUP_KEY = 'runColorGroup';

export const TAG_FILTER_KEY = 'tagFilter';

export const RUN_REGEX_FILTER_KEY = 'runRegexFilter';
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps simpler, runFilter to match tagFilter? They are both regexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't tagFilter related to another filter? (where the arrow points at)
Screen Shot 2021-11-10 at 10 53 29 AM
t)

Copy link
Contributor

Choose a reason for hiding this comment

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

So, in L29, we have "tagFilter" instead of "tagRegexFilter" even though tag filter also let you specify a regex.

Likewise, I am asking if we can drop word" regex" from "runRegexFilter" like we did for tag one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see. I thought you want to rename runRegexFilter to tag..

1 change: 1 addition & 0 deletions tensorboard/webapp/routes/testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export function buildDeserializedState(
return {
runs: {
groupBy: null,
regexFilter: null,
},
metrics: {
pinnedCards: [],
Expand Down
19 changes: 12 additions & 7 deletions tensorboard/webapp/runs/store/runs_reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,19 +97,24 @@ const dataReducer: ActionReducer<RunsDataState, Action> = createReducer(

const dehydratedState = partialState as URLDeserializedState;
const groupBy = dehydratedState.runs.groupBy;
if (!groupBy) {
const regexFilter = dehydratedState.runs.regexFilter ?? '';

if (!groupBy && !regexFilter) {
return state;
}

const regexString =
groupBy.key === GroupByKey.REGEX
? groupBy.regexString
: state.colorGroupRegexString;
if (groupBy) {
const regexString =
groupBy.key === GroupByKey.REGEX
? groupBy.regexString
: state.colorGroupRegexString;
state.colorGroupRegexString = regexString;
state.userSetGroupByKey = groupBy.key ?? null;
}

return {
...state,
colorGroupRegexString: regexString,
userSetGroupByKey: groupBy.key ?? null,
regexFilter,
};
}),
on(runsActions.fetchRunsRequested, (state, action) => {
Expand Down
75 changes: 75 additions & 0 deletions tensorboard/webapp/runs/store/runs_reducers_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,7 @@ describe('runs_reducers', () => {
const partialState: URLDeserializedState = {
runs: {
groupBy: {key: GroupByKey.EXPERIMENT},
regexFilter: null,
},
};
const nextState = runsReducers.reducers(
Expand All @@ -1031,6 +1032,7 @@ describe('runs_reducers', () => {
const partialState: URLDeserializedState = {
runs: {
groupBy: null,
regexFilter: null,
},
};
const nextState = runsReducers.reducers(
Expand Down Expand Up @@ -1082,6 +1084,7 @@ describe('runs_reducers', () => {
const partialState: URLDeserializedState = {
runs: {
groupBy: {key: GroupByKey.EXPERIMENT},
regexFilter: null,
},
};
const nextState = runsReducers.reducers(
Expand Down Expand Up @@ -1118,6 +1121,7 @@ describe('runs_reducers', () => {
const partialState: URLDeserializedState = {
runs: {
groupBy: {key: GroupByKey.REGEX, regexString: 'regex string'},
regexFilter: null,
},
};
const nextState = runsReducers.reducers(
Expand All @@ -1130,6 +1134,77 @@ describe('runs_reducers', () => {

expect(nextState.data.colorGroupRegexString).toBe('regex string');
});

it('does not set regexFilter when null value provided', () => {
const state = buildRunsState({
regexFilter: 'hello',
});

const partialState: URLDeserializedState = {
runs: {
groupBy: null,
regexFilter: null,
},
};
const nextState = runsReducers.reducers(
state,
stateRehydratedFromUrl({
routeKind: RouteKind.EXPERIMENT,
partialState,
})
);

expect(nextState.data.regexFilter).toEqual('hello');
Copy link
Contributor

Choose a reason for hiding this comment

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

toEqual -> toBe. Here and below.

});

it('sets regexFilter to the valid value provided', () => {
const state = buildRunsState({
regexFilter: 'hello',
});

const partialState: URLDeserializedState = {
runs: {
groupBy: null,
regexFilter: 'world',
},
};
const nextState = runsReducers.reducers(
state,
stateRehydratedFromUrl({
routeKind: RouteKind.EXPERIMENT,
partialState,
})
);

expect(nextState.data.regexFilter).toEqual('world');
});

it('set regexFilter and userSetGroupBy to the value provided', () => {
const state = buildRunsState({
colorGroupRegexString: '',
initialGroupBy: {key: GroupByKey.REGEX, regexString: ''},
userSetGroupByKey: GroupByKey.RUN,
regexFilter: 'hello',
});

const partialState: URLDeserializedState = {
runs: {
groupBy: {key: GroupByKey.EXPERIMENT},
regexFilter: 'world',
},
};

const nextState = runsReducers.reducers(
state,
stateRehydratedFromUrl({
routeKind: RouteKind.EXPERIMENT,
partialState,
})
);

expect(nextState.data.regexFilter).toEqual('world');
expect(nextState.data.userSetGroupByKey).toEqual(GroupByKey.EXPERIMENT);
});
});

describe('when freshly navigating', () => {
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/runs/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,6 @@ export type GroupBy = BaseGroupBy | RegexGroupBy;
export interface URLDeserializedState {
runs: {
groupBy: GroupBy | null;
regexFilter: string | null;
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
<div class="filter-row">
<tb-filter-input
class="run-filter"
value="{{ regexFilter }}"
(keyup)="onFilterKeyUp($event)"
placeholder="Filter runs (regex)"
></tb-filter-input>
Expand Down