diff --git a/nni/experiment/experiment.py b/nni/experiment/experiment.py index 57ff2356bf..e3ddffe2a8 100644 --- a/nni/experiment/experiment.py +++ b/nni/experiment/experiment.py @@ -91,7 +91,7 @@ def __init__(self, config=None, training_service=None): self.id: str = management.generate_experiment_id() self.port: Optional[int] = None self._proc: Optional[Popen] = None - self.mode = 'new' + self.action = 'create' self.url_prefix: Optional[str] = None args = [config, training_service] # deal with overloading @@ -127,7 +127,7 @@ def start(self, port: int = 8080, debug: bool = False, run_mode: RunMode = RunMo log_dir = Path.home() / f'nni-experiments/{self.id}/log' nni.runtime.log.start_experiment_log(self.id, log_dir, debug) - self._proc = launcher.start_experiment(self.mode, self.id, config, port, debug, run_mode, self.url_prefix) + self._proc = launcher.start_experiment(self.action, self.id, config, port, debug, run_mode, self.url_prefix) assert self._proc is not None self.port = port # port will be None if start up failed @@ -261,7 +261,7 @@ def view(experiment_id: str, port: int = 8080, non_blocking: bool = False): def _resume(exp_id, exp_dir=None): exp = Experiment() exp.id = exp_id - exp.mode = 'resume' + exp.action = 'resume' exp.config = launcher.get_stopped_experiment_config(exp_id, exp_dir) return exp @@ -269,7 +269,7 @@ def _resume(exp_id, exp_dir=None): def _view(exp_id, exp_dir=None): exp = Experiment() exp.id = exp_id - exp.mode = 'view' + exp.action = 'view' exp.config = launcher.get_stopped_experiment_config(exp_id, exp_dir) return exp diff --git a/nni/experiment/launcher.py b/nni/experiment/launcher.py index 8eff304cca..f7abbf2530 100644 --- a/nni/experiment/launcher.py +++ b/nni/experiment/launcher.py @@ -27,23 +27,27 @@ @dataclass(init=False) class NniManagerArgs: + # argv sent to "ts/nni_manager/main.js" + port: int experiment_id: int - start_mode: str # new or resume - mode: str # training service platform - log_dir: str + action: str # 'new', 'resume', 'view' + mode: str # training service platform, to be removed + experiments_directory: str # renamed "config.nni_experiments_directory", must be absolute log_level: str - readonly: bool = False foreground: bool = False - url_prefix: Optional[str] = None + url_prefix: Optional[str] = None # leading and trailing "/" must be stripped dispatcher_pipe: Optional[str] = None def __init__(self, action, exp_id, config, port, debug, foreground, url_prefix): self.port = port self.experiment_id = exp_id + self.action = action self.foreground = foreground self.url_prefix = url_prefix - self.log_dir = config.experiment_working_directory + # config field name "experiment_working_directory" is a mistake + # see "ts/nni_manager/common/globals/arguments.ts" for details + self.experiments_directory = config.experiment_working_directory if isinstance(config.training_service, list): self.mode = 'hybrid' @@ -54,20 +58,14 @@ def __init__(self, action, exp_id, config, port, debug, foreground, url_prefix): if debug and self.log_level not in ['debug', 'trace']: self.log_level = 'debug' - if action == 'resume': - self.start_mode = 'resume' - elif action == 'view': - self.start_mode = 'resume' - self.readonly = True - else: - self.start_mode = 'new' - def to_command_line_args(self): + # reformat fields to meet yargs library's format + # see "ts/nni_manager/common/globals/arguments.ts" for details ret = [] for field in fields(self): value = getattr(self, field.name) if value is not None: - ret.append('--' + field.name) + ret.append('--' + field.name.replace('_', '-')) if isinstance(value, bool): ret.append(str(value).lower()) else: @@ -76,6 +74,8 @@ def to_command_line_args(self): def start_experiment(action, exp_id, config, port, debug, run_mode, url_prefix): foreground = run_mode.value == 'foreground' + if url_prefix is not None: + url_prefix = url_prefix.strip('/') nni_manager_args = NniManagerArgs(action, exp_id, config, port, debug, foreground, url_prefix) _ensure_port_idle(port) @@ -135,7 +135,7 @@ def _start_rest_server(nni_manager_args, run_mode) -> Tuple[int, Popen]: cmd += nni_manager_args.to_command_line_args() if run_mode.value == 'detach': - log = Path(nni_manager_args.log_dir, nni_manager_args.experiment_id, 'log') + log = Path(nni_manager_args.experiments_directory, nni_manager_args.experiment_id, 'log') out = (log / 'nnictl_stdout.log').open('a') err = (log / 'nnictl_stderr.log').open('a') header = f'Experiment {nni_manager_args.experiment_id} start: {datetime.now()}' @@ -201,7 +201,7 @@ def _ensure_port_idle(port: int, message: Optional[str] = None) -> None: def _start_rest_server_retiarii(config: ExperimentConfig, port: int, debug: bool, experiment_id: str, - pipe_path: str = None, mode: str = 'new') -> Tuple[int, Popen]: + pipe_path: str, mode: str = 'create') -> Tuple[int, Popen]: if isinstance(config.training_service, list): ts = 'hybrid' else: @@ -213,24 +213,20 @@ def _start_rest_server_retiarii(config: ExperimentConfig, port: int, debug: bool 'port': port, 'mode': ts, 'experiment_id': experiment_id, - 'start_mode': mode, - 'log_dir': config.experiment_working_directory, + 'action': mode, + 'experiments_directory': config.experiment_working_directory, 'log_level': 'debug' if debug else 'info' } if pipe_path is not None: args['dispatcher_pipe'] = pipe_path - if mode == 'view': - args['start_mode'] = 'resume' - args['readonly'] = 'true' - import nni_node node_dir = Path(nni_node.__path__[0]) node = str(node_dir / ('node.exe' if sys.platform == 'win32' else 'node')) main_js = str(node_dir / 'main.js') cmd = [node, '--max-old-space-size=4096', main_js] for arg_key, arg_value in args.items(): - cmd.append('--' + arg_key) + cmd.append('--' + arg_key.replace('_', '-')) cmd.append(str(arg_value)) if sys.platform == 'win32': diff --git a/nni/tools/nnictl/legacy_launcher.py b/nni/tools/nnictl/legacy_launcher.py index 8641d11d5d..f72614a99c 100644 --- a/nni/tools/nnictl/legacy_launcher.py +++ b/nni/tools/nnictl/legacy_launcher.py @@ -70,21 +70,17 @@ def start_rest_server(port, platform, mode, experiment_id, foreground=False, log node_command = os.path.join(entry_dir, 'node') cmds = [node_command, '--max-old-space-size=4096', entry_file, '--port', str(port), '--mode', platform, \ '--experiment_id', experiment_id] - if mode == 'view': - cmds += ['--start_mode', 'resume'] - cmds += ['--readonly', 'true'] - else: - cmds += ['--start_mode', mode] + cmds += ['--action', mode] if log_dir is not None: - cmds += ['--log_dir', log_dir] + cmds += ['--experiments-directory', log_dir] if log_level is not None: - cmds += ['--log_level', log_level] + cmds += ['--log-level', log_level] if foreground: cmds += ['--foreground', 'true'] if url_prefix: _validate_prefix_path(url_prefix) set_prefix_url(url_prefix) - cmds += ['--url_prefix', url_prefix] + cmds += ['--url-prefix', url_prefix.strip('/')] stdout_full_path, stderr_full_path = get_log_path(experiment_id) with open(stdout_full_path, 'a+') as stdout_file, open(stderr_full_path, 'a+') as stderr_file: @@ -520,9 +516,9 @@ def create_experiment(args): try: if schema == 1: - launch_experiment(args, config_v1, 'new', experiment_id, 1) + launch_experiment(args, config_v1, 'create', experiment_id, 1) else: - launch_experiment(args, config_v2, 'new', experiment_id, 2) + launch_experiment(args, config_v2, 'create', experiment_id, 2) except Exception as exception: restServerPid = Experiments().get_all_experiments().get(experiment_id, {}).get('pid') if restServerPid: diff --git a/ts/nni_manager/common/globals/arguments.ts b/ts/nni_manager/common/globals/arguments.ts new file mode 100644 index 0000000000..f3973b59a6 --- /dev/null +++ b/ts/nni_manager/common/globals/arguments.ts @@ -0,0 +1,96 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/** + * Parse NNI manager's command line arguments. + **/ + +import assert from 'assert/strict'; + +import yargs from 'yargs/yargs'; + +/** + * Command line arguments provided by "nni/experiment/launcher.py". + * + * Hyphen-separated words are automatically converted to camelCases by yargs lib, but snake_cases are not. + * So it supports "--log-level" but does not support "--log_level". + * + * Unfortunately I misunderstood "experiment_working_directory" config field when deciding the name. + * It defaults to "~/nni-experiments" rather than "~/nni-experiments/", + * and further more the working directory is "site-packages/nni_node", not either. + * For compatibility concern we cannot change the public API, so there is an inconsistency here. + **/ +export interface NniManagerArgs { + readonly port: number; + readonly experimentId: string; + readonly action: 'create' | 'resume' | 'view'; + readonly experimentsDirectory: string; // renamed "config.experiment_working_directory", must be absolute + readonly logLevel: 'critical' | 'error' | 'warning' | 'info' | 'debug'; + readonly foreground: boolean; + readonly urlPrefix: string; // leading and trailing "/" must be stripped + + // these are planned to be removed + readonly mode: string; + readonly dispatcherPipe: string | undefined; +} + +export function parseArgs(rawArgs: string[]): NniManagerArgs { + const parser = yargs(rawArgs).options(yargsOptions).strict().fail((_msg, err, _yargs) => { throw err; }); + const parsedArgs: NniManagerArgs = parser.parseSync(); + + // strip yargs leftovers + const argsAsAny: any = {}; + for (const key in yargsOptions) { + argsAsAny[key] = (parsedArgs as any)[key]; + assert(!Number.isNaN(argsAsAny[key]), `Command line arg --${key} is not a number`); + } + if (argsAsAny.dispatcherPipe === '') { + argsAsAny.dispatcherPipe = undefined; + } + const args: NniManagerArgs = argsAsAny; + + const prefixErrMsg = `Command line arg --url-prefix "${args.urlPrefix}" is not stripped`; + assert(!args.urlPrefix.startsWith('/') && !args.urlPrefix.endsWith('/'), prefixErrMsg); + + return args; +} + +const yargsOptions = { + port: { + demandOption: true, + type: 'number' + }, + experimentId: { + demandOption: true, + type: 'string' + }, + action: { + choices: [ 'create', 'resume', 'view' ] as const, + demandOption: true + }, + experimentsDirectory: { + demandOption: true, + type: 'string' + }, + logLevel: { + choices: [ 'critical', 'error', 'warning', 'info', 'debug' ] as const, + demandOption: true + }, + foreground: { + default: false, + type: 'boolean' + }, + urlPrefix: { + default: '', + type: 'string' + }, + + mode: { + default: '', + type: 'string' + }, + dispatcherPipe: { + default: '', + type: 'string' + } +} as const; diff --git a/ts/nni_manager/main.ts b/ts/nni_manager/main.ts index 5449e5b13c..c3a0574b8b 100644 --- a/ts/nni_manager/main.ts +++ b/ts/nni_manager/main.ts @@ -20,15 +20,11 @@ import { SqlDB } from './core/sqlDatabase'; import { NNIExperimentsManager } from './core/nniExperimentsManager'; import { NNITensorboardManager } from './core/nniTensorboardManager'; import { RestServer } from './rest_server'; +import { parseArgs } from 'common/globals/arguments'; -function initStartupInfo( - startExpMode: string, experimentId: string, basePort: number, platform: string, - logDirectory: string, experimentLogLevel: string, readonly: boolean, dispatcherPipe: string, urlprefix: string): void { - const createNew: boolean = (startExpMode === ExperimentStartUpMode.NEW); - setExperimentStartupInfo(createNew, experimentId, basePort, platform, logDirectory, experimentLogLevel, readonly, dispatcherPipe, urlprefix); -} +const args = parseArgs(process.argv.slice(2)); -async function initContainer(foreground: boolean, _platformMode: string, logFileName?: string): Promise { +async function initContainer(): Promise { Container.bind(Manager) .to(NNIManager) .scope(Scope.Singleton); @@ -45,84 +41,32 @@ async function initContainer(foreground: boolean, _platformMode: string, logFile .to(NNITensorboardManager) .scope(Scope.Singleton); const DEFAULT_LOGFILE: string = path.join(getLogDir(), 'nnimanager.log'); - if (!foreground) { - if (logFileName === undefined) { - startLogging(DEFAULT_LOGFILE); - } else { - startLogging(logFileName); - } + if (!args.foreground) { + startLogging(DEFAULT_LOGFILE); } // eslint-disable-next-line @typescript-eslint/no-use-before-define - setLogLevel(logLevel); + setLogLevel(args.logLevel); const ds: DataStore = component.get(DataStore); await ds.init(); } -function usage(): void { - console.info('usage: node main.js --port --mode \ - --start_mode --experiment_id --foreground '); -} - -const strPort: string = parseArg(['--port', '-p']); -if (!strPort || strPort.length === 0) { - usage(); - process.exit(1); -} - -const foregroundArg: string = parseArg(['--foreground', '-f']); -if (foregroundArg && !['true', 'false'].includes(foregroundArg.toLowerCase())) { - console.log(`FATAL: foreground property should only be true or false`); - usage(); - process.exit(1); -} -const foreground: boolean = (foregroundArg && foregroundArg.toLowerCase() === 'true') ? true : false; - -const port: number = parseInt(strPort, 10); - -const mode: string = parseArg(['--mode', '-m']); - -const startMode: string = parseArg(['--start_mode', '-s']); -if (![ExperimentStartUpMode.NEW, ExperimentStartUpMode.RESUME].includes(startMode)) { - console.log(`FATAL: unknown start_mode: ${startMode}`); - usage(); - process.exit(1); -} - -const experimentId: string = parseArg(['--experiment_id', '-id']); -if (experimentId.trim().length < 1) { - console.log(`FATAL: cannot resume the experiment, invalid experiment_id: ${experimentId}`); - usage(); - process.exit(1); -} - -const logDir: string = parseArg(['--log_dir', '-ld']); -if (logDir.length > 0) { - if (!fs.existsSync(logDir)) { - console.log(`FATAL: log_dir ${logDir} does not exist`); - } -} - -const logLevel: string = parseArg(['--log_level', '-ll']); - -const readonlyArg: string = parseArg(['--readonly', '-r']); -if (readonlyArg && !['true', 'false'].includes(readonlyArg.toLowerCase())) { - console.log(`FATAL: readonly property should only be true or false`); - usage(); - process.exit(1); -} -const readonly = (readonlyArg && readonlyArg.toLowerCase() == 'true') ? true : false; - -const dispatcherPipe: string = parseArg(['--dispatcher_pipe']); - -const urlPrefix: string = parseArg(['--url_prefix']); - -initStartupInfo(startMode, experimentId, port, mode, logDir, logLevel, readonly, dispatcherPipe, urlPrefix); +setExperimentStartupInfo( + args.action === 'create', + args.experimentId, + args.port, + args.mode, + args.experimentsDirectory, + args.logLevel, + args.action === 'view', + args.dispatcherPipe ?? '', + args.urlPrefix +); mkDirP(getLogDir()) .then(async () => { try { - await initContainer(foreground, mode); + await initContainer(); const restServer: RestServer = component.get(RestServer); await restServer.start(); } catch (err) { diff --git a/ts/nni_manager/package.json b/ts/nni_manager/package.json index 205ba066a5..f0aa3f5f75 100644 --- a/ts/nni_manager/package.json +++ b/ts/nni_manager/package.json @@ -33,7 +33,8 @@ "ts-deferred": "^1.0.4", "typescript-ioc": "^1.2.6", "typescript-string-operations": "^1.4.1", - "ws": "^7.4.6" + "ws": "^7.4.6", + "yargs": "^17.3.1" }, "devDependencies": { "@types/chai": "^4.2.18", @@ -55,6 +56,7 @@ "@types/tar": "^4.0.4", "@types/tmp": "^0.2.0", "@types/ws": "^7.4.4", + "@types/yargs": "^17.0.8", "@typescript-eslint/eslint-plugin": "^2.10.0", "@typescript-eslint/parser": "^4.26.0", "chai": "^4.3.4", diff --git a/ts/nni_manager/test/common/globals/arguments.test.ts b/ts/nni_manager/test/common/globals/arguments.test.ts new file mode 100644 index 0000000000..e7ed7d9ed8 --- /dev/null +++ b/ts/nni_manager/test/common/globals/arguments.test.ts @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import assert from 'assert/strict'; + +import { parseArgs } from 'common/globals/arguments'; + +const command = '--port 80 --experiment-id ID --action resume --experiments-directory DIR --log-level error'; +const expected = { + port: 80, + experimentId: 'ID', + action: 'resume', + experimentsDirectory: 'DIR', + logLevel: 'error', + foreground: false, + urlPrefix: '', + + mode: '', + dispatcherPipe: undefined, +}; + +function testGoodShort(): void { + const args = parseArgs(command.split(' ')); + assert.deepEqual(args, expected); +} + +function testGoodLong(): void { + const cmd = command + ' --url-prefix URL/prefix --foreground true'; + const args = parseArgs(cmd.split(' ')); + const expectedLong = Object.assign({}, expected); + expectedLong.urlPrefix = 'URL/prefix'; + expectedLong.foreground = true; + assert.deepEqual(args, expectedLong); +} + +function testBadKey(): void { + const cmd = command + ' --bad 1'; + assert.throws(() => parseArgs(cmd.split(' '))); +} + +function testBadPos(): void { + const cmd = command.replace('--port', 'port'); + assert.throws(() => parseArgs(cmd.split(' '))); +} + +function testBadNum(): void { + const cmd = command.replace('80', '8o'); + assert.throws(() => parseArgs(cmd.split(' '))); +} + +function testBadBool(): void { + const cmd = command + ' --foreground 1'; + assert.throws(() => parseArgs(cmd.split(' '))); +} + +function testBadChoice(): void { + const cmd = command.replace('resume', 'new'); + assert.throws(() => parseArgs(cmd.split(' '))); +} + +describe('## globals.arguments ##', () => { + it('good short', () => testGoodShort()); + it('good long', () => testGoodLong()); + it('bad key arg', () => testBadKey()); + it('bad positional arg', () => testBadPos()); + it('bad number', () => testBadNum()); + it('bad boolean', () => testBadBool()); + it('bad choice', () => testBadChoice()); +}); diff --git a/ts/nni_manager/yarn.lock b/ts/nni_manager/yarn.lock index 2ab0712694..35093aa2a1 100644 --- a/ts/nni_manager/yarn.lock +++ b/ts/nni_manager/yarn.lock @@ -820,6 +820,18 @@ dependencies: "@types/node" "*" +"@types/yargs-parser@*": + version "20.2.1" + resolved "https://registry.yarnpkg.com/@types/yargs-parser/-/yargs-parser-20.2.1.tgz#3b9ce2489919d9e4fea439b76916abc34b2df129" + integrity sha512-7tFImggNeNBVMsn0vLrpn1H1uPrUBdnARPTpZoitY37ZrdJREzf7I16tMrlK3hen349gr1NYh8CmZQa7CTG6Aw== + +"@types/yargs@^17.0.8": + version "17.0.8" + resolved "https://registry.yarnpkg.com/@types/yargs/-/yargs-17.0.8.tgz#d23a3476fd3da8a0ea44b5494ca7fa677b9dad4c" + integrity sha512-wDeUwiUmem9FzsyysEwRukaEdDNcwbROvQ9QGRKaLI6t+IltNzbn4/i4asmB10auvZGQCzSQ6t0GSczEThlUXw== + dependencies: + "@types/yargs-parser" "*" + "@typescript-eslint/eslint-plugin@^2.10.0": version "2.34.0" resolved "https://registry.yarnpkg.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-2.34.0.tgz#6f8ce8a46c7dea4a6f1d171d2bb8fbae6dac2be9" @@ -5752,7 +5764,7 @@ yallist@^4.0.0: resolved "https://registry.yarnpkg.com/yallist/-/yallist-4.0.0.tgz#9bb92790d9c0effec63be73519e11a35019a3a72" integrity sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A== -yargs-parser@20.2.4, yargs-parser@>=20.2.7, yargs-parser@^18.1.2, yargs-parser@^20.2.2: +yargs-parser@20.2.4, yargs-parser@>=20.2.7, yargs-parser@^18.1.2, yargs-parser@^20.2.2, yargs-parser@^21.0.0: version "20.2.7" resolved "https://registry.yarnpkg.com/yargs-parser/-/yargs-parser-20.2.7.tgz#61df85c113edfb5a7a4e36eb8aa60ef423cbc90a" integrity sha512-FiNkvbeHzB/syOjIUxFDCnhSfzAL8R5vs40MgLFBorXACCOAEaWu0gRZl14vG8MR9AOJIZbmkjhusqBYZ3HTHw== @@ -5797,6 +5809,19 @@ yargs@^15.0.2: y18n "^4.0.0" yargs-parser "^18.1.2" +yargs@^17.3.1: + version "17.3.1" + resolved "https://registry.yarnpkg.com/yargs/-/yargs-17.3.1.tgz#da56b28f32e2fd45aefb402ed9c26f42be4c07b9" + integrity sha512-WUANQeVgjLbNsEmGk20f+nlHgOqzRFpiGWVaBrYGYIGANIIu3lWjoyi0fNlFmJkvfhCZ6BXINe7/W2O2bV4iaA== + dependencies: + cliui "^7.0.2" + escalade "^3.1.1" + get-caller-file "^2.0.5" + require-directory "^2.1.1" + string-width "^4.2.3" + y18n "^5.0.5" + yargs-parser "^21.0.0" + yn@3.1.1: version "3.1.1" resolved "https://registry.yarnpkg.com/yn/-/yn-3.1.1.tgz#1e87401a09d767c1d5eab26a6e4c185182d2eb50"