Skip to content

Commit

Permalink
Stop removing experiment related query parameters (tensorflow#5717)
Browse files Browse the repository at this point in the history
* Stop removing query parameters when persisting application state to the url

* simplify query param serialization logic

* move feature flag overrides to feature flag metadata
  • Loading branch information
rileyajones authored and dna2github committed May 1, 2023
1 parent 6b36a15 commit 5485907
Show file tree
Hide file tree
Showing 13 changed files with 709 additions and 231 deletions.
14 changes: 14 additions & 0 deletions tensorboard/webapp/feature_flag/store/BUILD
Expand Up @@ -10,6 +10,7 @@ tf_ng_module(
"feature_flag_store_config_provider.ts",
],
deps = [
":feature_flag_metadata",
":types",
"//tensorboard/webapp/feature_flag:types",
"//tensorboard/webapp/feature_flag/actions",
Expand All @@ -21,6 +22,17 @@ tf_ng_module(
],
)

tf_ng_module(
name = "feature_flag_metadata",
srcs = [
"feature_flag_metadata.ts",
],
deps = [
"//tensorboard/webapp/feature_flag:types",
"@npm//@angular/core",
],
)

tf_ng_module(
name = "types",
srcs = [
Expand All @@ -45,10 +57,12 @@ tf_ng_module(
name = "store_test_lib",
testonly = True,
srcs = [
"feature_flag_metadata_test.ts",
"feature_flag_reducers_test.ts",
"feature_flag_selectors_test.ts",
],
deps = [
":feature_flag_metadata",
":store",
":testing",
":types",
Expand Down
127 changes: 127 additions & 0 deletions tensorboard/webapp/feature_flag/store/feature_flag_metadata.ts
@@ -0,0 +1,127 @@
/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {FeatureFlags} from '../types';

export type BaseFeatureFlagType = boolean | number | string | null | undefined;

export type FeatureFlagType = BaseFeatureFlagType | Array<BaseFeatureFlagType>;

export type FeatureFlagMetadata<T> = {
defaultValue: T;
queryParamOverride?: string;
parseValue: (str: string) => T extends (infer U)[] ? U : T; // The type, or, if the type is an array, the type of the array contents
isArray?: boolean;
};

export type FeatureFlagMetadataMapType<T extends FeatureFlags> = {
[FlagName in keyof T]: FeatureFlagMetadata<T[FlagName]>;
};

export function parseBoolean(str: string): boolean {
return str !== 'false';
}

export function parseBooleanOrNull(str: string): boolean | null {
if (str === 'null') {
return null;
}
return parseBoolean(str);
}

export const FeatureFlagMetadataMap: FeatureFlagMetadataMapType<FeatureFlags> =
{
scalarsBatchSize: {
defaultValue: undefined,
queryParamOverride: 'scalarsBatchSize',
parseValue: parseInt,
},
enabledColorGroup: {
defaultValue: true,
queryParamOverride: 'enableColorGroup',
parseValue: parseBoolean,
},
enabledColorGroupByRegex: {
defaultValue: true,
queryParamOverride: 'enableColorGroupByRegex',
parseValue: parseBoolean,
},
enabledExperimentalPlugins: {
defaultValue: [],
queryParamOverride: 'experimentalPlugin',
parseValue: (str: string) => str,
isArray: true,
},
enabledLinkedTime: {
defaultValue: false,
queryParamOverride: 'enableLinkedTime',
parseValue: parseBoolean,
},
enabledCardWidthSetting: {
defaultValue: true,
queryParamOverride: 'enableCardWidthSetting',
parseValue: parseBoolean,
},
enabledScalarDataTable: {
defaultValue: false,
queryParamOverride: 'enableDataTable',
parseValue: parseBoolean,
},
forceSvg: {
defaultValue: false,
queryParamOverride: 'forceSVG',
parseValue: parseBoolean,
},
enableDarkModeOverride: {
defaultValue: null,
parseValue: parseBooleanOrNull,
},
defaultEnableDarkMode: {
defaultValue: false,
queryParamOverride: 'darkMode',
parseValue: parseBoolean,
},
isAutoDarkModeAllowed: {
defaultValue: true,
parseValue: parseBoolean,
},
inColab: {
defaultValue: false,
queryParamOverride: 'tensorboardColab',
parseValue: parseBoolean,
},
metricsImageSupportEnabled: {
defaultValue: true,
parseValue: parseBoolean,
},
enableTimeSeriesPromotion: {
defaultValue: false,
parseValue: parseBoolean,
},
};

/**
* Gets gets just the default values of each feature flag from the provided metadata.
*/
export function generateFeatureFlagDefaults<T extends FeatureFlags>(
featureFlagMetadataMap: FeatureFlagMetadataMapType<T>
): T {
return Object.entries(featureFlagMetadataMap).reduce(
(map, [key, {defaultValue}]) => {
map[key] = defaultValue;
return map;
},
{} as Record<string, any>
) as T;
}
@@ -0,0 +1,45 @@
import {parseBoolean, parseBooleanOrNull} from './feature_flag_metadata';

/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
describe('feature flag query parameters', () => {
describe('parseBoolean', () => {
it('"false" should evaluate to false', () => {
expect(parseBoolean('false')).toBeFalse();
});

it('values other than "false" should evaluate to true', () => {
expect(parseBoolean('true')).toBeTrue();
expect(parseBoolean('foo bar')).toBeTrue();
expect(parseBoolean('')).toBeTrue();
});
});

describe('parseBooleanOrNull', () => {
it('"null" should return null', () => {
expect(parseBooleanOrNull('null')).toBeNull();
});

it('"false" should evaluate to false', () => {
expect(parseBooleanOrNull('false')).toBeFalse();
});

it('values other than "false" should evaluate to true', () => {
expect(parseBooleanOrNull('true')).toBeTrue();
expect(parseBooleanOrNull('foo bar')).toBeTrue();
expect(parseBooleanOrNull('')).toBeTrue();
});
});
});
Expand Up @@ -14,26 +14,15 @@ limitations under the License.
==============================================================================*/
import {InjectionToken} from '@angular/core';
import {StoreConfig} from '@ngrx/store';
import {
FeatureFlagMetadataMap,
generateFeatureFlagDefaults,
} from './feature_flag_metadata';
import {FeatureFlagState} from './feature_flag_types';

export const initialState: FeatureFlagState = {
isFeatureFlagsLoaded: false,
defaultFlags: {
isAutoDarkModeAllowed: true,
defaultEnableDarkMode: false,
enableDarkModeOverride: null,
enabledColorGroup: true,
enabledColorGroupByRegex: true,
enabledExperimentalPlugins: [],
inColab: false,
scalarsBatchSize: undefined,
metricsImageSupportEnabled: true,
enabledLinkedTime: false,
enableTimeSeriesPromotion: false,
enabledCardWidthSetting: true,
forceSvg: false,
enabledScalarDataTable: false,
},
defaultFlags: generateFeatureFlagDefaults(FeatureFlagMetadataMap),
flagOverrides: {},
};

Expand Down
20 changes: 20 additions & 0 deletions tensorboard/webapp/routes/BUILD
Expand Up @@ -37,11 +37,15 @@ tf_ts_library(
],
deps = [
":dashboard_deeplink_provider_types",
":feature_flag_serializer",
"//tensorboard/webapp:app_state",
"//tensorboard/webapp:selectors",
"//tensorboard/webapp/app_routing:deep_link_provider",
"//tensorboard/webapp/app_routing:route_config",
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/feature_flag:types",
"//tensorboard/webapp/feature_flag/store",
"//tensorboard/webapp/feature_flag/store:feature_flag_metadata",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics/data_source:types",
"//tensorboard/webapp/runs:types",
Expand All @@ -53,6 +57,18 @@ tf_ts_library(
],
)

tf_ts_library(
name = "feature_flag_serializer",
srcs = [
"feature_flag_serializer.ts",
],
deps = [
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/feature_flag:types",
"//tensorboard/webapp/feature_flag/store:feature_flag_metadata",
],
)

tf_ts_library(
name = "testing",
testonly = True,
Expand All @@ -69,16 +85,20 @@ tf_ts_library(
testonly = True,
srcs = [
"dashboard_deeplink_provider_test.ts",
"feature_flag_serializer_test.ts",
],
deps = [
":dashboard_deeplink_provider",
":feature_flag_serializer",
":testing",
"//tensorboard/webapp:app_state",
"//tensorboard/webapp:selectors",
"//tensorboard/webapp/angular:expect_angular_core_testing",
"//tensorboard/webapp/angular:expect_ngrx_store_testing",
"//tensorboard/webapp/app_routing:deep_link_provider",
"//tensorboard/webapp/app_routing:location",
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/feature_flag/store:feature_flag_metadata",
"//tensorboard/webapp/metrics:test_lib",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics/data_source:types",
Expand Down
54 changes: 18 additions & 36 deletions tensorboard/webapp/routes/dashboard_deeplink_provider.ts
Expand Up @@ -19,6 +19,12 @@ import {map} from 'rxjs/operators';
import {DeepLinkProvider} from '../app_routing/deep_link_provider';
import {SerializableQueryParams} from '../app_routing/types';
import {State} from '../app_state';
import {
FeatureFlagMetadata,
FeatureFlagMetadataMap,
FeatureFlagType,
} from '../feature_flag/store/feature_flag_metadata';
import {getOverriddenFeatureFlags} from '../feature_flag/store/feature_flag_selectors';
import {
isPluginType,
isSampledPlugin,
Expand All @@ -27,11 +33,6 @@ import {
import {CardUniqueInfo} from '../metrics/types';
import {GroupBy, GroupByKey} from '../runs/types';
import * as selectors from '../selectors';
import {
ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY,
ENABLE_COLOR_GROUP_QUERY_PARAM_KEY,
EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY,
} from '../webapp_data_source/tb_feature_flag_data_source_types';
import {
DeserializedState,
PINNED_CARDS_KEY,
Expand All @@ -40,6 +41,7 @@ import {
SMOOTHING_KEY,
TAG_FILTER_KEY,
} from './dashboard_deeplink_provider_types';
import {featureFlagsToSerializableQueryParams} from './feature_flag_serializer';

const COLOR_GROUP_REGEX_VALUE_PREFIX = 'regex:';

Expand Down Expand Up @@ -83,36 +85,6 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
);
}

private getFeatureFlagStates(
store: Store<State>
): Observable<SerializableQueryParams> {
return combineLatest([
store.select(selectors.getEnabledExperimentalPlugins),
store.select(selectors.getOverriddenFeatureFlags),
]).pipe(
map(([experimentalPlugins, overriddenFeatureFlags]) => {
const queryParams = experimentalPlugins.map((pluginId) => {
return {key: EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, value: pluginId};
});
if (typeof overriddenFeatureFlags.enabledColorGroup === 'boolean') {
queryParams.push({
key: ENABLE_COLOR_GROUP_QUERY_PARAM_KEY,
value: String(overriddenFeatureFlags.enabledColorGroup),
});
}
if (
typeof overriddenFeatureFlags.enabledColorGroupByRegex === 'boolean'
) {
queryParams.push({
key: ENABLE_COLOR_GROUP_BY_REGEX_QUERY_PARAM_KEY,
value: String(overriddenFeatureFlags.enabledColorGroupByRegex),
});
}
return queryParams;
})
);
}

serializeStateToQueryParams(
store: Store<State>
): Observable<SerializableQueryParams> {
Expand All @@ -126,7 +98,17 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
return [{key: TAG_FILTER_KEY, value: filterText}];
})
),
this.getFeatureFlagStates(store),
store.select(getOverriddenFeatureFlags).pipe(
map((featureFlags) => {
return featureFlagsToSerializableQueryParams(
featureFlags,
FeatureFlagMetadataMap as Record<
string,
FeatureFlagMetadata<FeatureFlagType>
>
);
})
),
store.select(selectors.getMetricsSettingOverrides).pipe(
map((settingOverrides) => {
if (Number.isFinite(settingOverrides.scalarSmoothing)) {
Expand Down

0 comments on commit 5485907

Please sign in to comment.