config refactor stage 1 - ExperimentConfig and LocalConfig #4313
Conversation
nni/experiment/config/exp_config.py
Outdated
| raise ValueError('ExperimentConfig: search_space and search_space_file must be set one') | ||
|
|
||
| if self.search_space_file is not None: | ||
| self.search_space = yaml.safe_load(open(self.search_space_file)) |
There was a problem hiding this comment.
does the file need close()?
There was a problem hiding this comment.
Well I'm pretty sure it won't cause problem to open a file read-only and let GC to close it, but you are right it's better to use with.
| from ..training_service import TrainingServiceConfig # avoid circular import | ||
|
|
||
| # import all custom config classes so they can be found in TrainingServiceConfig.__subclasses__() | ||
| custom_ts_config_path = nni.runtime.config.get_config_file('training_services.json') |
There was a problem hiding this comment.
What is content of training_service.json file? Does the file has path validation?
There was a problem hiding this comment.
This is a config file for custom training services, similar to the config file for registered algorithms.
The availability is guaranteed by the config module.
| def _canonicalize(self, parents): | ||
| super()._canonicalize(parents) | ||
| self.gpu_indices = canonical_gpu_indices(self.gpu_indices) | ||
| self.nni_manager_ip = None |
There was a problem hiding this comment.
do we have nni_manager_ip in the training service config before?
There was a problem hiding this comment.
nni_manager_ip, trial_command, trial_gpu_number, etc were only in the top level config and not in training service configs.
But it was decided several releases ago, according to discussion, that training service config should contain all things needed by training service. And then these fields in top level config will work as "shortcuts" for convenience and for compatibility.
Description
Port
ExperimentConfigto the new base class.Now the canonicalize and validation logic is shorter, and should be far more readable.
Checklist
How to test
Should be tested together with other configure refactor PRs.