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 all 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
9 changes: 9 additions & 0 deletions src/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,9 @@ export namespace Scheduler {
compute_type?: string;
schedule?: string;
timezone?: string;
maxRetryAttempts: number;
maxRunTime: number;
maxWaitTime: number;
}

export interface IUpdateJobDefinition {
Expand All @@ -389,6 +392,9 @@ export namespace Scheduler {
create_time: number;
update_time: number;
active: boolean;
maxRetryAttempts: number;
maxRunTime: number;
maxWaitTime: number;
}

export interface IEmailNotifications {
Expand All @@ -415,6 +421,9 @@ export namespace Scheduler {
output_filename_template?: string;
output_formats?: string[];
compute_type?: string;
maxRetryAttempts: number;
maxRunTime: number;
maxWaitTime: number;
}

export interface ICreateJobFromDefinition {
Expand Down
100 changes: 92 additions & 8 deletions 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 = parseInt(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 @@ -288,11 +308,11 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
if (jobParameters[name] !== undefined) {
console.error(
'Parameter ' +
name +
' already set to ' +
jobParameters[name] +
' and is about to be set again to ' +
value
name +
' already set to ' +
jobParameters[name] +
' and is about to be set again to ' +
value
);
} else {
jobParameters[name] = value;
Expand All @@ -319,7 +339,10 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
compute_type: props.model.computeType,
idempotency_token: props.model.idempotencyToken,
tags: props.model.tags,
runtime_environment_parameters: props.model.runtimeEnvironmentParameters
runtime_environment_parameters: props.model.runtimeEnvironmentParameters,
maxRetryAttempts: props.model.maxRetryAttempts,
maxRunTime: props.model.maxRunTime,
maxWaitTime: props.model.maxWaitTime,
};

if (props.model.parameters !== undefined) {
Expand Down Expand Up @@ -364,7 +387,10 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
tags: props.model.tags,
runtime_environment_parameters: props.model.runtimeEnvironmentParameters,
schedule: props.model.schedule,
timezone: props.model.timezone
timezone: props.model.timezone,
maxRetryAttempts: props.model.maxRetryAttempts,
maxRunTime: props.model.maxRunTime,
maxWaitTime: props.model.maxWaitTime,
};

if (props.model.parameters !== undefined) {
Expand Down Expand Up @@ -495,6 +521,7 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
environmentList={environmentList}
value={props.model.environment}
/>

<OutputFormatPicker
label={trans.__('Output formats')}
name="outputFormat"
Expand Down Expand Up @@ -549,6 +576,63 @@ export function CreateJob(props: ICreateJobProps): JSX.Element {
</FormLabel>
</AccordionSummary>
<AccordionDetails id={`${formPrefix}create-panel-content`}>
<div>
<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"
/>
</div>
<div>
<TextField
label={trans.__('Max Run Time (In Seconds)')}
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"
/>
</div>
<div>
<TextField
label={trans.__('Max Wait Time (In 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"
/>
</div>
<props.advancedOptions
jobsView={JobsView.CreateForm}
model={props.model}
Expand Down
20 changes: 16 additions & 4 deletions src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,17 @@ export type ModelWithScheduleFields = {
* Value of input field for past the hour.
*/
scheduleMinute: string;

maxRetryAttempts: number;

maxRunTime: number;

maxWaitTime: number;
};

export interface ICreateJobModel
extends ModelWithScheduleFields,
PartialJSONObject {
PartialJSONObject {
/**
* Key of the CreateJob component. When changed, this forces a re-mount.
*/
Expand All @@ -99,6 +105,9 @@ export interface ICreateJobModel
tags?: string[];
// Is the create button disabled due to a submission in progress?
createInProgress?: boolean;
maxRetryAttempts: number;
maxRunTime: number;
maxWaitTime: number;
}

export const defaultScheduleFields: ModelWithScheduleFields = {
Expand All @@ -108,7 +117,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 All @@ -125,7 +137,7 @@ export function emptyCreateJobModel(): ICreateJobModel {

export interface IUpdateJobDefinitionModel
extends ModelWithScheduleFields,
PartialJSONObject {
PartialJSONObject {
definitionId: string;
name: string;
environment: string;
Expand Down Expand Up @@ -267,7 +279,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
62 changes: 62 additions & 0 deletions src/util/job-name-validation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,65 @@ 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');
}

const integerValue = parseInt(maxRetryAttempts)
if (isNaN(integerValue)) {
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

);
}

// 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');
}

const integerValue = parseInt(maxRunTime)
if (isNaN(integerValue)) {
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

);
}

// 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');
}
const integerValue = parseInt(maxWaitTime)
if (isNaN(integerValue)) {
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

);
}

// 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 '';
}