feat: Add/remove HPs when creating experiment through HP search #9610
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 #9610 +/- ##
==========================================
- Coverage 51.37% 46.31% -5.06%
==========================================
Files 1252 929 -323
Lines 152174 123093 -29081
Branches 3023 3026 +3
==========================================
- Hits 78181 57015 -21166
+ Misses 73835 65920 -7915
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| const [currentHPs, setCurrentHPs] = | ||
| useState<{ hyperparameter: Hyperparameter; name: string }[]>(); | ||
| useEffect(() => { | ||
| !currentHPs && hyperparameters && setCurrentHPs(hyperparameters); | ||
| }, [hyperparameters, currentHPs]); | ||
|
|
There was a problem hiding this comment.
can we use the form state for this instead?
There was a problem hiding this comment.
I might be wrong, but I don't think the state for this form is explicitly defined due to complexity
There was a problem hiding this comment.
Deleting a hyperparameter causes ripple effects to non-constant hps later in the list.
https://www.loom.com/share/1c4bc85dfa5c43dcaa9d13a63119d2ad?sid=37875841-aac9-4ba4-8ac6-a8909ce58abb
| const hpName = hp[0]; | ||
| if (!currentHPs?.map((h) => h.name).includes(hpName)) return; |
There was a problem hiding this comment.
nit: could simplify to one loop with something like !currentHPs?.some((h) => h.name === hpName)
| const [currentHPs, setCurrentHPs] = | ||
| useState<{ hyperparameter: Hyperparameter; name: string }[]>(); | ||
| useEffect(() => { | ||
| !currentHPs && hyperparameters && setCurrentHPs(hyperparameters); | ||
| }, [hyperparameters, currentHPs]); | ||
|
|
There was a problem hiding this comment.
sugg: this useEffect is only being used to set the initial state, and since hyperparameters isn't referred to elsewhere, I believe this whole section can be rewritten to something like:
const calculateInitialHyperparameters = useCallback(() => {
return Object.entries(experiment.hyperparameters).map((hp) => {
const hpObject = { hyperparameter: hp[1], name: hp[0] };
if (trialHyperparameters?.[hp[0]]) {
hpObject.hyperparameter.val = trialHyperparameters[hp[0]];
}
return hpObject;
});
}, [experiment.hyperparameters, trialHyperparameters]);
const [currentHPs, setCurrentHPs] =
useState<{ hyperparameter: Hyperparameter; name: string }[]>(calculateInitialHyperparameters);
| prev | ||
| ? [...prev, { hyperparameter: emptyHP, name: `hp_${prev.length}` }] | ||
| : [{ hyperparameter: emptyHP, name: 'hp_0' }], |
There was a problem hiding this comment.
sugg: you could do [...(prev ?? []), { hyperparameter: emptyHP, name: `hp_${prev?.length ?? 0}` }]
There was a problem hiding this comment.
But that's personal taste, up to you.
| <p>Select hyperparameters and define the search space.</p> | ||
| <p> | ||
| Select hyperparameters and define the search space. <br /> | ||
| The experiment code needs to be able to handle hyperparameters for them to take effect.{' '} |
There was a problem hiding this comment.
nit: don't need the {' '}
Ticket
MD-402
Description
Allow user to add or remove hyperparameters when creating experiment through HP search
Test Plan
Navigate to
Hyperparameterstab of an experiment, click on theHyperparameters SearchbuttonWhen selecting hyperparameters, add and remove hyperparameters
Click on the
Run Experimentbutton to create a experimentVerify that the new experiment is using the updated hyperparameters
Screen.Recording.2024-07-09.at.1.48.24.PM.mov
Checklist
docs/release-notes/See Release Note for details.