Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add share button in experiment view #4936
Changes from 1 commit
3a76591
628cde6
21177f1
0054928
daf1e33
b98baf9
67e89ef
2b13cba
4547ed8
918f792
d2d54ae
ff72a0c
962ac4f
96460ce
6289ac3
d6eec5a
0d79e46
57e5288
9f98959
8996132
9c07ac4
6428c91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tonew ExperimentViewPersistedState().toJSON()
and the URL state is not considered.https://github.com/mlflow/mlflow/pull/4936/files#diff-371936974df23203064a143d69e994cc78e7d7ef09a0b369044c0930218290f2R244
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theExperimentViewPersistedState
properties should be with order of precedence:urlState
I can update the PR such that the url query params are parsed in ExperimentPage and passed as a prop
urlState
to ExperimentView instead oflocation
but we would still use it in the same way wrt to ExperimentViewPersistedState. Would that help?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to
getDerivedStateFromProps
, isn't the switching betweenExperimentViewPersistedState
handled by the update of theexperiment
prop inExperimentView
? Which in turn leads to loading in the newExperimentViewPersistedState
according to the above mentioned precedence in the constructor ofExperimentView
.I'm still quite new to React so maybe I'm reading the situation wrong 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theExperimentViewPersistedState
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 theExperimentViewPersistedState
correctly as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we need to put
updateUrlWithViewState
as a pure function inExperimentViewUtil
? 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 toExperimentViewUtil.updateUrlWithViewState
along with props and state. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunishsheth2009 thanks for your comments. I've now moved the
updateUrlWithViewState
function to theExperimentPage
. 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding something like this in experiment page in
getDerivedStateFromProps
function might help with maintaining state.also the code for
This can be reused in constructor as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromExperimentViewPersistedState
back intoExperimentPagePersistedState
, and pass in the needed values toExperimentView
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofExperimentPage
state in localstorage. Curious to hear your what you think!