chore: migrate continue trial form and remove steps [DET-3780 DET-3854] #1050

hamidzr · 2020-08-11T00:29:21Z

Description

add continue trial form
migrate from max_steps to max_length

Test Plan

old experiment config:

after config upgrade:

Commentary (optional)

webui/react/src/pages/TrialDetails.tsx

hkang1 · 2020-08-11T05:32:14Z

webui/react/src/types.ts

@@ -1,3 +1,6 @@
+/* eslint-disable-next-line @typescript-eslint/no-explicit-any */
+export type RawJson = Record<string, any>;


question: probably a good reason for this, but just curious of as to using any instead of unknown

if it's unknown we can't access any level in the object other than the first level. may if there was a way of defining a recursive Record<string, unknown>; type?

unknown was working fine because clone actually turns it into any. open to suggestions :)

I see, let's leave it

hkang1 · 2020-08-11T05:38:12Z

webui/react/src/pages/TrialDetails.tsx

 const TrialDetailsComp: React.FC = () => {
  const { trialId: trialIdParam } = useParams<Params>();
  const trialId = parseInt(trialIdParam);
  const [ trial, triggerTrialRequest ] =
    useRestApi<TrialDetailsParams, TrialDetails>(getTrialDetails, { id: trialId });
  const [ contModalVisible, setContModalVisible ] = useState(false);
  const [ contModalConfig, setContModalConfig ] = useState('Loading');
+  const [ contFormVisible, setContFormVisible ] = useState<boolean>(false);
+  const [ contMaxLength, setContMaxLength ] = useState<number>(100);


question: I asked about whether there is a good default value to set to for max length and the answer was "not really". Can we leave it blank so it forces the user to have to make that decision. @vishnu2kmohan @stoksc what are your thoughts on the default value of 100 for max length / batches?

Yeah, I don't think there's a great default for how long to continue the trial. 100 batches is probably just fine.

What if the user specified records and epochs instead of batches in the base trial?
The user would expect that the unit from the base trial be carried over?

If we want that, then maybe if records, 100 * global_batch_size, if batches , 100 and if epochs math.ceil((100 * global_batch_size) / records_per_epoch). But in the event global_batch_size, we pick the max?

But I think just not having a default is better, at that point.

Can we leave it blank

yes, and it'd fall back to what the experiment had set before which is the same behavior as the existing trial page. so I thnk this should be fine. does this sound right?

webui/react/src/pages/TrialDetails.tsx

stoksc · 2020-08-12T04:01:18Z

webui/react/src/pages/TrialDetails.tsx

+    try {
+      const expConfig = clone(experiment.configRaw);
+      expConfig.description = formValues.description;
+      setTrialLength(expConfig, parseInt(formValues.maxLength));


We may also need to shim min_validation_period and min_checkpoint_period in the same way.

@stoksc looks like min_validation_period and min_checkpoint_period are the same format as max_length? Where they can be batches, epochs or records?

Reference:
https://github.com/determined-ai/determined/compare/remove-steps

yep, they are.

would these also apply to experiment forking/creation? if so I think we should address it separately where it affects both workflows.

hkang1 · 2020-08-12T19:48:58Z

webui/react/src/ErrorHandler.ts

@@ -26,6 +26,7 @@ export enum ErrorLevel {
 export enum ErrorType {
  Server = 'server', // internal apis and server errors.
  Auth = 'auth',
+  Deprecated = 'deprecated',


question: looks like we're not using this yet. Is the plan to use this in the near future or this is just a forgotten loose end during PR dev?

hmm I go back and forth on it. some of the errors we run into about experiment config could be due to depracated attributes but we could also use Api type

I see, in that case I think the more info the better. Meaning knowing that it's deprecation error, separate from API error is a good thing.

webui/react/src/components/CreateExperimentModal.tsx

hkang1 · 2020-08-12T20:14:10Z

webui/react/src/pages/TrialDetails.tsx

+  return Object.entries(maxLength)[0] as [string, number];
+};
+
+const setTrialLength = (experimentConfig: RawJson, length: number): void => {


suggestion: how about returning new RawJson (cloned) instead of mutating the exiting one?
Something like:

const setTrialLength = (experimentConfig: RawJson, length: number): RawJson => { const newConfig = clone(experimentConfig); ... do stuff to clone return newConfig; };

in the current usecase (and some future ones) the pattern can provide an already cloned version. I didn't clone here to avoided double cloning. I'm thinking the use can always provide a cloned object if they want to? or if the cloning isn't expensive we can always do it.

ah if it's already cloned, you're right, no need to do it again.

hkang1 · 2020-08-12T20:17:38Z

nit: wdy think about Create Trial for the modal confirmation button label instead of Create?

hkang1

This PR looks good and worked great. Wrote some comments and questions throughout.

hamidzr · 2020-08-13T19:06:35Z

nit: wdy think about Create Trial for the modal confirmation button label instead of Create?

hmm it's actually creating a single trial experiment so that could be confusing..

Continue trial? or just continue.

but then continue is less strong than creatte

webui/react/src/utils/types.ts

webui/react/src/components/CreateExperimentModal.tsx

hkang1

Love the backwards compatibility! Two small nits.

pick 7ff1aa7 pr feedback part 1 pick cf64b18 delegate error view to createexperiment modal pick 9e09845 clean up and make it reset on cancel pick ee237da upgrade config for experiment detail as well pick 20be1bc use Api type for config errors instead of Deprecated pick 93c0a9f7 sort some props and use isNumber utility

cla-bot bot added the cla-signed label Aug 11, 2020

hamidzr changed the title ~~3780 cont trial form~~ chore: migrate continue trial form and remove steps [DET-3780] Aug 11, 2020

hamidzr requested a review from hkang1 August 11, 2020 00:30

hamidzr assigned hkang1 Aug 11, 2020

hkang1 reviewed Aug 11, 2020

View reviewed changes

webui/react/src/pages/TrialDetails.tsx Show resolved Hide resolved

hkang1 reviewed Aug 11, 2020

View reviewed changes

webui/react/src/pages/TrialDetails.tsx Outdated Show resolved Hide resolved

hkang1 reviewed Aug 11, 2020

View reviewed changes

webui/react/src/pages/TrialDetails.tsx Outdated Show resolved Hide resolved

stoksc reviewed Aug 12, 2020

View reviewed changes

hkang1 reviewed Aug 12, 2020

View reviewed changes

webui/react/src/components/CreateExperimentModal.tsx Outdated Show resolved Hide resolved

hkang1 reviewed Aug 12, 2020

View reviewed changes

hkang1 assigned hamidzr and unassigned hkang1 Aug 12, 2020

hamidzr force-pushed the 3780-cont-trial-form branch from 7ecbbb1 to 74ddeb9 Compare August 13, 2020 18:48

hamidzr force-pushed the 3780-cont-trial-form branch from 0b5e121 to ee237da Compare August 13, 2020 23:53

hamidzr changed the title ~~chore: migrate continue trial form and remove steps [DET-3780]~~ chore: migrate continue trial form and remove steps [DET-3780 DET-3854] Aug 14, 2020

hamidzr requested a review from hkang1 August 14, 2020 00:03

hamidzr assigned hkang1 and unassigned hamidzr Aug 14, 2020

hkang1 reviewed Aug 14, 2020

View reviewed changes

webui/react/src/utils/types.ts Outdated Show resolved Hide resolved

hkang1 reviewed Aug 14, 2020

View reviewed changes

webui/react/src/utils/types.ts Outdated Show resolved Hide resolved

hkang1 reviewed Aug 14, 2020

View reviewed changes

webui/react/src/components/CreateExperimentModal.tsx Show resolved Hide resolved

hkang1 approved these changes Aug 14, 2020

View reviewed changes

hkang1 assigned hamidzr and unassigned hkang1 Aug 14, 2020

hamidzr added 2 commits August 14, 2020 14:39

enable find in monaco

6a768cf

hamidzr force-pushed the 3780-cont-trial-form branch from 20be1bc to 92ff9db Compare August 14, 2020 22:25

hamidzr merged commit 065c1b0 into determined-ai:master Aug 14, 2020

hamidzr deleted the 3780-cont-trial-form branch August 14, 2020 22:45

dannysauer added this to the 0.13.0 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate continue trial form and remove steps [DET-3780 DET-3854] #1050

chore: migrate continue trial form and remove steps [DET-3780 DET-3854] #1050

hamidzr commented Aug 11, 2020 •

edited

Loading

hkang1 Aug 11, 2020

hamidzr Aug 13, 2020 •

edited

Loading

hkang1 Aug 14, 2020

hkang1 Aug 11, 2020

stoksc Aug 11, 2020

vishnu2kmohan Aug 11, 2020

stoksc Aug 11, 2020

stoksc Aug 11, 2020

hamidzr Aug 13, 2020

stoksc Aug 12, 2020

hkang1 Aug 12, 2020 •

edited

Loading

stoksc Aug 12, 2020

hamidzr Aug 13, 2020 •

edited

Loading

hkang1 Aug 12, 2020

hamidzr Aug 13, 2020 •

edited

Loading

hkang1 Aug 14, 2020

hkang1 Aug 12, 2020

hamidzr Aug 13, 2020

hkang1 Aug 14, 2020

hkang1 commented Aug 12, 2020

hkang1 left a comment

hamidzr commented Aug 13, 2020 •

edited

Loading

hkang1 left a comment •

edited

Loading

		@@ -1,3 +1,6 @@
		/* eslint-disable-next-line @typescript-eslint/no-explicit-any */
		export type RawJson = Record<string, any>;

chore: migrate continue trial form and remove steps [DET-3780 DET-3854] #1050

chore: migrate continue trial form and remove steps [DET-3780 DET-3854] #1050

Conversation

hamidzr commented Aug 11, 2020 • edited Loading

Description

Test Plan

Commentary (optional)

Choose a reason for hiding this comment

hamidzr Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkang1 Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkang1 commented Aug 12, 2020

hkang1 left a comment

Choose a reason for hiding this comment

hamidzr commented Aug 13, 2020 • edited Loading

hkang1 left a comment • edited Loading

Choose a reason for hiding this comment

hamidzr commented Aug 11, 2020 •

edited

Loading

hamidzr Aug 13, 2020 •

edited

Loading

hkang1 Aug 12, 2020 •

edited

Loading

hamidzr Aug 13, 2020 •

edited

Loading

hamidzr Aug 13, 2020 •

edited

Loading

hamidzr commented Aug 13, 2020 •

edited

Loading

hkang1 left a comment •

edited

Loading