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 2 commits
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?