Skip to content
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

added retry & timeout fields #371

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 72 additions & 1 deletion src/mainviews/create-job.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Scheduler, SchedulerService } from '../handler';
import { useTranslator } from '../hooks';
import { ICreateJobModel, IJobParameter, JobsView } from '../model';
import { Scheduler as SchedulerTokens } from '../tokens';
import { NameError } from '../util/job-name-validation';
import { NameError, MaxRetryAttemptsError, MaxRunTimeError, MaxWaitTimeError } from '../util/job-name-validation';

import { caretDownIcon } from '@jupyterlab/ui-components';

Expand Down Expand Up @@ -191,6 +191,26 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
}
};

const handleNumericInputChange = (event: ChangeEvent<HTMLInputElement>) => {
const target = event.target;

const parameterNameIdx = parameterNameMatch(target.name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is copied from the code for setting parameters, which have user-provided names and values. Your change is intended to add input fields with only user-provided values.

const parameterValueIdx = parameterValueMatch(target.name);
const newParams = props.model.parameters || [];

if (parameterNameIdx !== null) {
newParams[parameterNameIdx].name = target.value;
props.handleModelChange({ ...props.model, parameters: newParams });
} else if (parameterValueIdx !== null) {
newParams[parameterValueIdx].value = +target.value;
props.handleModelChange({ ...props.model, parameters: newParams });
} else {
const value = +target.value;
const name = target.name;
props.handleModelChange({ ...props.model, [name]: isNaN(value)? target.value: value });
}
};

const handleSelectChange = (event: SelectChangeEvent<string>) => {
const target = event.target;

Expand Down Expand Up @@ -495,6 +515,57 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
environmentList={environmentList}
value={props.model.environment}
/>
<TextField
label={trans.__('Maximum Retry Attempts')}
variant="outlined"
onChange={e => {
// Validate name
setErrors({
...errors,
maxRetryAttempts: MaxRetryAttemptsError(e.target.value, trans)
});
handleNumericInputChange(e as ChangeEvent<HTMLInputElement>);
}}
error={!!errors['maxRetryAttempts']}
helperText={errors['maxRetryAttempts'] ?? ''}
value={props.model.maxRetryAttempts}
id={`${formPrefix}maxRetryAttempts`}
name="maxRetryAttempts"
/>
<TextField
label={trans.__('Max Run Time (In Seconds)')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label={trans.__('Max Run Time (In Seconds)')}
label={trans.__('Maximum Run Time (seconds)')}

"Maximum" was not abbreviated above

variant="outlined"
onChange={e => {
// Validate name
setErrors({
...errors,
maxRunTime: MaxRunTimeError(e.target.value, trans)
});
handleNumericInputChange(e as ChangeEvent<HTMLInputElement>);
}}
error={!!errors['maxRunTime']}
helperText={errors['maxRunTime'] ?? ''}
value={props.model.maxRunTime}
id={`${formPrefix}maxRunTime`}
name="maxRunTime"
/>
<TextField
label={trans.__('Max Wait Time (In Seconds)')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label={trans.__('Max Wait Time (In Seconds)')}
label={trans.__('Maximum Wait Time (seconds)')}

variant="outlined"
onChange={e => {
// Validate name
setErrors({
...errors,
maxWaitTime: MaxWaitTimeError(e.target.value, trans)
});
handleNumericInputChange(e as ChangeEvent<HTMLInputElement>);
}}
error={!!errors['maxWaitTime']}
helperText={errors['maxWaitTime'] ?? ''}
value={props.model.maxWaitTime}
id={`${formPrefix}maxWaitTime`}
name="maxWaitTime"
/>
<OutputFormatPicker
label={trans.__('Output formats')}
name="outputFormat"
Expand Down
13 changes: 11 additions & 2 deletions src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ export type ModelWithScheduleFields = {
* Value of input field for past the hour.
*/
scheduleMinute: string;

maxRetryAttempts: number;

maxRunTime: number;

maxWaitTime: number;
};

export interface ICreateJobModel
Expand Down Expand Up @@ -108,7 +114,10 @@ export const defaultScheduleFields: ModelWithScheduleFields = {
scheduleMinute: '0',
scheduleMonthDay: '1',
scheduleWeekDay: '1',
timezone: Intl.DateTimeFormat().resolvedOptions().timeZone
timezone: Intl.DateTimeFormat().resolvedOptions().timeZone,
maxRetryAttempts: 1,
maxRunTime: 172800,
maxWaitTime: 172800,
};

export function emptyCreateJobModel(): ICreateJobModel {
Expand Down Expand Up @@ -267,7 +276,7 @@ export class JobsModel extends VDomModel {

fromJson(data: IJobsModel): void {
this._jobsView = data.jobsView ?? JobsView.ListJobs;
this._createJobModel = data.createJobModel ?? emptyCreateJobModel();
this._createJobModel = data.createJobModel ? { ...defaultScheduleFields, ...data.createJobModel } : emptyCreateJobModel();
this._listJobsModel = data.listJobsModel ?? emptyListJobsModel();
this._jobDetailModel = data.jobDetailModel ?? emptyDetailViewModel();
this._updateJobDefinitionModel =
Expand Down
61 changes: 61 additions & 0 deletions src/util/job-name-validation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { TranslationBundle } from '@jupyterlab/translation';
const jobNameRegex = /^[a-zA-Z0-9._][a-zA-Z0-9._ -]{0,62}$/;
const invalidFirstCharRegex = /^[^a-zA-Z0-9._]/;
const invalidCharRegex = /[^a-zA-Z0-9._ -]/g;
const validIntegerRregex = /^[+]?[1-9]\d*$/;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rregex is misspelled.

I don't see a reason why zero-prefixed integers like 035 should be rejected, since they'll resolve to valid numeric values anyway. The + is redundant in this case. This could be simplified to /^\d+$/.

const maxLength = 63;

export function NameIsValid(name: string): boolean {
Expand Down Expand Up @@ -64,3 +65,63 @@ export function NameError(name: string, trans: TranslationBundle): string {
'Name must contain only letters, numbers, spaces, periods, hyphens, and underscores'
);
}

export function MaxRetryAttemptsError(maxRetryAttempts: string, trans: TranslationBundle): string {
// Check for blank
if (maxRetryAttempts === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an optional parameter, since existing users editing job definitions will not have these values set.

return trans.__('You must specify max retry attempts');
}

if (!validIntegerRregex.test(maxRetryAttempts)) {
return trans.__(
'Max retry attempts must be an integer'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Max retry attempts must be an integer'
'Must be an integer between %1 and %2', min, max

Explain the acceptable range in the error

);
}
const integerValue = +maxRetryAttempts
// Check for length.
if (integerValue < 1 || integerValue > 30) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use symbolic constants to indicate the minimum and maximum acceptable values.

Why isn't 0 an acceptable value? It would fit if I wanted a job to be tried only once, and never retried if it fails.

return trans.__('Max retry attempts must be between 1 and 30');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return trans.__('Max retry attempts must be between 1 and 30');
'Must be an integer between %1 and %2', min, max

Explain the acceptable range in the error

}

return '';
}

export function MaxRunTimeError(maxRunTime: string, trans: TranslationBundle): string {
// Check for blank
if (maxRunTime === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an optional parameter, since existing users editing job definitions will not have these values set.

return trans.__('You must specify max run time');
}

if (!validIntegerRregex.test(maxRunTime)) {
return trans.__(
'Max run time must be an integer'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Max run time must be an integer'
'Must be an integer, %1 or greater', minRunTime

);
}
const integerValue = +maxRunTime
// Check for length.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix or delete this comment

if (integerValue < 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a symbolic constant

return trans.__('Max run time must be greater than 1');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return trans.__('Max run time must be greater than 1');
'Must be an integer, %1 or greater', minRunTime

}

return '';
}

export function MaxWaitTimeError(maxWaitTime: string, trans: TranslationBundle): string {
// Check for blank
if (maxWaitTime === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an optional parameter, since existing users editing job definitions will not have these values set.

return trans.__('You must specify max run time');
}

if (!validIntegerRregex.test(maxWaitTime)) {
return trans.__(
'Max wait time must be an integer'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Max wait time must be an integer'
'Must be an integer, %1 or greater', minWaitTime

);
}
const integerValue = +maxWaitTime
// Check for length.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix or delete this comment

if (integerValue < 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a symbolic constant

return trans.__('Max wait time must be greater than 1');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return trans.__('Max wait time must be greater than 1');
return trans.__('Must be an integer, %1 or greater', minWaitTime)

}

return '';
}