Skip to content
This repository has been archived by the owner on Nov 24, 2021. It is now read-only.

WIP: Improve status code handling. #28

Draft
wants to merge 1 commit into
base: master
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
4 changes: 2 additions & 2 deletions packages/model-runner/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ release:
run-local: build
rm -rf input log output export.zip
mkdir -p log input output
cp test/test-job.json input/inputFile.json
./bin/run-model test/test-job.json
jq .configuration test/test-job-mrc-ide-covidsim.json > input/inputFile.json
./bin/run-model test/test-job-mrc-ide-covidsim.json

.PHONY: run-remote
run-remote:
Expand Down
11 changes: 3 additions & 8 deletions packages/model-runner/src/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export async function pullImage(
export async function runContainer(
client: Dockerode,
image: string
): Promise<void> {
): Promise<number> {
// Setup the log file.
const logFile = path.join(LOG_DIR, 'runner.log')
const logWriteStream = fs.createWriteStream(logFile, {
Expand All @@ -82,18 +82,13 @@ export async function runContainer(
},
{},
(err, result) => {
// TODO: Consider using dockerClient.modem.followProgress(stream, onFinished, onProgress)
// or some equivalent for result
if (err) return reject(err)
logger.info(result)
// const output = result[0]
// const container = result[1]
logger.info('Model exited with status code: %d', result.StatusCode)
// resolve(container.remove())
// result[1].remove()
if (result.Error) {
reject(result.Error)
} else {
resolve()
resolve(result.StatusCode)
}
}
)
Expand Down
46 changes: 31 additions & 15 deletions packages/model-runner/src/main.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,31 @@
import * as pino from 'pino'
import * as path from 'path'
import * as mkdirp from 'mkdirp'
import { RunStatus, RequestInput } from '@covid-modeling/api'
import { RequestInput, RunStatus } from '@covid-modeling/api'
import { BlobStorage } from './blobstore'
import { notifyUI } from './notify-ui'
import { logger } from './logger'
import {
LOG_DIR,
OUTPUT_DIR,
RUNNER_SHARED_SECRET,
AZURE_STORAGE_ACCOUNT,
AZURE_STORAGE_CONTAINER,
INPUT_DIR,
HOST_WORK_DIR,
INPUT_DIR,
LOG_DIR,
OUTPUT_DIR,
RUNNER_SHARED_SECRET,
} from './config'
import * as Dockerode from 'dockerode'
import * as fs from 'fs'
import * as crypto from 'crypto'
import { createExportZip } from './export'
import * as docker from './docker'
import { enforceRunnerInputSchema, enforceOutputSchema } from './schema'
import { enforceOutputSchema, enforceRunnerInputSchema } from './schema'
import { convertModelStatusCode, StatusCodes } from './status-codes'

let inputID: string | number | null = null
let callbackURL: string | null = null
let modelSlug: string | null = null
let statusCode: number = StatusCodes.Ok

const handleRejection: NodeJS.UnhandledRejectionListener = err => {
const finalLogger = pino.final(logger)
Expand All @@ -35,10 +37,10 @@ const handleRejection: NodeJS.UnhandledRejectionListener = err => {
resultsLocation: '',
exportLocation: '',
}).finally(() => {
process.exit(1)
process.exit(StatusCodes.UnknownError)
})
} else {
process.exit(1)
process.exit(StatusCodes.UnknownError)
}
}

Expand Down Expand Up @@ -102,16 +104,29 @@ async function main() {
logger.info(JSON.stringify(input.configuration))

logger.info('Running container: %s', dockerImage)
await docker
.runContainer(dockerClient, dockerImage)
.then(data => {
// TODO: we seem to have lost the reference to the container
// logger.info('container %d removed', data)

statusCode = convertModelStatusCode(
await docker.runContainer(dockerClient, dockerImage).catch(err => {
logger.error(err)
return StatusCodes.DockerError
})
.catch(err => logger.error(err))
)

logger.info('Finished model run')

if (statusCode < 0) {
if (input.callbackURL) {
await notifyUI(input.callbackURL, RUNNER_SHARED_SECRET, input.id, {
modelSlug,
status: RunStatus.Failed,
resultsLocation: '',
exportLocation: '',
// TODO: Send back the status code or some other indicator of the type of failure.
})
}
process.exit(statusCode)
}

logger.info('Creating export zip.')
const exportZipFile = 'export.zip'
await createExportZip([INPUT_DIR, OUTPUT_DIR], exportZipFile)
Expand Down Expand Up @@ -162,6 +177,7 @@ async function main() {
} catch (err) {
handleRejection(err, Promise.resolve())
}
process.exit(statusCode)
}

function uniqueId(runInput: RequestInput, imageId: string): string {
Expand All @@ -183,7 +199,7 @@ Usage:
`.trim()
)

process.exit(1)
process.exit(StatusCodes.InvalidArguments)
}

main()
10 changes: 10 additions & 0 deletions packages/model-runner/src/status-codes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export enum StatusCodes {
Ok = 0,
InvalidArguments = 1,
UnknownError = 2,
DockerError = 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an unsupported region code, too?

}

export function convertModelStatusCode(code: number): number {
return -code
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is done to distinguish between "the model returned a non-zero status code" and "the runner returned a non-zero status code"? We aren't running on Windows on Actions so maybe not crucial, but negative exit codes aren't always handled nicely.
Perhaps for now it's enough to just have a single non-zero status code for "the model returned a non-zero code", since we log the model exit code anyway.

}