-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: continue trial from WebUI for multi-trial experiment #9589
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9589 +/- ##
==========================================
- Coverage 49.82% 45.21% -4.61%
==========================================
Files 1247 923 -324
Lines 162287 121992 -40295
Branches 2888 2893 +5
==========================================
- Hits 80855 55160 -25695
+ Misses 81260 66660 -14600
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 its working correctly, but let me try to understand it more.
looks like this experiment (before this change) doesnt show continue button even though its a multi experiment with ASHA and cancelled.
do you have an example how it works right now?
webui/react/src/utils/experiment.ts
Outdated
experiment?.numTrials === 1 || | ||
(experiment.state !== RunState.Completed && | ||
experiment.numTrials > 1 && | ||
['random', 'grid'].includes(experiment.config?.searcher.name || ''))) && |
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.
maybe ['random', 'grid']
can be moved out to the component with a good variable name, then export it for both components?
and this can be set instead of array
i'll defer the decision to you
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.
Sure, added a new variable named ContinuableNonSingleSearcherName
}); | ||
const newPath = paths.experimentDetails(experiment.id); | ||
routeToReactUrl(paths.reload(newPath)); | ||
} finally { |
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.
no catch
clause?
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.
probably worth notifying the user if the API fails to continue the experiment
@@ -263,6 +265,19 @@ const ExperimentDetailsHeader: React.FC<Props> = ({ | |||
setIsRunningDelete(experiment.state === RunState.Deleting); | |||
}, [experiment.state]); | |||
|
|||
const onClickContinueMultiTrialExp = useCallback(async () => { | |||
try { |
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.
its not formatted? indentation doesnt look aligned
webui/react/src/utils/experiment.ts
Outdated
(!!trial || experiment?.numTrials === 1) && | ||
(!!trial || | ||
experiment?.numTrials === 1 || | ||
(experiment.state !== RunState.Completed && |
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.
experiment.state !== RunState.Completed
is it needed?
we have terminalRunStates.has(experiment.state)
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.
to add to this part, terminalRunStates
include:
RunState.Canceled,
RunState.Completed,
RunState.Error,
RunState.DeleteFailed,
RunState.Deleted,
It seems like if it's Completed
or Deleted
it probably can not continue the trials. Might be worth defining a new curated list called continuableRunStates
?
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.
Continue on multi trial experiment is only available for Error
and Cancel
state, so I ended up using the existing erroredRunStates
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.
Most of it looks great, just a few of questions and suggestions
webui/react/src/utils/experiment.ts
Outdated
(!!trial || experiment?.numTrials === 1) && | ||
(!!trial || | ||
experiment?.numTrials === 1 || | ||
(experiment.state !== RunState.Completed && |
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.
to add to this part, terminalRunStates
include:
RunState.Canceled,
RunState.Completed,
RunState.Error,
RunState.DeleteFailed,
RunState.Deleted,
It seems like if it's Completed
or Deleted
it probably can not continue the trials. Might be worth defining a new curated list called continuableRunStates
?
key: 'continue-trial', | ||
label: experiment.unmanaged ? ( | ||
<Tooltip content={UNMANAGED_MESSAGE}>Continue Trial</Tooltip> | ||
) : ( | ||
'Continue Trial' | ||
), | ||
onClick: ContinueTrialModal.open, | ||
onClick: ['random', 'grid'].includes(experiment?.config.searcher.name) ? onClickContinueMultiTrialExp : ContinueTrialModal.open, |
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.
suggestion: looks like ['random', 'grid'].includes(experiment?.config.searcher.name)
is done more than once, could have a memoized variable isSearcherContinuable
or something that does this operation. Or even a more general one that also checks the RunState
called isContinuable
}); | ||
const newPath = paths.experimentDetails(experiment.id); | ||
routeToReactUrl(paths.reload(newPath)); | ||
} finally { |
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.
probably worth notifying the user if the API fails to continue the experiment
Hi @keita-determined not sure I completely understand your concerns, but multi experiments are only able to continue if the searcher is grid or random, so for ASHA experiments this feature won't be available |
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.
\o/
Ticket
MD-437
Description
Allow user to continue multi-trial experiments if applicable.
Multi-trial experiments that are able to continue must satisfy the following criteria:
Test Plan
Navigate the the experiment details page of a multi trial experiment, if the experiment is not able to continue, then the
Continue Experiment
button should not be available.Find a multi-trial experiment with
Continue Experiment
button, clicking on the button will continue the experiment.0627.mp4
Checklist
docs/release-notes/
See Release Note for details.