migrate nnicli #3334
migrate nnicli #3334
Conversation
| @@ -142,17 +142,6 @@ testCases: | |||
| kwargs: | |||
| import_data_file_path: config/nnictl_experiment/test_import.json | |||
|
|
|||
| - name: nnicli | |||
There was a problem hiding this comment.
similar test should be added back to test the refactored code?
There was a problem hiding this comment.
we don't support launch from config file and stop a connected experiment right now, will add back in next pr.
| @@ -193,8 +207,369 @@ def run(self, port: int = 8080, debug: bool = False) -> bool: | |||
| self.stop() | |||
|
|
|||
|
|
|||
| def get_status(self) -> str: | |||
| def connect_experiment(self, port: int): | |||
There was a problem hiding this comment.
it would be better to have a BaseExperiment class which has basic functionality of an experiment, then write different Experiment classes inheriting BaseExperiment to provide view/launch experiment and to manage experiments
There was a problem hiding this comment.
good point, we need to discuss this, in the previous discussion, we use parameters to distinguish different launch ways.
|
this pr is about the design of experiment apis, let's fully discuss it |
| if config is None: | ||
| self.config = ExperimentConfig(training_service) | ||
| else: | ||
| self.config = config |
There was a problem hiding this comment.
Suggest print a message when tuner is None.
There was a problem hiding this comment.
yes, I will add a message.
| self.get_status() | ||
|
|
||
|
|
||
| def _experiment_rest_get(self, port: int, api: str) -> Any: |
There was a problem hiding this comment.
double-line between member functions looks weird to me. Suggest reformatting this file with tools like autopep8.
There was a problem hiding this comment.
I will confirm with @liuzhe-lz
nni/experiment/experiment.py
Outdated
| return "TrialResult(parameter: {} value: {} trialJobId: {})".format(self.parameter, self.value, self.trialJobId) | ||
|
|
||
|
|
||
| class TrialMetricData: |
There was a problem hiding this comment.
Maybe we should rewrite these classes with @DataClass?
There was a problem hiding this comment.
good suggestion, I will rewrite them
| @@ -93,7 +93,7 @@ class NnicliValidator(ITValidator): | |||
| def __call__(self, rest_endpoint, experiment_dir, nni_source_dir, **kwargs): | |||
| print(rest_endpoint) | |||
| exp = Experiment() | |||
| exp.connect_experiment(rest_endpoint) | |||
| exp.connect_experiment(int(rest_endpoint.split(':')[-1])) | |||
There was a problem hiding this comment.
Is this the correct way to get the port part? What if the endpoint ends with /?
There was a problem hiding this comment.
it's a workaround, and will refactor in next pr, this will never be called right now.
need do in next pr: