[WIP] Enable optional Pod Spec for FrameworkController platform #3379

mbu93 · 2021-02-10T14:06:48Z

As discussed in #3350, this PR introduces:

The option to specify a custom framework controller config for framework controller mode
A minimum working setup for the mnist-pytorch example (nni config and framework controller config)
Required changes to the related python parser / webserver logic

In addition, the following changes have been applied:

Option to specify a k8s namespace for the trials to run

Currently, the following components have not yet been considered (thus the WIP state):

documentation for the code and examples
unit tests covering the changes

ghost · 2021-02-10T14:07:02Z

All CLA requirements met.

SparkSnail · 2021-02-22T08:37:58Z

nni/tools/nnictl/config_schema.py

+    }, {Optional('configPath'): setType('configPath', str),
+        Optional('storage'): setChoice('storage', 'nfs', 'azureStorage', 'pvc'),
+        Optional('serviceAccountName'): setType('serviceAccountName', str),
+        Optional('configPath'): setType('configPath', str),


duplicated configPath

Deleted it.

SparkSnail · 2021-02-22T08:42:10Z

nni/tools/nnictl/config_schema.py

+        Optional('storage'): setChoice('storage', 'nfs', 'azureStorage', 'pvc'),
+        Optional('serviceAccountName'): setType('serviceAccountName', str),
+        Optional('configPath'): setType('configPath', str),
+        'pvc': {'path': setType('server', str)},


Is configPath setting only for pvc storage? When set pvc storage, is it a optional field?

I changed it so configPath is optional for NFS and Azure and mandatory for PVC, as it is required there to attach a PVC at all (via custom template)

SparkSnail · 2021-02-22T08:52:02Z

nni/tools/nnictl/config_schema.py

@@ -364,7 +370,7 @@ def validate(self, data):
 frameworkcontroller_trial_schema = {
    'trial': {
        'codeDir':  setPathCheck('codeDir'),
-        'taskRoles': [{
+        Optional('taskRoles'): [{


If set taskRoles as optional field, better to add validation, refer https://github.com/microsoft/nni/blob/master/nni/tools/nnictl/config_schema.py#L536

I totally missed that validation part of the code! Super cool. I added a (even a little more extended) validation for the frameworkcontroller job config.

SparkSnail · 2021-02-22T09:26:29Z

...anager/training_service/kubernetes/frameworkcontroller/frameworkcontrollerTrainingService.ts

+                minSucceededTaskCount: -1
+            }
+            const trialConfig = <FrameworkControllerTrialConfigTemplate>{
+                name: x.name,


will x.name be undefined?

Using the new validation mechanism, it is ensured, that the name field will never be empty. Is this sufficient or should
I handle the undefined case at that point anyways?

If it is validated in other place, it's ok here.

SparkSnail · 2021-02-22T09:36:05Z

...anager/training_service/kubernetes/frameworkcontroller/frameworkcontrollerTrainingService.ts

+            configTaskRoles = this.parseCustomTaskRoles(this.fcTemplate.spec.taskRoles)
+        }
+        const namespace = this.fcClusterConfig.namespace ? this.fcClusterConfig.namespace : "default";
+        this.genericK8sClient.setNamespace = namespace


SparkSnail · 2021-02-22T09:45:30Z

...anager/training_service/kubernetes/frameworkcontroller/frameworkcontrollerTrainingService.ts

@@ -156,7 +253,7 @@ class FrameworkControllerTrainingService extends KubernetesTrainingService imple

                // Validate to make sure codeDir doesn't have too many files
                try {
-                    await validateCodeDir(this.fcTrialConfig.codeDir);
+                    await validateCodeDir(this.fcTrialConfig ? this.fcTrialConfig.codeDir : './');


If this.fcTrialConfig.codeDir is undefined, what does ./ contains?

I removed this again. I actually didn't realize, the path is modified within the python logic and felt like setting it to the "current" folder per default (which wouldn't turn out to work as I realized).

SparkSnail · 2021-02-22T09:57:26Z

...anager/training_service/kubernetes/frameworkcontroller/frameworkcontrollerTrainingService.ts

@@ -202,6 +301,10 @@ class FrameworkControllerTrainingService extends KubernetesTrainingService imple
            const fcClusterConfigNFS: FrameworkControllerClusterConfigNFS = <FrameworkControllerClusterConfigNFS>this.fcClusterConfig;
            const nfsConfig: NFSConfig = fcClusterConfigNFS.nfs;
            return `nfs://${nfsConfig.server}:${destDirectory}`;
+        } else if (this.fcClusterConfig.storage === 'pvc') {
+            await cpp.exec(`mkdir -p ${this.trialLocalNFSTempFolder}/${destDirectory}`);


trialLocalNFSTempFolder is used for NFS storage, rename trialLocalNFSTempFolder to trialLocalPVCTempFolder here, or unify them to trialLocalTempFolder.

I unified it :)

SparkSnail · 2021-02-22T09:59:33Z

please add doc for the configuration change.

SparkSnail · 2021-02-22T11:35:12Z

examples/trials/mnist-pytorch/fc_template.yml

+            emptyDir: {}
+          - name: data-volume
+            persistentVolumeClaim:
+              claimName: nni-storage


Does users need to create PVC manually before create NNI experiments?

Yes. I added some documentation for the new custom capability and explicitly mentioned this prerequisite. I'm aware of the
fact, that the k8s API offers and endpoint to create a PVC algorithmically, but this can easily mess up a cluster's storage management, so this seems like a bad idea. If a person is willing to use a PVC, then I guess it should be somewhat enforced this person has sufficient rights to do so and is aware what's about to happen, which makes me think manual creation beforehand is a good idea. What do you think?

Agree to let users manually create PVC, as long as we declare it in doc and examples.

mbu93 · 2021-02-23T09:55:20Z

Hey @SparkSnail,
Thanks for the review! I'll try to take care of the required changes asap! :)

J-shang · 2021-02-25T03:14:50Z

Hi @mbu93 Thanks for your contribution, we are about to code freeze, if it's not urgent, this pr will defer to the next version.

mbu93 · 2021-03-04T09:36:29Z

Hey @SparkSnail, I was busy with a deadline and didn't find the time to take care of the PR. Next Version is fine, it's not urgent :)

- a check for empty taskRoles - a check for empty or duplicate taskRole names - configPath validation for frameworkcontroller jobs (necessity / availability) - an optional configPath field for NFS and Azure configs - custom templates for NFS and azure - revoked path defaults ("./") as this is already handled in config parsing - unified temp paths

mbu93 · 2021-03-12T16:53:11Z

Note: I just realized there is a little bug pending, that will cause the 2nd+ trial to use the wrong command, leading to a failed reporting of the results. I must have overseen that when testing. I'll take care of it asap.

mbu93 · 2021-03-19T09:17:57Z

@SparkSnail @J-shang I applied the changes as proposed in the review and solved the remaining issue. Currently, however, python unit tests fail due to Yann LeCun's page being unreachable when downloading the MNIST dataset for some test scenarios :/ Speaking about tests: Is there any work to be done in this PR?

SparkSnail · 2021-03-23T11:22:17Z

@SparkSnail @J-shang I applied the changes as proposed in the review and solved the remaining issue. Currently, however, python unit tests fail due to Yann LeCun's page being unreachable when downloading the MNIST dataset for some test scenarios :/ Speaking about tests: Is there any work to be done in this PR?

Looks good to me.

…osoft#3379)

mbu93 added 7 commits February 3, 2021 18:20

Adapt python nnictl tool to parse the new schema

f51e96b

Add custom framework controller example and config template

af38d74

Add backend validators and code to load and send the custom fc template

9a81a28

Fix small missing Optional for trails taskRole field

3648381

Change example path

9df007e

Add namespace definition possibility

2640be7

Improve examples

83a48c1

J-shang requested review from SparkSnail and liuzhe-lz February 19, 2021 08:33

SparkSnail reviewed Feb 22, 2021

View reviewed changes

SparkSnail closed this Feb 22, 2021

SparkSnail reopened this Feb 22, 2021

SparkSnail reviewed Feb 22, 2021

View reviewed changes

mbu93 added 2 commits March 12, 2021 11:57

Add documentation

948c893

mbu93 added 2 commits March 15, 2021 14:17

Fix typescript linting error

6c9cc33

Fix result reporting bug

c7456b4

SparkSnail approved these changes Mar 23, 2021

View reviewed changes

SparkSnail closed this Mar 23, 2021

SparkSnail reopened this Mar 23, 2021

liuzhe-lz approved these changes Mar 29, 2021

View reviewed changes

SparkSnail merged commit fbffbc7 into microsoft:master Apr 1, 2021

acured pushed a commit to acured/nni that referenced this pull request Apr 7, 2021

[WIP] Enable optional Pod Spec for FrameworkController platform (micr…

32363e0

…osoft#3379)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Enable optional Pod Spec for FrameworkController platform #3379

[WIP] Enable optional Pod Spec for FrameworkController platform #3379

mbu93 commented Feb 10, 2021 •

edited

Loading

ghost commented Feb 10, 2021 •

edited by ghost

Loading

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021

SparkSnail Feb 22, 2021 •

edited

Loading

mbu93 Mar 12, 2021

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021 •

edited

Loading

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021

SparkSnail Mar 23, 2021

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021

SparkSnail Feb 22, 2021 •

edited

Loading

mbu93 Mar 12, 2021

SparkSnail commented Feb 22, 2021

SparkSnail Feb 22, 2021

mbu93 Mar 12, 2021

SparkSnail Mar 23, 2021

mbu93 commented Feb 23, 2021

J-shang commented Feb 25, 2021

mbu93 commented Mar 4, 2021

mbu93 commented Mar 12, 2021

mbu93 commented Mar 19, 2021

SparkSnail commented Mar 23, 2021

[WIP] Enable optional Pod Spec for FrameworkController platform #3379

[WIP] Enable optional Pod Spec for FrameworkController platform #3379

Conversation

mbu93 commented Feb 10, 2021 • edited Loading

ghost commented Feb 10, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkSnail Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbu93 Mar 12, 2021 • edited Loading

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

SparkSnail Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkSnail commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbu93 commented Feb 23, 2021

J-shang commented Feb 25, 2021

mbu93 commented Mar 4, 2021

mbu93 commented Mar 12, 2021

mbu93 commented Mar 19, 2021

SparkSnail commented Mar 23, 2021

mbu93 commented Feb 10, 2021 •

edited

Loading

ghost commented Feb 10, 2021 •

edited by ghost

Loading

SparkSnail Feb 22, 2021 •

edited

Loading

mbu93 Mar 12, 2021 •

edited

Loading

SparkSnail Feb 22, 2021 •

edited

Loading