Add native support for v2 config #3466
Conversation
nni/experiment/config/common.py
Outdated
| @@ -66,6 +68,7 @@ class ExperimentConfig(ConfigBase): | |||
| assessor: Optional[_AlgorithmConfig] = None | |||
| advisor: Optional[_AlgorithmConfig] = None | |||
| training_service: Union[TrainingServiceConfig, List[TrainingServiceConfig]] | |||
| _deprecated: Optional[str, Any] = None | |||
There was a problem hiding this comment.
Deprecated fields as well as fields not supported by v2 (yet)...
It appears in at least 3 places and I placed the comments somewhere else. Because this file is "more visible" to end users.
This is a workaround for backward compatibility.
| remove_algo_meta_data(meta['builtinName']) | ||
| save_algo_meta_data(meta) | ||
| else: | ||
| print_error(f'Cannot overwrite builtin algorithm') |
There was a problem hiding this comment.
I don't really understand the old logic but seems not.
We have no restriction on meta_list's order. So as you can see the old code calls verify_algo_import on a part of the list. I believe the correct logic is either verify all or verify none.
| @@ -160,7 +160,7 @@ class NNIRestHandler { | |||
| } | |||
|
|
|||
| private startExperiment(router: Router): void { | |||
| router.post('/experiment', expressJoi(ValidationSchemas.STARTEXPERIMENT), (req: Request, res: Response) => { | |||
| router.post('/experiment', (req: Request, res: Response) => { | |||
There was a problem hiding this comment.
we do not need to validate now?
There was a problem hiding this comment.
I think validate in launcher is enough. It's not DRY to validate twice.
If a user calls this API from a custom client, it's at their own risks.
| @@ -10,7 +10,7 @@ import { AzureStorage, KeyVaultConfig, KubernetesClusterConfig, KubernetesCluste | |||
| } from '../kubernetesConfig'; | |||
|
|
|||
| // operator types that kubeflow supported | |||
| export type KubeflowOperator = 'tf-operator' | 'pytorch-operator' ; | |||
| export type KubeflowOperator = string; // 'tf-operator' | 'pytorch-operator' | |||
There was a problem hiding this comment.
The unpack logic before this yields string, and tsc complained a lot type mismatch, so...
Too lazy.
nni/experiment/config/convert.py
Outdated
| data['logDir'] = data.pop('experimentWorkingDirectory') | ||
| def to_v2(v1) -> ExperimentConfig: | ||
| platform = v1.pop('trainingServicePlatform') | ||
| assert platform in ['local', 'remote', 'openpai'] |
There was a problem hiding this comment.
K8S based will stick with v1.
AML is a mistake and has been fixed.
| chai.assert.equal(environmentService.testGetEnvironments().size, 2, "as env not reused, so only 2 envs should be here."); | ||
| const trials = await trialDispatcher.listTrialJobs(); | ||
| chai.assert.equal(trials.length, 2, "there should be 2 trials"); | ||
| //trialDispatcher.setClusterMetadata( |
There was a problem hiding this comment.
forgot...
please let me fix it during bug bash
| classArgs?: object; | ||
| } | ||
|
|
||
| export interface ExperimentConfig { |
There was a problem hiding this comment.
Currently version check is bound to debug. I think it's enough for this release.
| maxExperimentDuration?: string; | ||
| maxTrialNumber?: number; | ||
| nniManagerIp?: string; | ||
| //useAnnotation: boolean; |
There was a problem hiding this comment.
does not support annotation anymore?
There was a problem hiding this comment.
Annotation is handled inside nnictl. All other modules does not aware whether it's user written code & search space, or it's de-annotated code & search space.
nni.Experiment does not support annotation for now.
No description provided.