Skip to content

Commit

Permalink
Fix the issue with coloring when trying to add more experiments. (#6861)
Browse files Browse the repository at this point in the history
There was a problem when user tried to 1) enable coloring by experiment
name, 2) trying to add more experiments. It stemmed from not
implementing the serialization and deserialization of REGEX_BY_EXP in
the query pararms of the route. To followup the #6846 and #6847 I have
added the 'regex_exp:' as a query parameter for coloring by experiment
name.

## Motivation for features / changes

To fix the issue with the new feature. 

## Technical description of changes

Added `regex_exp:` query param for serialization and deserialization of
`REGEX_BY_EXP` GroupBy. It ensures that whenever user share the URL the
filter will remain in place.

## Screenshots of UI changes (or N/A)

## Detailed steps to verify changes work correctly (as executed by you)

1) Enable the coloring by experiment name.
2) Try to add more experiments via pressing `Add more experiments` in
the top panel.

## Alternate designs / implementations considered (or N/A)
  • Loading branch information
AnuarTB committed Jun 4, 2024
1 parent ae7d0b9 commit c3fd2b5
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 1 deletion.
10 changes: 10 additions & 0 deletions tensorboard/webapp/routes/dashboard_deeplink_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
import {featureFlagsToSerializableQueryParams} from './feature_flag_serializer';

const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:';
const COLOR_GROUP_REGEX_BY_EXP_VALUE_PREFIX = 'regex_by_exp:';

/**
* Provides deeplinking for the core dashboards page.
Expand Down Expand Up @@ -135,6 +136,9 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
case GroupByKey.REGEX:
value = `${COLOR_GROUP_REGEX_VALUE_PREFIX}${groupBy.regexString}`;
break;
case GroupByKey.REGEX_BY_EXP:
value = `${COLOR_GROUP_REGEX_BY_EXP_VALUE_PREFIX}${groupBy.regexString}`;
break;
default:
throw new RangeError(`Serialization not implemented`);
}
Expand Down Expand Up @@ -196,6 +200,12 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
);
groupBy = {key: GroupByKey.REGEX, regexString};
}
if (value.startsWith(COLOR_GROUP_REGEX_BY_EXP_VALUE_PREFIX)) {
const regexString = value.slice(
COLOR_GROUP_REGEX_BY_EXP_VALUE_PREFIX.length
);
groupBy = {key: GroupByKey.REGEX_BY_EXP, regexString};
}
break;
}
case TAG_FILTER_KEY:
Expand Down
36 changes: 36 additions & 0 deletions tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,19 @@ describe('core deeplink provider', () => {
assert('run', {key: GroupByKey.RUN});
assert('regex:', {key: GroupByKey.REGEX, regexString: ''});
assert('regex:hello', {key: GroupByKey.REGEX, regexString: 'hello'});
assert('regex_by_exp:', {
key: GroupByKey.REGEX_BY_EXP,
regexString: '',
});
assert('regex_by_exp:world', {
key: GroupByKey.REGEX_BY_EXP,
regexString: 'world',
});
assert('', null);
assert('regex', null);
assert('runs', null);
assert('experiments', null);
assert('regex_by_exp', null);
});
});

Expand Down Expand Up @@ -434,6 +443,15 @@ describe('core deeplink provider', () => {
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex:hello:world'}]
);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX_BY_EXP,
regexString: 'exp_name',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex_by_exp:exp_name'}]
);
});

it('serializes interesting regex strings', () => {
Expand All @@ -454,6 +472,24 @@ describe('core deeplink provider', () => {
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex:hello/(world):goodbye'}]
);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX_BY_EXP,
regexString: '',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex_by_exp:'}]
);

store.overrideSelector(selectors.getRunUserSetGroupBy, {
key: GroupByKey.REGEX_BY_EXP,
regexString: 'one|two',
});
store.refreshState();
expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual(
[{key: 'runColorGroup', value: 'regex_by_exp:one|two'}]
);
});
});

Expand Down
6 changes: 5 additions & 1 deletion tensorboard/webapp/runs/store/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ export function groupRuns(
break;

case GroupByKey.REGEX_BY_EXP:
if (!groupBy.regexString || !expNameByExpId) {
if (
!groupBy.regexString ||
!expNameByExpId ||
Object.keys(expNameByExpId).length === 0
) {
break;
}

Expand Down
23 changes: 23 additions & 0 deletions tensorboard/webapp/runs/store/utils_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,29 @@ describe('run store utils test', () => {
});
});

it('does not group if no experiment names are provided', () => {
const actual = groupRuns(
{key: GroupByKey.REGEX_BY_EXP, regexString: 'foo\\d+)bar'},
[
buildRun({id: 'eid1/alpha', name: 'foo1bar1'}),
buildRun({id: 'eid1/beta', name: 'foo1bar2'}),
buildRun({id: 'eid2/beta', name: 'foo2bar1'}),
buildRun({id: 'eid2/gamma', name: 'gamma'}),
],
{
'eid1/alpha': 'eid1',
'eid1/beta': 'eid1',
'eid2/beta': 'eid2',
'eid2/gamma': 'eid2',
},
{}
);
expect(actual).toEqual({
matches: {},
nonMatches: [],
});
});

it('groups run by regex without capture group', () => {
const actual = groupRuns(
{key: GroupByKey.REGEX_BY_EXP, regexString: 'foo'},
Expand Down

0 comments on commit c3fd2b5

Please sign in to comment.