Manage Experiments from MLflow UI (Create/Rename/Delete Experiments)

[ggliem]

What changes are proposed in this pull request?

Manage Experiments from the UI:

• Added an option to create experiments to the UI
• Added an option to rename experiments to the UI
• Added an option to delete experiments to the UI

Instead of using the CLI, users can now manage experiments via the UI more easily.
These features (among others) have also been requested in #1028.

The following gif demonstrates the change:

How is this patch tested?

• Manual testing
• Added basic unit tests to ExperimentListView.test.js and related forms

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

Enable users to create, delete and rename experiments from the UI.

What component(s) does this PR affect?

• UI
• CLI
• API
• REST-API
• Examples
• Docs
• Tracking
• Projects
• Artifacts
• Models
• Scoring
• Serving
• R
• Java
• Python

How should the PR be classified in the release notes? Choose one:

• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[Zangr]

Thank you @ggliem for adding the delete/rename feature! The component layout LGTM in this PR. We are currently migrating to antd as our new design language and component library, so we are trying to use antd components if possible in new changes. For this PR I'd suggest using antd Modal and Form as shown in this example or RegisterModelButton instead of ReactModal + Formik. It will be even better if you can point the rename modal in the run page to the new antd modal as well. We'll be glad to review your changes :)

[ggliem]

Thank you @ggliem for adding the delete/rename feature! The component layout LGTM in this PR. We are currently migrating to antd as our new design language and component library, so we are trying to use antd components if possible in new changes. For this PR I'd suggest using antd Modal and Form as shown in this example or RegisterModelButton instead of ReactModal + Formik. It will be even better if you can point the rename modal in the run page to the new antd modal as well. We'll be glad to review your changes :)

Thank you @Zangr for your feedback. I started migrating the modals to antd. I changed both the "RenameExperimentModal" as well as the "RenameRunModal" to use antd's Modal&Form instead of ReactModal&Formik in the recent commit. I will update the "DeleteExperimentModal" accordingly. While migrating the "Rename... modals" to antd I noticed the following things:

1. It seems like setting the focus on the input field does not work properly when reopening the modal. I tried calling .focus() on the selected reference (Input) but it did not have the desired effect. I also found some articles online stating that that does not work. As such, the user has to click on the input field first. I noticed that that RegisterModelModal has the same 'issue'. Do you know a workaround for that?
2. Compared to ReactModal, the modal seems to not have an 'Enter' event (Modal Component has ESC event，why no Enter event ant-design/ant-design#11088) (by design). This also holds for the RegisterModelModal.
3. I used the getFieldDecorator in the form. It seems like I can't change the initialValue unless I unmount Form.Item. Compared to the previous implementation, the previous run/experiment name is thus not displayed in the input field when opening the modal. Could you please clarify whether that's ok?
   

[Zangr]

@ggliem Thank you for the refactoring! To solve the 3 issues you mentioned, we can leverage Ref and some imperative ops in componentDidUpdate.

...

autoFocusInputRef = (inputToAutoFocus) => {
  this.inputToAutoFocus = inputToAutoFocus;
  inputToAutoFocus.focus();
}

componentDidUpdate(prevProps) {
  this.autoFocus(prevProps);
  this.resetFields(prevProps);
}

autoFocus(prevProps) {
  if (prevProps.visible === false && this.props.visible === true) {
    this.inputToAutoFocus && this.inputToAutoFocus.focus();
  }
}

resetFields(prevProps) {
  if (prevProps.initialTitle !== this.props.initialTitle) {
    this.props.form.resetFields(['title']);
  }
}

...

<Form.Item label="Title">
  {getFieldDecorator('title', {
    rules: [{ required: true, message: 'Please input the title of collection!' }],
    initialValue: initialTitle,
  })(<Input ref={this.autoFocusInputRef}/>)}
</Form.Item>

...

Here is a sample modal implementation with solutions to all you asks.

[ggliem]

@ggliem Thank you for the refactoring! To solve the 3 issues you mentioned, we can leverage Ref and some imperative ops in componentDidUpdate.

Nice, thanks for the input! I will go ahead and change it.

[ggliem]

@Zangr I implemented the changes & ended up refactoring the "Rename" modals even further by putting the shared code in a generic component. I also migrated the ConfirmModal to antd.

I noticed that when calling the 'Update Experiment' API (/experiments/update) there is no check whether the experiment name already exists. In contrast, the backend service throws an error when trying to create a new experiment with the same name.
Since having experiments with the same name could cause some issues (e.g. when calling mlflow.set_experiment(experiment_name)), I plan to change the service implementation later. Could you please confirm that this is in fact a bug?

Update: I opened a PR for this bug fix: #2357

[ggliem]

@Zangr One more question: I noticed that the CI pipeline failed while executing the "Small" Python (Build: 7746) or "Large" Python (Build: 7709) step. Could you please provide some insight whether there is a way to fix that?

[Zangr]

Manually tested the code change, works pretty well. I left some comments mostly related to naming/usage convention and discussion wrapper component extraction.

[Zangr]

cc @smurching for the question on python test failure

[ggliem]

Thanks @Zangr for the feedback. I replied to some of your comments where I had some follow-up questions/remarks.

[gioa]

@ggliem Thank you for making the change! I got an error banner flashing by when creating a new experiment that already exists in the trash. Can we handle and display the error on the error dialog instead? So users can see what went wrong in one place. Thanks!

[ggliem]

@gioa Good catch, thanks for pointing that out! I totally agree, let's try to make it consistent. You suggested to display the error on the error dialog. However, I'd like to clarify the behavior again before implementing the change.

Looking at the error handling of the RegisterModelModal I noticed that the error is displayed in an Antd-Message (the one displayed on the top of the screen in your screenshot), without closing the actual modal nor displaying the error dialog. The actual error is displayed in the message (e.g. "Internal Server Error").
On the contrary, the modals DeleteRunModal or RestoreRunModal display this kind of error dialog instead, while using a generic error message (e.g. 'While restoring an experiment run, an error occurred.').

In addition to that, the "original" Formik implementation of the RenameRunFormView displayed the occurred error directly in the RenameRunModal, without closing it.

It seems like I ended up mixing the former two options. If an error occurs while creating/deleting/renaming an experiment, should we:

1. Close the current modal & show the error modal or
2. Keep the current modal open & display this kind of message at the top?

I am leaning towards option 2) since the user might have the chance to directly solve the issue (e.g. modifying the name) - what do you think?

[gioa]

@ggliem Thank you for looking at all these different implementations and suggesting a better solution! I agree the option 2 is better given the user can update the name directly there. I hope the message does not dismiss itself before users finish reading.

IMO an even better option is to display the error directly on the create dialog, similar to RenameRunFormView. So users can see the error and update the experiment's name right there. What do you think?

CC @Zangr Shall we look at all the different error handlings and put together a guideline for when to use what components?

[ggliem]

@gioa The time the message is shown is configurable, so we could pick an appropriate time such that it doesn't dismiss itself before users finish reading.

I am not sure about the option to display it directly on the dialog. I personally like the message option better. I think a general guideline makes sense such that the errors are handled in a consistent way.
Does it make sense to have different guidelines for Create/Rename and Delete? I think for create/rename it definitely makes the most sense to have either a message or display the error directly in the modal but I am not sure whether the same holds for the "delete" modals. What do you think?

[gioa]

Agree the create/rename and the delete can be handled differently. Create and rename's errors can be corrected by the user input while delete is often due to a system error - the user cannot do much about it but retrying.

Just curious why you prefer the message option rather than display it directly on a dialog. Would love to learn what users think. Also for comparison, I mocked the two options as below.

[ggliem]

@gioa Should we agree on handling the deletion errors in such a way that we display this kind of generic error modal?

Thanks for providing the side-by-side comparison. Seeing the direct comparison, I like the mockup in which the error is directly displayed on the dialog. However, one of the reasons I initially did not like the idea of showing it directly in the dialog too much is that the returned error could also be rather technical, e.g. an "Internal Server Error" which does not have to be related to the input field. Hence, I don't know whether the input field should be highlighted. This is how it looked like in the RenameRunFormView which I personally did not like too much:

Another thing we could do is handling the "name duplication" error via form validators, similar to the validator in the RegisterModelForm. We could check for name duplication (while renaming or creating experiments) without calling the API. However, the UI currently only keeps track of the active experiments. To make the behavior consistent while using the validators we would probably also have to keep track of the deleted experiments. We could do so by modifying the listExperiments call a bit but I am not sure whether that's intended (cc @Zangr)? The validator checks correspond to your mockup. Errors like the "Internal Server Error" could then be displayed in a message.

[Zangr]

To make the behavior consistent while using the validators we would probably also have to keep track of the deleted experiments. We could do so by modifying the listExperiments call a bit but I am not sure whether that's intended (cc @Zangr)?

@ggliem Yes, to check the experiment on the client side we can do an on-demand list_experiments call with an extra query parameter ?viewType=ALL and leverage the async validator built-in with antd form to do the validation against current name in the form input.

[Zangr]

CC @Zangr Shall we look at all the different error handlings and put together a guideline for when to use what components?

@gioa Yes, we should. Overall my feeling is that errors from user actions fit more naturally with the pop ups while 4xx/5xx errors from accessing the main resource of the page during initial visiting can have a dedicated 4xx/5xx page for them.

[ggliem]

@ggliem Yes, to check the experiment on the client side we can do an on-demand list_experiments call with an extra query parameter ?viewType=ALL and leverage the async validator built-in with antd form to do the validation against current name in the form input.

@Zangr Do you think it makes more sense to call listExperiments when rendering the modals (Rename/Create) (i.e. on-demand) or keep the deleted experiments in the global store as well? If doing on-demand calls I would add the query parameter ?viewType=DELETED_ONLY since the active ones are part of the Redux store anyways.

[gioa]

@ggliem Thank you for explaining! Your reasons are quite legit and I agree that the internal server error does not relate to the input, displaying it in a message on the top makes more sense in terms of consistency (I remember seeing internal server error on a message).

I like your suggestion of using the form validator to check the input and display an input error directly on the modal. For other system errors like internal server error, we can display in a message. I also assume deletion errors are not input errors, so we can display them in a message as well.

[ggliem]

I think it's more easily reproducible when there are larger experiments. [...]
It's likely caused by the concurrent requests here:
Promise.all([createExperimentPromise, listExperimentsPromise])
I'd recommend putting list experiment in a then block after create experiment, that way we can make sure list experiment gets up-to-date information.

I could not reproduce the issue locally, however, it was likely caused by the mentioned issue since that did not guarantee the order in which the promises were resolved. That being said, I do not think that the suggested code works since the state of createExperimentApi promise has to be shared through the promise chain. I pushed a change - could you please check whether you still get the error?

Also, it would be nice to add a reducer entry for CREATE_EXPERIMENT_API as well.

Could you please clarify what you would expect this reducer to do? I feel like relying on the follow-up LIST_EXPERIMENT_API action to update the state is fine since some things that are not captured by the createExperimentApi call could change in the future (e.g. default artifact location, additional field when creating an experiment...).

[Zangr]

I could not reproduce the issue locally, however, it was likely caused by the mentioned issue since that did not guarantee the order in which the promises were resolved. That being said, I do not think that the suggested code works since the state of createExperimentApi promise has to be shared through the promise chain. I pushed a change - could you please check whether you still get the error?

I got your point! However, it's a bit awkward to have to pass down using Promise.all. What do you think about the following implementation with async/await? Also, actually we are not using request ids in this component so we can actually remove them for simplicity.

  handleCreateExperiment = async (values) => {
    // get values of input fields
    const experimentName = values[EXP_NAME_FIELD];
    const artifactLocation = values[ARTIFACT_LOCATION];
    const response = await this.props.createExperimentApi(experimentName, artifactLocation);
    await this.props.listExperimentsApi();
    const { value: { experiment_id: newExperimentId } } = response;
    if (newExperimentId) {
      this.props.history.push(Routes.getExperimentPageRoute(newExperimentId));
    }
  };

To use the above syntax, you may want to update .eslintrc.js with the following:

    'space-before-function-paren': [2, {
      'anonymous': 'always',
      'named': 'never',
      'asyncArrow': 'always',
    }],

Could you please clarify what you would expect this reducer to do? I feel like relying on the follow-up LIST_EXPERIMENT_API action to update the state is fine since some things that are not captured by the createExperimentApi call could change in the future (e.g. default artifact location, additional field when creating an experiment...).

You are right. Too bad we don't have the complete experiment metadata in the response. We do need the listing for now. No change is needed for the reducer.

[ggliem]

I got your point! However, it's a bit awkward to have to pass down using Promise.all. What do you think about the following implementation with async/await? Also, actually we are not using request ids in this component so we can actually remove them for simplicity.

I agree, I initially also looked at the async/await pattern but stuck to the Promise.all implementation since async/await was not used elsewhere in the application. It looks much cleaner now, thanks for the feedback.

[ggliem]

Python tests seem to be failing again for the last commits

[harupy]

The CI fails when installingfbprophet.
It seems fbprophet dropped python 2 support in the latest version (0.6).

Related issue:
facebook/prophet#1339

fbprophet release history:
https://pypi.org/project/fbprophet/0.6/#history

[Zangr]

LGTM Thank you @ggliem, this is a huge UX improvement for experiment CRUD!
Ready to merge after we did the python build fix.