feature: export experiment results #2706
Conversation
|
@QuanluZhang @SparkSnail PR is ready for review. |
docs/en_US/Tutorial/Nnictl.md
Outdated
| @@ -24,6 +24,7 @@ nnictl support commands: | |||
| * [nnictl package](#package) | |||
| * [nnictl ss_gen](#ss_gen) | |||
| * [nnictl --version](#version) | |||
| * [nnictl export_results](#export-results) | |||
There was a problem hiding this comment.
what is the difference of this command from nnictl experiment export in https://nni.readthedocs.io/en/latest/Tutorial/Nnictl.html#manage-experiment-information
There was a problem hiding this comment.
It dumps not only experiment settings, trial final results, but also all intermediate results.
There was a problem hiding this comment.
we should combine the commands, otherwise, the commands are messy. please try to merge your command to nnictl experiment export
There was a problem hiding this comment.
OK, I will add a CLI option to specify whether intermediate results are required or not.
There was a problem hiding this comment.
remove unused command in doc.
remove useless doc
tools/nni_cmd/nnictl_utils.py
Outdated
| if response is not None and check_response(response): | ||
| content = json.loads(response.text) |
There was a problem hiding this comment.
what is the content of response.text and what is the content of intermediate_results.text?
There was a problem hiding this comment.
response.text contains basic experiment settings, such as hyperparameters. intermediate_results.text only include trial id, intermediate result and timestamp.
tools/nni_cmd/nnictl_utils.py
Outdated
| trial_records = [] | ||
| for record in content: | ||
| record_value = json.loads(record['value']) | ||
| if not isinstance(record_value, (float, int)): | ||
| formated_record = {**record['parameter'], **record_value, **{'id': record['id']}} | ||
| if args.intermediate: |
There was a problem hiding this comment.
so many checks of if args.intermediate, could you refactor the logic flow a little bit to make it simpler?
There was a problem hiding this comment.
I manage to use only two if args.intermediate.
One is for requesting for intermediate data (at 711th line).
The other is for controlling the output of csv file (at 726th line).
tools/nni_cmd/nnictl_utils.py
Outdated
| return groupby | ||
|
|
||
| def trans_intermediate_dict(record): | ||
| return {'intermediate': '[' + str(reduce(lambda x, y: x + ',' + y, record)) + ']'} |
There was a problem hiding this comment.
i don't know the content of record, but could we use ','.join(record) instead of str(reduce(lambda x, y: x + ',' + y, record))?
There was a problem hiding this comment.
what is the content of record?
There was a problem hiding this comment.
record is a list with float numbers in it.
','.join() is a good solution.
There was a problem hiding this comment.
I have not tested intermediate results with dict metric. I will do it this afternoon.
docs/en_US/Tutorial/Nnictl.md
Outdated
| @@ -465,13 +465,14 @@ Debug mode will disable version check function in Trialkeeper. | |||
| |id| False| |ID of the experiment | | |||
| |--filename, -f| True| |File path of the output file | | |||
| |--type| True| |Type of output file, only support "csv" and "json"| | |||
| |--intermediate, -i|False||Is intermediate results required| | |||
There was a problem hiding this comment.
Are intermediate results included
grammar fix
tools/nni_cmd/nnictl_utils.py
Outdated
| print_error('Unknown type: %s' % args.type) | ||
| exit(1) | ||
| if not running: | ||
| print_error('Restful server is not Running') |
tools/nni_cmd/nnictl_utils.py
Outdated
| if response is not None and check_response(response): | ||
| content = json.loads(response.text) | ||
| if args.intermediate: | ||
| intermediate_results = rest_get(metric_data_url(rest_port), REST_TIME_OUT) |
There was a problem hiding this comment.
in my point, use intermediate_results_response is more meaningful.
tools/nni_cmd/nnictl_utils.py
Outdated
| sorted(intermediate_results, key=lambda x: x['timestamp']) | ||
| groupby = dict() | ||
| for content in intermediate_results: | ||
| groupby.setdefault(content['trialJobId'], []).append(eval(content['data'])) |
There was a problem hiding this comment.
The returned data is serialized. For example, "\"{\\\"default\\\": 93.64, \\\"other_metric\\\": 2.0}\"".
Using eval() can easily handle the \ part and reconstruct the data structure from the string. It is troublesome for users to handle serialized data.
The data from the server can be trusted so I think using eval here is safe here.
There was a problem hiding this comment.
why not simply use json.loads?
There was a problem hiding this comment.
It looks that json.loads() is a good solution.
There was a problem hiding this comment.
please double check whether it works properly
There was a problem hiding this comment.
Sure I will. Thanks for reminding me.
(cherry picked from commit d654eff)
No description provided.