Refactor NNI manager globals (step 3) - globals module (#4703)

ts/nni_manager/common/experimentStartupInfo.ts

 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import assert from 'assert/strict';
-import path from 'path';
-
-import type { NniManagerArgs } from 'common/globals/arguments';
-
-let singleton: ExperimentStartupInfo | null = null;
+import globals from 'common/globals';
 
 export class ExperimentStartupInfo {
-
-    public experimentId: string;
-    public newExperiment: boolean;
-    public basePort: number;
-    public logDir: string = '';
-    public logLevel: string;
-    public readonly: boolean;
-    public dispatcherPipe: string | null;
-    public platform: string;
-    public urlprefix: string;
-
-    constructor(args: NniManagerArgs) {
-        this.experimentId = args.experimentId;
-        this.newExperiment = (args.action === 'create');
-        this.basePort = args.port;
-        this.logDir = path.join(args.experimentsDirectory, args.experimentId);  // TODO: handle in globals
-        this.logLevel = args.logLevel;
-        this.readonly = (args.action === 'view');
-        this.dispatcherPipe = args.dispatcherPipe ?? null;
-        this.platform = args.mode as string;
-        this.urlprefix = args.urlPrefix;
-    }
+    public experimentId: string = globals.args.experimentId;
+    public newExperiment: boolean = (globals.args.action === 'create');
+    public basePort: number = globals.args.port;
+    public logDir: string = globals.paths.experimentRoot;
+    public logLevel: string = globals.args.logLevel;
+    public readonly: boolean = (globals.args.action === 'view');
+    public dispatcherPipe: string | null = globals.args.dispatcherPipe ?? null;
+    public platform: string = globals.args.mode as string;
+    public urlprefix: string = globals.args.urlPrefix;
 
     public static getInstance(): ExperimentStartupInfo {
-        assert.notEqual(singleton, null);
-        return singleton!;
+        return new ExperimentStartupInfo();
     }
 }
 
 export function getExperimentStartupInfo(): ExperimentStartupInfo {
-    return ExperimentStartupInfo.getInstance();
-}
-
-export function setExperimentStartupInfo(args: NniManagerArgs): void {
-    singleton = new ExperimentStartupInfo(args);
+    return new ExperimentStartupInfo();
 }
 
 export function getExperimentId(): string {
-    return getExperimentStartupInfo().experimentId;
+    return globals.args.experimentId;
 }
 
 export function getBasePort(): number {
-    return getExperimentStartupInfo().basePort;
+    return globals.args.port;
 }
 
 export function isNewExperiment(): boolean {
-    return getExperimentStartupInfo().newExperiment;
+    return globals.args.action === 'create';
 }
 
 export function getPlatform(): string {
-    return getExperimentStartupInfo().platform;
+    return globals.args.mode as string;
 }
 
 export function isReadonly(): boolean {
-    return getExperimentStartupInfo().readonly;
+    return globals.args.action === 'view';
 }
 
 export function getDispatcherPipe(): string | null {
-    return getExperimentStartupInfo().dispatcherPipe;
+    return globals.args.dispatcherPipe ?? null;
 }

ts/nni_manager/common/globals/index.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ *  Collection of global objects.
+ *
+ *  Although global is anti-pattern in OOP, there are two scenarios NNI uses globals.
+ *
+ *   1. Some constant configs (like command line args) are accessed here and there with util functions.
+ *      It is possible to pass parameters instead, but not worthy the refactor.
+ *
+ *   2. Some singletons (like root logger) are indeed global.
+ *      The singletons need to be registered in `global` to support 3rd-party training services,
+ *      because they are compiled outside NNI manager and therefore module scope singletons will not work.
+ **/
+
+import assert from 'assert/strict';
+
+import { NniManagerArgs, parseArgs } from './arguments';
+import { NniPaths, createPaths } from './paths';
+
+export { NniManagerArgs, NniPaths };
+
+/**
+ *  Collection of global objects.
+ *
+ *  It can be obtained with `import globals from 'common/globals'` or `global.nni`.
+ *  The former is preferred because it exposes less underlying implementations.
+ **/
+export interface NniGlobals {
+    readonly args: NniManagerArgs;
+    readonly paths: NniPaths;
+}
+
+// give type hint to `global.nni` (copied from SO, dunno how it works)
+declare global {
+    var nni: NniGlobals;  // eslint-disable-line
+}
+
+// prepare the namespace object and export it
+if (global.nni === undefined) {
+    global.nni = {} as NniGlobals;
+}
+const globals: NniGlobals = global.nni;
+export default globals;
+
+/**
+ *  Initialize globals.
+ *  Must and must only be invoked once in "main.ts".
+ **/
+export function initGlobals(): void {
+    assert.deepEqual(global.nni, {});
+
+    const args = parseArgs(process.argv.slice(2));
+    const paths = createPaths(args);
+
+    const globals: NniGlobals = { args, paths };
+    Object.assign(global.nni, globals);
+}

ts/nni_manager/common/globals/paths.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ *  Manage experiment paths.
+ *
+ *  Ideally all path constants should be put here so other modules (especially training services)
+ *  do not need to know file hierarchy of nni-experiments folder, which is an implicit undocumented protocol.
+ **/
+
+import assert from 'assert/strict';
+import fs from 'fs';
+import path from 'path';
+
+import type { NniManagerArgs } from './arguments';
+
+export interface NniPaths {
+    readonly experimentRoot: string;
+    readonly experimentsDirectory: string;
+    readonly logDirectory: string;  // contains nni manager and dispatcher log; trial logs are not here
+    readonly nniManagerLog: string;
+}
+
+export function createPaths(args: NniManagerArgs): NniPaths {
+    assert(
+        path.isAbsolute(args.experimentsDirectory),
+        `Command line arg --experiments-directory "${args.experimentsDirectory}" is not absoulte`
+    );
+    const experimentRoot = path.join(args.experimentsDirectory, args.experimentId);
+
+    const logDirectory = path.join(experimentRoot, 'log');
+
+    // TODO: move all `mkdir`s here
+    fs.mkdirSync(logDirectory, { recursive: true });
+
+    const nniManagerLog = path.join(logDirectory, 'nnimanager.log');
+
+    return {
+        experimentRoot,
+        experimentsDirectory: args.experimentsDirectory,
+        logDirectory,
+        nniManagerLog,
+    };
+}

ts/nni_manager/common/globals/unittest.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ *  Unit test helper.
+ *  It should be inside "test", but must be here for compatibility, until we refactor all test cases.
+ *
+ *  Use this module to replace NNI globals with mocked values:
+ *
+ *      import globals from 'common/globals/unittest';
+ *
+ *  You can then edit these mocked globals and the injection will be visible to all modules.
+ *  Remember to invoke `resetGlobals()` in "after()" hook if you do so.
+ **/
+
+import os from 'os';
+import path from 'path';
+
+import type { NniManagerArgs } from './arguments';
+import { NniPaths, createPaths } from './paths';
+
+// copied from https://www.typescriptlang.org/docs/handbook/2/mapped-types.html
+type Mutable<Type> = {
+    -readonly [Property in keyof Type]: Type[Property];
+};
+
+export interface MutableGlobals {
+    args: Mutable<NniManagerArgs>;
+    paths: Mutable<NniPaths>;
+}
+
+export function resetGlobals(): void {
+    const args: NniManagerArgs = {
+        port: 8080,
+        experimentId: 'unittest',
+        action: 'create',
+        experimentsDirectory: path.join(os.homedir(), 'nni-experiments'),
+        logLevel: 'info',
+        foreground: false,
+        urlPrefix: '',
+        mode: 'unittest',
+        dispatcherPipe: undefined
+    };
+
+    const paths = createPaths(args);
+
+    const globals = { args, paths };
+    if (global.nni === undefined) {
+        global.nni = globals;
+    } else {
+        Object.assign(global.nni, globals);
+    }
+}
+
+function isUnitTest(): boolean {
+    const event = process.env['npm_lifecycle_event'] ?? '';
+    return event.startsWith('test') || event === 'mocha' || event === 'nyc';
+}
+
+if (isUnitTest()) {
+    resetGlobals();
+}
+
+const globals: MutableGlobals = global.nni;
+export default globals;

ts/nni_manager/common/utils.ts

@@ -18,21 +18,22 @@ import { Container } from 'typescript-ioc';
 import glob from 'glob';
 
 import { Database, DataStore } from './datastore';
-import { getExperimentStartupInfo, setExperimentStartupInfo } from './experimentStartupInfo';
+import globals from './globals';
+import { resetGlobals } from './globals/unittest';  // TODO: this file should not contain unittest helpers
 import { ExperimentConfig, Manager } from './manager';
 import { ExperimentManager } from './experimentManager';
 import { HyperParameters, TrainingService, TrialJobStatus } from './trainingService';
 
 function getExperimentRootDir(): string {
-    return getExperimentStartupInfo().logDir;
+    return globals.paths.experimentRoot;
 }
 
 function getLogDir(): string {
-    return path.join(getExperimentRootDir(), 'log');
+    return globals.paths.logDirectory;
 }
 
 function getLogLevel(): string {
-    return getExperimentStartupInfo().logLevel;
+    return globals.args.logLevel;
 }
 
 function getDefaultDatabaseDir(): string {
@@ -153,18 +154,7 @@ function prepareUnitTest(): void {
     Container.snapshot(Manager);
     Container.snapshot(ExperimentManager);
 
-    setExperimentStartupInfo({
-        port: 8080,
-        experimentId: 'unittest',
-        action: 'create',
-        experimentsDirectory: path.join(os.homedir(), 'nni-experiments'),
-        logLevel: 'info',
-        foreground: false,
-        urlPrefix: '',
-        mode: 'unittest',
-        dispatcherPipe: undefined,
-    });
-    mkDirPSync(getLogDir());
+    resetGlobals();
 
     const sqliteFile: string = path.join(getDefaultDatabaseDir(), 'nni.sqlite');
     try {

ts/nni_manager/main.ts

@@ -28,7 +28,7 @@ import { Container, Scope } from 'typescript-ioc';
 import * as component from 'common/component';
 import { Database, DataStore } from 'common/datastore';
 import { ExperimentManager } from 'common/experimentManager';
-import { NniManagerArgs, parseArgs } from 'common/globals/arguments';
+import globals, { initGlobals } from 'common/globals';
 import { getLogger, setLogLevel, startLogging } from 'common/log';
 import { Manager } from 'common/manager';
 import { TensorboardManager } from 'common/tensorboardManager';
@@ -40,10 +40,6 @@ import { SqlDB } from 'core/sqlDatabase';
 import { RestServer } from 'rest_server';
 
 import path from 'path';
-import { setExperimentStartupInfo } from 'common/experimentStartupInfo';
-
-// TODO: this line should be inside initGlobals()
-const args: NniManagerArgs = parseArgs(process.argv.slice(2));
 
 async function start(): Promise<void> {
     getLogger('main').info('Start NNI manager');
@@ -57,7 +53,7 @@ async function start(): Promise<void> {
     const ds: DataStore = component.get(DataStore);
     await ds.init();
 
-    const restServer = new RestServer(args.port, args.urlPrefix);
+    const restServer = new RestServer(globals.args.port, globals.args.urlPrefix);
     await restServer.start();
 }
 
@@ -74,12 +70,11 @@ process.on('SIGINT', shutdown);
 
 /* main */
 
+initGlobals();
+
 // TODO: these should be handled inside globals module
-setExperimentStartupInfo(args);
-const logDirectory = path.join(args.experimentsDirectory, args.experimentId, 'log');
-fs.mkdirSync(logDirectory, { recursive: true });
-startLogging(path.join(logDirectory, 'nnimanager.log'));
-setLogLevel(args.logLevel);
+startLogging(globals.paths.nniManagerLog);
+setLogLevel(globals.args.logLevel);
 
 start().then(() => {
     getLogger('main').debug('start() returned.');

ts/nni_manager/rest_server/index.ts

@@ -30,9 +30,8 @@ import express, { Request, Response, Router } from 'express';
 import httpProxy from 'http-proxy';
 import { Deferred } from 'ts-deferred';
 
-import { Singleton } from 'common/component';
+import globals from 'common/globals';
 import { Logger, getLogger } from 'common/log';
-import { getLogDir } from 'common/utils';
 import { createRestHandler } from './restHandler';
 
 /**
@@ -41,7 +40,6 @@ import { createRestHandler } from './restHandler';
  *  RestServer must be initialized with start() after NNI manager constructing, but not necessarily after initializing.
  *  This is because RestServer needs NNI manager instance to register API handlers.
  **/
-@Singleton
 export class RestServer {
     private port: number;
     private urlPrefix: string;
@@ -122,7 +120,7 @@ function rootRouter(stopCallback: () => Promise<void>): Router {
     // The REST API path "/logs" does not match file system path "/log".
     // Here we use an additional router to workaround this problem.
     const logRouter = Router();
-    logRouter.get('*', express.static(logDirectory ?? getLogDir()));
+    logRouter.get('*', express.static(globals.paths.logDirectory));
     router.use('/logs', logRouter);
 
     /* NAS model visualization */
@@ -151,11 +149,10 @@ function netronProxy(): Router {
 
 let webuiPath: string = path.resolve('static');
 let netronUrl: string = 'https://netron.app';
-let logDirectory: string | undefined = undefined;
 
 export namespace UnitTestHelpers {
     export function getPort(server: RestServer): number {
-        return (<any>server).port;
+        return (server as any).port;
     }
 
     export function setWebuiPath(mockPath: string): void {
@@ -165,8 +162,4 @@ export namespace UnitTestHelpers {
     export function setNetronUrl(mockUrl: string): void {
         netronUrl = mockUrl;
     }
-
-    export function setLogDirectory(path: string): void {
-        logDirectory = path;
-    }
 }

ts/nni_manager/test/core/dataStore.test.ts

@@ -8,7 +8,6 @@ import { Container, Scope } from 'typescript-ioc';
 
 import * as component from '../../common/component';
 import { Database, DataStore, TrialJobInfo } from '../../common/datastore';
-import { setExperimentStartupInfo } from '../../common/experimentStartupInfo';
 import { ExperimentProfile, TrialJobStatistics } from '../../common/manager';
 import { TrialJobStatus } from '../../common/trainingService';
 import { cleanupUnitTest, prepareUnitTest } from '../../common/utils';

ts/nni_manager/test/core/sqlDatabase.test.ts

@@ -9,7 +9,6 @@ import * as path from 'path';
 import { Container } from 'typescript-ioc';
 import * as component from '../../common/component';
 import { Database, MetricDataRecord, TrialJobEvent, TrialJobEventRecord } from '../../common/datastore';
-import { setExperimentStartupInfo } from '../../common/experimentStartupInfo';
 import { ExperimentConfig, ExperimentProfile } from '../../common/manager';
 import { cleanupUnitTest, getDefaultDatabaseDir, mkDirP, prepareUnitTest } from '../../common/utils';
 import { SqlDB } from '../../core/sqlDatabase';

ts/nni_manager/test/rest_server/rest_server.test.ts

@@ -7,7 +7,7 @@ import path from 'path';
 
 import fetch from 'node-fetch';
 
-import { setExperimentStartupInfo } from 'common/experimentStartupInfo';
+import globals, { resetGlobals } from 'common/globals/unittest';
 import { RestServer, UnitTestHelpers } from 'rest_server';
 import * as mock_netron_server from './mock_netron_server';
 
@@ -121,6 +121,7 @@ before(async () => {
 
 after(async () => {
     await restServer.shutdown();
+    resetGlobals();
 });
 
 async function configRestServer(urlPrefix?: string) {
@@ -128,7 +129,7 @@ async function configRestServer(urlPrefix?: string) {
         await restServer.shutdown();
     }
 
-    UnitTestHelpers.setLogDirectory(path.join(__dirname, 'log'));
+    globals.paths.logDirectory = path.join(__dirname, 'log');
     UnitTestHelpers.setWebuiPath(path.join(__dirname, 'static'));
 
     restServer = new RestServer(0, urlPrefix ?? '');