Add share button in experiment view

[marijncv]

Signed-off-by: Marijn Valk marijncv@hotmail.com

What changes are proposed in this pull request?

Added a button that will copy the current experiment view state to your clipboard so it can be shared. closes #4820

How is this patch tested?

Open an experiment, change some state (order by, column selected etc.) use the share button and open in another browser.

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.

Introduced a "share" button that can be used to share a view / configuration of the MLflow experiment UI with other users

What component(s), interfaces, languages, and integrations does this PR affect?

Components

• area/artifacts: Artifact stores and artifact logging
• area/build: Build and test infrastructure for MLflow
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/projects: MLproject format, project running backends
• area/scoring: MLflow Model server, model deployment tools, Spark UDFs
• area/server-infra: MLflow Tracking server backend
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

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

[marijncv]

Hey @sunishsheth2009, @dbczumar, I tried a bit of a different approach in this PR in comparison with the previous draft PR.

This prototype maintains the state of the below elements in the url:

• search
• startTime
• orderByKey
• orderByAsc
• showMultiColumns
• categorizedUncheckedKeys
• diffSwitchSelected
• preSwitchCategorizedUncheckedKeys
• postSwitchCategorizedUncheckedKeys

Also, it creates the share button that copies the current state to the clipboard of the user, and shows a small notification of this.

Some points I'm wondering about:

• Is the button in the correct place?
• Should it have an icon in it?
• Does the notification message have the correct content and is it styled correctly?
• Are there any other state elements that should be included in the share button/url?

[sunishsheth2009]

Is there a reason we need to store this in a state here? Can the parent parse the location and send down the parsed location as props so we don't need to maintain one more state.

Also assuming we need to maintain state here, we might need to correctly set the state again in getDerivedStateFromProps where we set the persistantState to new ExperimentViewPersistedState().toJSON() and the URL state is not considered.
https://github.com/mlflow/mlflow/pull/4936/files#diff-371936974df23203064a143d69e994cc78e7d7ef09a0b369044c0930218290f2R244

[marijncv]

Can you clarify what you mean with one more state? In my view we are storing the same number of state variables as before the PR (the properties defined in ExperimentViewPersistedState). Only now we take into account the urlState when deciding what the values of the ExperimentViewPersistedState properties should be with order of precedence:

1. urlState
2. State persisted in localstorage
3. Default values

I can update the PR such that the url query params are parsed in ExperimentPage and passed as a prop urlState to ExperimentView instead of location but we would still use it in the same way wrt to ExperimentViewPersistedState. Would that help?

[marijncv]

With regards to getDerivedStateFromProps, isn't the switching between ExperimentViewPersistedState handled by the update of the experiment prop in ExperimentView? Which in turn leads to loading in the new ExperimentViewPersistedState according to the above mentioned precedence in the constructor of ExperimentView.

I'm still quite new to React so maybe I'm reading the situation wrong 😊

[sunishsheth2009]

I think it would help to parse the URL in experimentPage and sending the state down which can be used as default state for the current values that we have.

Regarding the getDerivedStateFromProps, when the experiment changes, I don't think the constructor is called again. In that case, do we need the URL state to correctly update the ExperimentViewPersistedState correctly? We can also test this usecase by click the back button of the browser and seeing that the URL state changing, does that change the ExperimentViewPersistedState correctly as well?

[sunishsheth2009]

Is there a reason we need to put updateUrlWithViewState as a pure function in ExperimentViewUtil? Can we just put in experimentView where we don't need to pass in props and state to that function, we just need to pass in the new URL state?

If we really need a pure function then we can create a wrapper function of updateUrlWithViewState which takes in the new diff URL state and passes that data to ExperimentViewUtil.updateUrlWithViewState along with props and state. What do you think?

[marijncv]

The main reason was actually that the linter started flagging that the ExperimentView.js file had more than 1500 lines. I've added a wrapper function in the latest commit. Curious to hear your thoughts!

[sunishsheth2009]

I think since we discussed to maintain the URLState in experimentPage, updating the URLState can also be done in experimentPage as well. So we can move updateUrlWithViewState function to experimentPage where we can update the URL and passed the new parse state down to experimentView. Let me know what you think about that

[marijncv]

@sunishsheth2009 thanks for your comments. I've now moved the updateUrlWithViewState function to the ExperimentPage. Although, I feel I'm a bit stuck on dealing with the user changing the url manually (or pressing the back button in the browser). Can you provide some pointers (or suggestions on my branch) on how to do that gracefully?

I'll fix the failing tests after you feel my attempt is going in the right direction!

[sunishsheth2009]

Adding something like this in experiment page in getDerivedStateFromProps function might help with maintaining state.

if (props.location.search !== state.urlState) {
      return {
        lastRunsRefreshTime: Date.now(),
        numberOfNewRuns: 0,
        nextPageToken: null,
        persistedState: { ...getPersistedStateFromUrl(props.location.search) },
        urlState: props.location.search,
        searchRunsRequestId: getUUID(),
        ...PAGINATION_DEFAULT_STATE,
      };
    }

also the code for

const getPersistedStateFromUrl = (url) => {
 const urlState = Utils.getSearchParamsFromUrl(url);

 return {
   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',
   startTime: urlState.startTime === undefined ? DEFAULT_START_TIME : urlState.startTime,
 };
};

This can be reused in constructor as well

[marijncv]

Thanks for the pointer @sunishsheth2009 ! Will give it another go.

With the goal of maintaining the state in ExperimentPage, wouldn't it make sense now to lift the urlState elements from ExperimentViewPersistedState back into ExperimentPagePersistedState, and pass in the needed values to ExperimentView as props?

Referring to:

• showMultiColumns
• categorizedUncheckedKeys
• diffSwitchSelected
• preSwitchCategorizedUncheckedKeys
• postSwitchCategorizedUncheckedKeys

Having the urlState spread across the two persisted state objects is quite confusing to deal with from my perspective. Or maybe I'm still missing a piece of the puzzle.

In any case, will try and take some time over the weekend to work it out

[sunishsheth2009]

With the goal of maintaining the state in ExperimentPage, wouldn't it make sense now to lift the urlState elements from ExperimentViewPersistedState back into ExperimentPagePersistedState, and pass in the needed values to ExperimentView as props?

Yes that would be great! :) I like that idea of maintaining the state in one place so it can update it correctly.
Thank you. let me know if you need any help from my side

[marijncv]

Thanks again for the pointer @sunishsheth2009, I've now updated the PR to maintain the urlState in ExperimentPage. In the end it became quite a refactor, and I've also (re-)enabled the storing of ExperimentPage state in localstorage. Curious to hear your what you think!

[dbczumar]

@marijncv I tried this out locally, and it's fantastic! I'll ask our UX designer for some input on the location / appearance of the button & clipboard copy workflow; I should have an update in the next couple of days.

Would it be possible to incorporate the following pieces of Filter state into the URL?

• Run lifecycle stage (state): "Active" or "Deleted
• Linked Models: ["All Runs", "With Model Versions", "Without Model Versions"]

[tytownley]

Hi @marijncv. I am the UX designer @dbczumar mentioned. @dbczumar and I recommend placing the 'Share' button in the page title to the far right of the screen center-aligned to the page title text.

I've attached a few design artifacts to help communicate how to implement the share button.

Thanks for your suggestions!

[marijncv]

Thanks @tytownley & @dbczumar for the recommendations! I've updated the placement of the button in the latest commit.

[BenWilson2]

Hi @marijncv the test failure that was shown in your CI should be resolved in master now. Feel free to pull and resubmit. Sorry for the inconvenience.

[dbczumar]

@marijncv Thanks so much for updating the styling - looks awesome!

I left one small comment here: #4936 (comment). I also ran into a couple small issues demonstrated in the following video:

Screen.Recording.2021-11-04.at.3.41.59.PM.mov

1. When navigating between experiments or reopening the experiment in a new window via the UI, state kept in local storage isn't reflected in the experiment URL, so the "Share" button doesn't record the state.

2. While not directly related to this PR, column ordering state is not kept in local storage (perhaps we can address that as a follow-up).

It would be fantastic to address (1) as part of this PR. Thank you so much for your contributions!

[marijncv]

@dbczumar thanks for the comment! I think I've addressed point 1 in the latest commit, and would be happy to look into point 2 in a follow up PR.

I noticed one more issue with the current implementation that I'd like to hear your thoughts on. Right now, some state elements are only encoded into the url if their value is non-default. For example: diffSwitchSelected will only be encoded in the URL if it's true and not if it's false. This keeps the URL shorter, but can cause an issue if user 1 copies the URL, and
user 2 has a local storage state different from the default value. In that case, the current implementation will use the value that is in the local storage of user 2. In this scenario, the views of user 1 and 2 are different.

Two possible solutions would be:

1. Encode state in the URL even if the values are default values
2. During parsing of the URL assign the default value for missing values in the URL

Curious to hear what you would prefer, or if you see another solution to this issue!

[dbczumar]

@dbczumar thanks for the comment! I think I've addressed point 1 in the latest commit, and would be happy to look into point 2 in a follow up PR.

I noticed one more issue with the current implementation that I'd like to hear your thoughts on. Right now, some state elements are only encoded into the url if their value is non-default. For example: diffSwitchSelected will only be encoded in the URL if it's true and not if it's false. This keeps the URL shorter, but can cause an issue if user 1 copies the URL, and user 2 has a local storage state different from the default value. In that case, the current implementation will use the value that is in the local storage of user 2. In this scenario, the views of user 1 and 2 are different.

Two possible solutions would be:

1. Encode state in the URL even if the values are default values
2. During parsing of the URL assign the default value for missing values in the URL

Curious to hear what you would prefer, or if you see another solution to this issue!

@marijncv Thank you for addressing the previous comment and proposing solutions here. I think that (1) sounds like the least surprising approach; it ensures that the URL remains the source of truth without any ambiguity. Can we try this out?

cc @sunishsheth2009 for verification

[sunishsheth2009]

@dbczumar thanks for the comment! I think I've addressed point 1 in the latest commit, and would be happy to look into point 2 in a follow up PR.
I noticed one more issue with the current implementation that I'd like to hear your thoughts on. Right now, some state elements are only encoded into the url if their value is non-default. For example: diffSwitchSelected will only be encoded in the URL if it's true and not if it's false. This keeps the URL shorter, but can cause an issue if user 1 copies the URL, and user 2 has a local storage state different from the default value. In that case, the current implementation will use the value that is in the local storage of user 2. In this scenario, the views of user 1 and 2 are different.
Two possible solutions would be:

1. Encode state in the URL even if the values are default values
2. During parsing of the URL assign the default value for missing values in the URL

Curious to hear what you would prefer, or if you see another solution to this issue!

@marijncv Thank you for addressing the previous comment and proposing solutions here. I think that (1) sounds like the least surprising approach; it ensures that the URL remains the source of truth without any ambiguity. Can we try this out?

cc @sunishsheth2009 for verification

Yup I agree with that solution that maintaining all the state in the URL as the source of truth is a great idea.

[sunishsheth2009]

I am curious on how would this work as a backwards compatible change? Given that users have already set some state in local storage with experimentView. With this change, I think those state would be not read and it would break the filters/settings that users might have?

Let me know if I am missing something

[sunishsheth2009]

Is there a reason we need to switch to using here? cc @xanderwebs

[xanderwebs]

I think ideally we wouldn't touch this component (and sizes below)since it's shared across many different pages

Also, this amendment doesn't seem to do anything extra really, since according to https://ant.design/components/button/, default is not a valid size.

[marijncv]

@sunishsheth2009, @xanderwebs, reverted the change to the export of HeaderButton (also removing the size='default' part).

Wrt to the changes in the CSS part: I thought this would be the best way to make the design adhere to the sketch provided by @tytownley. But now indeed this change is reflected on the other pages that make use of the PageHeader component too. Should we add some separate styling that only applies to the PageHeader in the ExperimentView? Or is it alright to have the same style changes on the other pages that make use of the PageHeader?

The main effect of the styling is that the headerbutton has a slightly different padding, and that the margin below the pagetitle is 16px instead of 8px.

Curious to hear your thoughts!

[xanderwebs]

@marijncv Could we do something that specifically affects the Share button? I think adding scoped global styles on buttons would be potentially annoying to deal with if we have any other buttons in the header.

Maybe something like

function ShareButton(props) {
   return <HeaderButton className={css(...buttonOverrides)} {...props} />;
}

[marijncv]

@xanderwebs thanks for the suggestion! In the latest commit I've removed the style changes in the PageHeader component and created a local style definition for the sharebutton only in the ExperimentView component

[sunishsheth2009]

Why do we care if its a POP or some navigation in the future that changes the URL state. We could just care if the URL changed from the previous state, we can just update the local state? What do you think?

[marijncv]

Tried for a while to get it to work like that, but ended up in the situation where the isLoading prop was always true (obscuring the view with a spinner and causing all sorts of problems).

I can take another look. It would probably require the state handling functions to be changed to no longer use the this.setState({}) function and instead use updateUrlWithViewState(). This would cause an url change, which will be loaded into the component state in getDerivedStateFromProps

[sunishsheth2009]

Why do we want to maintain the multicolumn toggle state?
@dbczumar what do you think about this requirement?

[marijncv]

It's something that was already in place before this PR. Personally I think it makes sense from a UX perspective as well assuming that most users have a preferred view (either the multicolumn view or the regular view). Clicking the clear button then, should not toggle/reset this view

[dbczumar]

I think we should preserve it, given that we did so previously

[marijncv]

I am curious on how would this work as a backwards compatible change?

Indeed the state elements that were previously in localstorage of ExperimentView for a user would be ignored and will show their default values the first time visiting the page. After that, the ExperimentView state in localstorage will no longer contain these elements, and ExperimentPage will contain them.

The elements for which this would be the case are:

• showMultiColumns
• categorizedUncheckedKeys
• diffSwitchSelected
• preSwitchCategorizedUncheckedKeys
• postSwitchCategorizedUncheckedKeys

As a countermeasure we could try to implement some logic that tries to look for these elements in ExperimentView localstorage and use them instead of defaults of ExperimentPagePersistedState. This logic could be removed later if we feel confident enough that all users have visited their experiment tracker at least once, since that will prompt the state to be kept in ExperimentPage. What do you think?

[dbczumar]

I am curious on how would this work as a backwards compatible change?

Indeed the state elements that were previously in localstorage of ExperimentView for a user would be ignored and will show their default values the first time visiting the page. After that, the ExperimentView state in localstorage will no longer contain these elements, and ExperimentPage will contain them.

The elements for which this would be the case are:

• showMultiColumns
• categorizedUncheckedKeys
• diffSwitchSelected
• preSwitchCategorizedUncheckedKeys
• postSwitchCategorizedUncheckedKeys

As a countermeasure we could try to implement some logic that tries to look for these elements in ExperimentView localstorage and use them instead of defaults of ExperimentPagePersistedState. This logic could be removed later if we feel confident enough that all users have visited their experiment tracker at least once, since that will prompt the state to be kept in ExperimentPage. What do you think?

IMO, I think it's okay to make this a backwards-incompatible change that resets experiment pages to the default view, especially given that certain important parts of the experiment page state (e.g. column sort order) are not persisted in local storage. @sunishsheth2009 @marijncv let me know if you feel strongly that we need a backwards compatibility solution.

[dbczumar]

@marijncv From a functional perspective, everything looks great to me! Thanks
so much for your contribution! @sunishsheth2009 @xanderwebs can you take another pass and leave remaining feedback? I'd imagine we'll be almost ready to merge once additional comments are addressed.

[marijncv]

@marijncv let me know if you feel strongly that we need a backwards compatibility solution.

@dbczumar I agree with your assessment

[sunishsheth2009]

Except the mlflow/server/js/src/shared/building_blocks/PageHeader.jsx change, all the other code looks good to me.
I would defer to @xanderwebs to review that file, since he owns that component.

Thank you @marijncv for making this change and adding this feature. This is amazing.

[xanderwebs]

Thanks for the changes!

[dbczumar]

Thanks so much for your contribution, @marijncv ! This is super exciting!