Update pai yaml merge method #2369

SparkSnail · 2020-04-23T17:43:46Z

No description provided.

merge master

Update evolution doc (microsoft#1493)

merge master

augment pylintrc (microsoft#1643)

fix console.log (microsoft#1636)

merge master

Filter prune algo implementation (microsoft#1655)

merge master

Support monitor mode when creating or resuming a new experiment (microsoft#1933)

Add test for documentation build (microsoft#1924)

fix pipeline status badge (microsoft#1942)

merge master

ultmaster · 2020-04-26T13:43:34Z

tools/nni_cmd/launcher_utils.py

-                    print_error('Please set paiStoragePlugin field, or set plugin in your own paiConfig!')
-                    exit(1)
-                experiment_config['trial']['paiStoragePlugin'] = pai_config['extras']['com.microsoft.pai.runtimeplugin'][0]['plugin']
+            if pai_config.get('taskRoles', {}).get('taskrole', {}).get('commands', [])[0] is None:


Default of commands is []. Using [0] would raise an error.

ultmaster · 2020-04-26T13:46:34Z

tools/nni_cmd/launcher_utils.py

+            if pai_config.get('taskRoles', {}).get('taskrole', {}).get('commands', [])[0] is None:
+                print_error('Please set paiStoragePlugin field, or set plugin in your own paiConfig!')
+                exit(1)
+            experiment_config['trial']['command'] = pai_config['taskRoles']['taskrole']['commands'][0]


Seems that the support of PAI configuration is quite limited:

Taskrole with name taskrole needs to exist.

Only commands in taskrole will be wrapped with trial keeper.

Multi-line commands are not supported.

Better states that in docs.

Yes, uses need to provide commands field in config file, which is under taskrole field.

ultmaster · 2020-04-26T13:46:51Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

-                    }
+        let paiJobConfig: any = undefined;
+        if (this.paiTrialConfig.paiConfigPath) {
+                paiJobConfig = yaml.safeLoad(fs.readFileSync(this.paiTrialConfig.paiConfigPath, 'utf8'));


Bad indentation. Will this actually pass linter?

ultmaster · 2020-04-26T13:47:01Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

-            } catch (error) {
-                this.log.error(`Error occurs during loading and merge ${this.paiTrialConfig.paiConfigPath} : ${error}`);
+            if (this.paiTrialConfig.virtualCluster) {
+                paiJobConfig.defaults=  {


Missing whitespace.

is eslint correctly enabled?

eslint is enabled, but seems didn't detect whitespace errors.

@chicm-ms could you help confirm that our current eslint config does not check whitespace?

ultmaster · 2020-04-26T13:50:55Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

+        let paiJobConfig: any = undefined;
+        if (this.paiTrialConfig.paiConfigPath) {
+                paiJobConfig = yaml.safeLoad(fs.readFileSync(this.paiTrialConfig.paiConfigPath, 'utf8'));
+                paiJobConfig.name = jobName;


Actually I found the job name nni generated are not human-readable and quite meaningless. I hope that the job name specified in PAI configuration can be used as a prefix of jobName. For example, if in my PAI yaml, name is proxylessnas, here paiJobConfig.name = "proxylessnas_nni_bdef1234_12345". However, having a long jobname is also troublesome on PAI. It's up to you to decide.

Sure, refactor jobName is another topic, this pr will keep consistent with original design temporarily.

ultmaster · 2020-04-26T13:51:34Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

@@ -144,66 +143,60 @@ class PAIK8STrainingService extends PAITrainingService {
            throw new Error('trial config is not initialized');
        }
        const jobName = `nni_exp_${this.experimentId}_trial_${trialJobId}`


I think showing experimentName in jobName would be more user-friendly.

Experiment Id will be shown in NNICTL commands, it is easier to get for users..

ultmaster · 2020-04-26T13:52:45Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

-                    }
+        let paiJobConfig: any = undefined;
+        if (this.paiTrialConfig.paiConfigPath) {
+                paiJobConfig = yaml.safeLoad(fs.readFileSync(this.paiTrialConfig.paiConfigPath, 'utf8'));


If my paiJobConfig is accidentally changed or deleted during the run of experiment, it will submit inconsistent jobs. RIght?

Yes, paiJobConfig file is read when new trial job is created, it may be changed by users.

ultmaster · 2020-04-26T13:55:59Z

src/nni_manager/training_service/pai/paiK8S/paiK8STrainingService.ts

+        if (this.paiTrialConfig.paiConfigPath) {
+                paiJobConfig = yaml.safeLoad(fs.readFileSync(this.paiTrialConfig.paiConfigPath, 'utf8'));
+                paiJobConfig.name = jobName;
+                paiJobConfig.taskRoles.taskrole.commands = [command]


(I can't comment on L266 of this file.) Better to escape special characters in command. But it's not necessary for this PR.

so there is only one key (i.e., taskrole) under taskRoles? what if there are multiple keys?

NNI only parse the first taskrole, users should provide multiple taskroles in their configuration.

you means only the first taskrole's command is wrapped by trial keeper? other taskroles' command is kept as it is

Not the first taskrole, but the taskrole named "taskrole".

update logic, NNI will use all of taskRoles now.

QuanluZhang · 2020-04-29T06:37:21Z

@SparkSnail please update doc to make the usage very clear.

SparkSnail · 2020-04-29T08:06:07Z

@SparkSnail please update doc to make the usage very clear.

updated.

QuanluZhang · 2020-04-29T08:23:27Z

docs/en_US/TrainingService/PaiMode.md

 * paiConfigPath
    * Optional key. Set the file path of pai job configuration, the file is in yaml format.
+    If users set paiConfigPath in NNI's configuration file, the `command`, `paiStoragePlugin`, `virtualCluster`, `image`, `memoryMB`, `cpuNum`, `gpuNum` in `trial` filed will lose efficacy, and configurations from `paiConfigPath` will take effect. Notice that if users set `paiConfigPath`, NNI will check the validation of `commands` under  `taskRoles/taskRole/commands`.


suggest to explain the merge rule when user specify paiConfigPath, for example, jobName is changed by NNI to become a random string?

Some notes (limitations) I'm thinking of:

Taskrole with name taskrole needs to exist.

Only commands in taskrole will be wrapped with trial keeper. Other taskroles will be kept as is.

Multi-line commands are not supported. Only the first command will be kept and wrapped with trial keeper.

Special characters (like " and \) are not supported in commands.

jobName specified in the original PAI configuration will be ignored.

PAI configuration needs to be kept unchanged during the whole experiment. If it's changed or deleted, either accidentally or not, nni will use the updated configuration file to submit new jobs.

Although some of these "limitations" are by design, I think, it's still "good to know" for users.

Also, what is "efficacy"?

Updated.

add explication for jobName in doc.

remove hardcode taskrole, traversal all taskRoles instead.

wrap all taskRoles' command.

multi-line command will be connected by &&.

use \ to transfer meaning for " and \ in command.

In current design, jobName in original PAI configuration will be replaced to NNI's job name, If we add original jobName in NNI experiment, it may cause name-too-long exception in PAI.

only read original PAI configuration once in experiment, it will keep consistent in all trial jobs.

QuanluZhang · 2020-04-30T10:31:40Z

docs/en_US/TrainingService/PaiMode.md

+    Note:
+
+      1.  If users set multiple taskRoles in PAI's configuration file, NNI will wrap all of these taksRoles and start multiple tasks in one trial job, users should ensure that only one taskRole report metric to NNI, otherwise there might be some conflict error. 
+      2. The job name in PAI's configuration file will be replaced by a new job name, the new job name is created by NNI, the name format is nni_exp_${this.experimentId}_trial_${trialJobId}


better to switch point 1 and point 2.

QuanluZhang · 2020-04-30T10:35:36Z

docs/en_US/TrainingService/PaiMode.md

 * paiConfigPath
    * Optional key. Set the file path of pai job configuration, the file is in yaml format.
+    If users set paiConfigPath in NNI's configuration file, the `command`, `paiStoragePlugin`, `virtualCluster`, `image`, `memoryMB`, `cpuNum`, `gpuNum` in `trial` filed will be replaced by configurations from `paiConfigPath`.   


If users set paiConfigPath in NNI's configuration file, no need to specify the fields command, paiStoragePlugin, virtualCluster, image, memoryMB, cpuNum, gpuNum in trial configuration. These fields will use the values from the config file specified by paiConfigPath.

merge master

QuanluZhang · 2020-04-30T10:53:28Z

@SparkSnail , suggest to add UT or IT to test this merge feature

SparkSnail and others added 30 commits August 6, 2019 18:58

Merge pull request #200 from microsoft/master

704b50e

merge master

Merge pull request #204 from microsoft/master

5b0034e

merge master

Merge pull request #205 from microsoft/master

8fe2588

Update evolution doc (microsoft#1493)

Merge pull request #206 from microsoft/master

9fae194

merge master

Merge pull request #207 from microsoft/master

c785655

merge master

Merge pull request #208 from microsoft/master

2f5272c

merge master

Merge pull request #209 from microsoft/master

1892bc2

augment pylintrc (microsoft#1643)

Merge pull request #210 from microsoft/master

7c1ab11

fix console.log (microsoft#1636)

Merge pull request #211 from microsoft/master

8c203f3

merge master

check pylint for nni_cmd

d7a62f6

fix id error

e259d10

Merge pull request #212 from microsoft/master

4997295

merge master

Merge pull request #213 from microsoft/master

c037a7c

merge master

Merge pull request #214 from microsoft/master

7620e7c

merge master

Merge pull request #215 from microsoft/master

d16dbe9

Filter prune algo implementation (microsoft#1655)

Merge pull request #216 from microsoft/master

9ce751d

merge master

Merge pull request #217 from microsoft/master

a0846f2

merge master

Merge pull request #218 from microsoft/master

cd3a912

merge master

Merge pull request #219 from microsoft/master

32efaa3

merge master

Merge pull request #220 from microsoft/master

543239c

merge master

Merge pull request #221 from microsoft/master

36e6e35

merge master

Merge pull request #222 from microsoft/master

f9ee589

merge master

Merge pull request #223 from microsoft/master

b9a7a95

merge master

Merge pull request #224 from microsoft/master

1a5c017

merge master

Merge pull request #225 from microsoft/master

392460a

merge master

Merge pull request #226 from microsoft/master

9bafa4c

Support monitor mode when creating or resuming a new experiment (microsoft#1933)

Merge pull request #227 from microsoft/master

c23b807

Add test for documentation build (microsoft#1924)

Merge pull request #228 from microsoft/master

4132f62

fix pipeline status badge (microsoft#1942)

add merge config

92c2ce7

fix comments

956b413

SparkSnail added 6 commits April 16, 2020 21:29

Merge pull request #241 from microsoft/master

6728799

merge master

Merge pull request #242 from microsoft/master

1b9daa3

merge master

Merge pull request #243 from microsoft/master

e0c2c0e

merge master

fix conflict

bd9fda5

update pai yaml merge

0e1ad4f

remove deepmerge package

61c8eaf

SparkSnail requested a review from ultmaster April 24, 2020 08:49

ultmaster reviewed Apr 26, 2020

View reviewed changes

SparkSnail added 2 commits April 29, 2020 14:53

fix comments

052b3ec

add doc for paiConfigPath

ce11bf7

QuanluZhang requested review from chicm-ms and liuzhe-lz April 29, 2020 08:20

QuanluZhang reviewed Apr 29, 2020

View reviewed changes

SparkSnail added 3 commits April 30, 2020 01:23

fix comments

8aca557

update doc

8a3768f

fix pylint

f4b4775

QuanluZhang reviewed Apr 30, 2020

View reviewed changes

SparkSnail added 5 commits April 30, 2020 18:41

Merge pull request #244 from microsoft/master

e29b58a

merge master

fix comments

caadc04

remove unused doc

0516655

format doc

9201699

update yarn.loc

45ce90d

QuanluZhang approved these changes Apr 30, 2020

View reviewed changes

ultmaster approved these changes May 6, 2020

View reviewed changes

SparkSnail merged commit ee25377 into microsoft:master May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pai yaml merge method #2369

Update pai yaml merge method #2369

SparkSnail commented Apr 23, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

QuanluZhang Apr 29, 2020

SparkSnail Apr 29, 2020

QuanluZhang Apr 29, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

SparkSnail Apr 29, 2020

ultmaster Apr 26, 2020

QuanluZhang Apr 29, 2020

SparkSnail Apr 29, 2020

QuanluZhang Apr 29, 2020

ultmaster Apr 29, 2020

SparkSnail Apr 29, 2020

QuanluZhang commented Apr 29, 2020

SparkSnail commented Apr 29, 2020

QuanluZhang Apr 29, 2020

ultmaster Apr 29, 2020

SparkSnail Apr 29, 2020

QuanluZhang Apr 30, 2020

SparkSnail Apr 30, 2020

QuanluZhang Apr 30, 2020

SparkSnail Apr 30, 2020

QuanluZhang commented Apr 30, 2020

Update pai yaml merge method #2369

Update pai yaml merge method #2369

Conversation

SparkSnail commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuanluZhang commented Apr 29, 2020

SparkSnail commented Apr 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuanluZhang commented Apr 30, 2020