Refactor NNI manager globals (step 1) - argparse (#4510)

nni/experiment/experiment.py

@@ -91,7 +91,7 @@ class Experiment:
         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 @@ class Experiment:
             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 @@ class Experiment:
     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 @@ class Experiment:
     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
 

nni/experiment/launcher.py

@@ -27,23 +27,27 @@ _logger = logging.getLogger('nni.experiment')
 
 @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 @@ class NniManagerArgs:
         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 @@ class NniManagerArgs:
 
 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':

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:

ts/nni_manager/common/globals/arguments.ts

+// 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/<experiment-id>",
+ *  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;

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<void> {
+async function initContainer(): Promise<void> {
     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 <port> --mode \
-    <local/remote/pai/kubeflow/frameworkcontroller/aml/adl/hybrid/dlc> --start_mode <new/resume> --experiment_id <id> --foreground <true/false>');
-}
-
-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) {

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",

ts/nni_manager/test/common/globals/arguments.test.ts

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

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"