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

Add share button in experiment view #4936

Merged
merged 22 commits into from Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions mlflow/server/js/src/common/utils/Utils.js
Expand Up @@ -711,10 +711,9 @@ class Utils {

static getSearchParamsFromUrl(search) {
const params = qs.parse(search, { ignoreQueryPrefix: true });
const str = JSON.stringify(params, function replaceUndefined(key, value) {
return value === undefined ? '' : value;
const str = JSON.stringify(params, function replaceUndefinedAndBools(key, value) {
return value === undefined ? '' : value === 'true' ? true : value === 'false' ? false : value;
marijncv marked this conversation as resolved.
Show resolved Hide resolved
});

return params ? JSON.parse(str) : [];
}

Expand Down
5 changes: 5 additions & 0 deletions mlflow/server/js/src/common/utils/Utils.test.js
Expand Up @@ -462,6 +462,7 @@ test('getSearchParamsFromUrl', () => {
const url1 = '?p=&q=&r=';
const url2 = '?';
const url3 = '?searchInput=some-Input';
const url4 = '?boolVal1=true&boolVal2=false';
expect(Utils.getSearchParamsFromUrl(url0)).toEqual({
searchInput: '',
});
Expand All @@ -470,6 +471,10 @@ test('getSearchParamsFromUrl', () => {
expect(Utils.getSearchParamsFromUrl(url3)).toEqual({
searchInput: 'some-Input',
});
expect(Utils.getSearchParamsFromUrl(url4)).toEqual({
boolVal1: true,
boolVal2: false,
});
});

test('getSearchUrlFromState', () => {
Expand Down
Expand Up @@ -22,13 +22,14 @@ import { Experiment } from '../sdk/MlflowMessages';
import { injectIntl } from 'react-intl';
import {
LIFECYCLE_FILTER,
MODEL_VERSION_FILTER,
PAGINATION_DEFAULT_STATE,
MAX_DETECT_NEW_RUNS_RESULTS,
DETECT_NEW_RUNS_INTERVAL,
DEFAULT_ORDER_BY_KEY,
DEFAULT_ORDER_BY_ASC,
DEFAULT_START_TIME,
DEFAULT_LIFECYCLE_FILTER,
DEFAULT_MODEL_VERSION_FILTER,
ATTRIBUTE_COLUMN_SORT_KEY,
} from '../constants';

Expand Down Expand Up @@ -73,19 +74,20 @@ export class ExperimentPage extends Component {
numberOfNewRuns: 0,
// Last experiment, if any, displayed by this instance of ExperimentPage
lastExperimentId: undefined,
// Lifecycle filter of runs to display
lifecycleFilter: LIFECYCLE_FILTER.ACTIVE,
// Filter of model versions to display
modelVersionFilter: MODEL_VERSION_FILTER.ALL_RUNS,
...PAGINATION_DEFAULT_STATE,
getExperimentRequestId: null,
searchRunsRequestId: null,
persistedState: {
searchInput: urlState.search === undefined ? '' : urlState.search,
orderByKey: urlState.orderByKey === undefined ? DEFAULT_ORDER_BY_KEY : urlState.orderByKey,
orderByAsc:
urlState.orderByAsc === undefined ? DEFAULT_ORDER_BY_ASC : urlState.orderByAsc === 'true',
orderByAsc: urlState.orderByAsc === undefined ? DEFAULT_ORDER_BY_ASC : urlState.orderByAsc,
startTime: urlState.startTime === undefined ? DEFAULT_START_TIME : urlState.startTime,
lifecycleFilter:
urlState.lifecycle === undefined ? DEFAULT_LIFECYCLE_FILTER : urlState.lifecycle,
modelVersionFilter:
urlState.modelVersion === undefined
? DEFAULT_MODEL_VERSION_FILTER
: urlState.modelVersion,
},
};
}
Expand All @@ -105,9 +107,8 @@ export class ExperimentPage extends Component {
persistedState:
state.lastExperimentId === undefined
? state.persistedState
: new ExperimentPagePersistedState({ orderByKey: DEFAULT_ORDER_BY_KEY }).toJSON(),
: new ExperimentPagePersistedState().toJSON(),
lastExperimentId: props.experimentId,
lifecycleFilter: LIFECYCLE_FILTER.ACTIVE,
nextPageToken: null,
getExperimentRequestId: getUUID(),
searchRunsRequestId: getUUID(),
Expand Down Expand Up @@ -198,8 +199,8 @@ export class ExperimentPage extends Component {
}

handleGettingRuns = (getRunsAction, requestId) => {
const { persistedState, lifecycleFilter, nextPageToken } = this.state;
const { searchInput } = persistedState;
const { persistedState, nextPageToken } = this.state;
const { searchInput, lifecycleFilter } = persistedState;
const viewType = lifecycleFilterToRunViewType(lifecycleFilter);
const orderBy = this.getOrderByExpr();
const startTime = this.getStartTimeExpr();
Expand Down Expand Up @@ -248,19 +249,12 @@ export class ExperimentPage extends Component {

onSearch = (
searchInput,
lifecycleFilterInput,
lifecycleFilter,
orderByKey,
orderByAsc,
modelVersionFilterInput,
modelVersionFilter,
startTime,
) => {
this.updateUrlWithSearchFilter({
searchInput,
orderByKey,
orderByAsc,
startTime,
});

this.setState(
{
lastRunsRefreshTime: Date.now(),
Expand All @@ -270,9 +264,9 @@ export class ExperimentPage extends Component {
orderByKey,
orderByAsc,
startTime,
lifecycleFilter,
modelVersionFilter,
}).toJSON(),
lifecycleFilter: lifecycleFilterInput,
modelVersionFilter: modelVersionFilterInput,
nextPageToken: null,
},
() => {
Expand Down Expand Up @@ -329,29 +323,6 @@ export class ExperimentPage extends Component {
return orderBy;
}

updateUrlWithSearchFilter({ searchInput, orderByKey, orderByAsc, startTime }) {
const state = {};
if (searchInput) {
state['search'] = searchInput;
}
if (startTime) {
state['startTime'] = startTime;
}
if (orderByKey) {
state['orderByKey'] = orderByKey;
}
// orderByAsc defaults to true, so only encode it if it is false.
if (orderByAsc === false) {
state['orderByAsc'] = orderByAsc;
}
const newUrl = `/experiments/${this.props.experimentId}/s?${Utils.getSearchUrlFromState(
state,
)}`;
if (newUrl !== this.props.history.location.pathname + this.props.history.location.search) {
this.props.history.push(newUrl);
}
}

renderExperimentView = (isLoading, shouldRenderError, requests) => {
let searchRunsError;
const getExperimentRequest = Utils.getRequestWithId(
Expand All @@ -376,14 +347,22 @@ export class ExperimentPage extends Component {
return <Spinner />;
}

const { searchInput, orderByKey, orderByAsc, startTime } = this.state.persistedState;
const {
searchInput,
orderByKey,
orderByAsc,
startTime,
lifecycleFilter,
modelVersionFilter,
} = this.state.persistedState;

const experimentViewProps = {
experimentId: this.props.experimentId,
experiment: this.props.experiment,
location: this.props.location,
searchRunsRequestId: this.state.searchRunsRequestId,
modelVersionFilter: this.state.modelVersionFilter,
lifecycleFilter: this.state.lifecycleFilter,
modelVersionFilter: modelVersionFilter,
lifecycleFilter: lifecycleFilter,
onSearch: this.onSearch,
searchRunsError: searchRunsError,
searchInput: searchInput,
Expand Down
@@ -1,5 +1,4 @@
import React from 'react';
import qs from 'qs';
import { shallow } from 'enzyme';
import { MemoryRouter as Router } from 'react-router-dom';

Expand All @@ -17,9 +16,11 @@ import {
PAGINATION_DEFAULT_STATE,
DEFAULT_ORDER_BY_KEY,
DEFAULT_ORDER_BY_ASC,
DEFAULT_START_TIME,
DEFAULT_LIFECYCLE_FILTER,
DEFAULT_MODEL_VERSION_FILTER,
} from '../constants';

const BASE_PATH = '/experiments/17/s';
const EXPERIMENT_ID = '17';

jest.useFakeTimers();
Expand All @@ -39,12 +40,7 @@ beforeEach(() => {
loadMoreRunsApi = jest.fn(() => Promise.resolve());
searchForNewRuns = jest.fn(() => Promise.resolve());
location = {};

history = {};
history.location = {};
history.location.pathname = BASE_PATH;
history.location.search = '';
history.push = jest.fn();
});

const getExperimentPageMock = (additionalProps) => {
Expand All @@ -63,18 +59,17 @@ const getExperimentPageMock = (additionalProps) => {
);
};

function expectSearchState(historyEntry, state) {
const expectedPrefix = BASE_PATH + '?';
expect(historyEntry.startsWith(expectedPrefix)).toBe(true);
const search = historyEntry.substring(expectedPrefix.length);
const parsedHistory = qs.parse(search);
expect(parsedHistory).toEqual(state);
}

test('URL is empty for blank search', () => {
test('State and search params are correct for blank search', () => {
const wrapper = getExperimentPageMock();
wrapper.instance().onSearch('', 'Active', null, true, null);
expectSearchState(history.push.mock.calls[0][0], {});

expect(wrapper.state().persistedState.searchInput).toEqual('');
expect(wrapper.state().persistedState.lifecycleFilter).toEqual('Active');
expect(wrapper.state().persistedState.orderByKey).toEqual(null);
expect(wrapper.state().persistedState.orderByAsc).toEqual(true);
expect(wrapper.state().persistedState.modelVersionFilter).toEqual(null);
expect(wrapper.state().persistedState.startTime).toEqual(undefined);

const searchRunsCallParams = searchRunsApi.mock.calls[1][0];

expect(searchRunsCallParams.experimentIds).toEqual([EXPERIMENT_ID]);
Expand All @@ -83,25 +78,33 @@ test('URL is empty for blank search', () => {
expect(searchRunsCallParams.orderBy).toEqual([]);
});

test('URL can encode a complete search', () => {
test('State and search params are correct for complete search', () => {
const wrapper = getExperimentPageMock();
wrapper.instance().onSearch('metrics.metric0 > 3', 'Deleted', null, true, null, 'ALL');
expectSearchState(history.push.mock.calls[0][0], {
search: 'metrics.metric0 > 3',
startTime: 'ALL',
});

expect(wrapper.state().persistedState.searchInput).toEqual('metrics.metric0 > 3');
expect(wrapper.state().persistedState.lifecycleFilter).toEqual('Deleted');
expect(wrapper.state().persistedState.orderByKey).toEqual(null);
expect(wrapper.state().persistedState.orderByAsc).toEqual(true);
expect(wrapper.state().persistedState.modelVersionFilter).toEqual(null);
expect(wrapper.state().persistedState.startTime).toEqual('ALL');

const searchRunsCallParams = searchRunsApi.mock.calls[1][0];
expect(searchRunsCallParams.filter).toEqual('metrics.metric0 > 3');
expect(searchRunsCallParams.runViewType).toEqual(ViewType.DELETED_ONLY);
});

test('URL can encode order_by', () => {
test('State and search params are correct for search with order_by', () => {
const wrapper = getExperimentPageMock();
wrapper.instance().onSearch('', 'Active', 'my_key', false, null);
expectSearchState(history.push.mock.calls[0][0], {
orderByKey: 'my_key',
orderByAsc: 'false',
});

expect(wrapper.state().persistedState.searchInput).toEqual('');
expect(wrapper.state().persistedState.lifecycleFilter).toEqual('Active');
expect(wrapper.state().persistedState.orderByKey).toEqual('my_key');
expect(wrapper.state().persistedState.orderByAsc).toEqual(false);
expect(wrapper.state().persistedState.modelVersionFilter).toEqual(null);
expect(wrapper.state().persistedState.startTime).toEqual(undefined);

const searchRunsCallParams = searchRunsApi.mock.calls[1][0];
expect(searchRunsCallParams.filter).toEqual('');
expect(searchRunsCallParams.orderBy).toEqual(['my_key DESC']);
Expand All @@ -111,17 +114,24 @@ test('Loading state without any URL params', () => {
const wrapper = getExperimentPageMock();
const { state } = wrapper.instance();
expect(state.persistedState.searchInput).toEqual('');
expect(state.persistedState.lifecycleFilter).toEqual(DEFAULT_LIFECYCLE_FILTER);
expect(state.persistedState.modelVersionFilter).toEqual(DEFAULT_MODEL_VERSION_FILTER);
expect(state.persistedState.orderByKey).toBe(DEFAULT_ORDER_BY_KEY);
expect(state.persistedState.orderByAsc).toEqual(DEFAULT_ORDER_BY_ASC);
expect(state.persistedState.startTime).toEqual(DEFAULT_START_TIME);
});

test('Loading state with all URL params', () => {
location.search = 'params=a&metrics=b&search=c&orderByKey=d&orderByAsc=false';
location.search =
'search=c&orderByKey=d&orderByAsc=false&startTime=LAST_HOUR&lifecycle=Deleted&modelVersion=With%20Model%20Versions';
const wrapper = getExperimentPageMock();
const { state } = wrapper.instance();
expect(state.persistedState.searchInput).toEqual('c');
expect(state.persistedState.lifecycleFilter).toEqual('Deleted');
expect(state.persistedState.modelVersionFilter).toEqual('With Model Versions');
expect(state.persistedState.orderByKey).toEqual('d');
expect(state.persistedState.orderByAsc).toEqual(false);
expect(state.persistedState.startTime).toEqual('LAST_HOUR');
});

test('should render permission denied view when getExperiment yields permission error', () => {
Expand Down