-
Notifications
You must be signed in to change notification settings - Fork 1.8k
add nnictl command to list trial results with highest/lowest metric #2747
Conversation
What is the user scenario of this command? How do you handle dict metric? |
tools/nni_cmd/nnictl_utils.py
Outdated
else: | ||
print_error('Restful server is not Running') | ||
|
||
def experiment_tail(args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experiment_tail and experiment_head can be implemented with one function, use the sort order as parameter.
tools/nni_cmd/nnictl_utils.py
Outdated
if int(args.num) > len(content): | ||
print_error('Required number is too large.') | ||
return | ||
content = sorted(content, key=lambda x: float(x['data'][2: -2]), reverse=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the metric is of dict type, use 'default' key to sort.
It is for users who can not access webui on the server. I saw the demand in the NNI group in WeChat. |
tools/nni_cmd/nnictl.py
Outdated
'n trials with the highest metric') | ||
parser_experiment_head.add_argument('id', nargs='?', help='the id of experiment') | ||
parser_experiment_head.add_argument('--num', default=10, help='the number of items to be listed') | ||
parser_experiment_head.set_defaults(func=experiment_head) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can nnictl trial ls
with sorting do the same work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it, suggest to reuse nnictl trial
command since we are listing the trials.
Consider following options :
- add options to
nnictl trial ls
, such asnnictl trial ls -n 10 --sort asc --detail
nnictl trial head 10
,nnictl trial tail 10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter looks good to me. How about nnictl trial head 10 --reverse True
instead of nnictl trial tail 10
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
head 10 --reverse
maybe misunderstood as reverse sorting the the top 10, not bottom 10. The meaning of tail 10
is clear.
It can handle dict metric by keeping only the $nnictl trial head Xhc1YImB -n 5 -r
INFO: ----------------------------------------------------------------------------------------
Experiment Top Minimal Trials
Trial id: eZ5ci Default metric: 93.64
Trial id: bRVNu Default metric: 94.24
Trial id: MkYEz Default metric: 95.36
Trial id: EiL7P Default metric: 95.62
Trial id: FtMIJ Default metric: 96.49
---------------------------------------------------------------------------------------- |
I have a concern to use |
docs/en_US/Tutorial/Nnictl.md
Outdated
|
||
Note that if `--num` exceeds the number of trials, all trial results are listed. | ||
|
||
* __nnictl trial tail__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to merge tail
and head
into nnictl trial ls
. what do you think? @chicm-ms @SparkSnail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, nnictl trial ls
show all of trial results, and this nnictl trial tail
filter trial results, we can merge them to nnictl trial ls --tail {number}
to simplify nnictl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that nnictl trial ls
shows too much information and it is displayed in JSON format. Users cannot see the desired results at first glance even if trial results are sorted. Can we show the information in tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tabVersion i don't get. what do you mean?
tools/nni_cmd/nnictl.py
Outdated
@@ -102,6 +102,8 @@ def parse_args(): | |||
parser_trial_subparsers = parser_trial.add_subparsers() | |||
parser_trial_ls = parser_trial_subparsers.add_parser('ls', help='list trial jobs') | |||
parser_trial_ls.add_argument('id', nargs='?', help='the id of experiment') | |||
parser_trial_ls.add_argument('--head', type=int) | |||
parser_trial_ls.add_argument('--tail', type=int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow other commands' coding style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tools/nni_cmd/nnictl_utils.py
Outdated
@@ -259,6 +274,15 @@ def trial_ls(args): | |||
response = rest_get(trial_jobs_url(rest_port), REST_TIME_OUT) | |||
if response and check_response(response): | |||
content = json.loads(response.text) | |||
if args.head: | |||
assert int(args.head) > 0, 'The number of requested data must be greater than 0.' | |||
args.head = min(int(args.head), len(content)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use int(args.head)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't know if specify the arg's type as int, it will be converted to int.
tools/nni_cmd/nnictl_utils.py
Outdated
args.head = min(int(args.head), len(content)) | ||
content = sorted(filter(lambda x: 'finalMetricData' in x, content), | ||
key=cmp_to_key(final_metric_data_cmp), reverse=True)[:args.head] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tools/nni_cmd/nnictl_utils.py
Outdated
elif args.tail: | ||
assert int(args.tail) > 0, 'The number of requested data must be greater than 0.' | ||
args.tail = min(int(args.tail), len(content)) | ||
content = sorted(content, key=cmp_to_key(final_metric_data_cmp))[:args.tail] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no filter here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…icrosoft#2747) (cherry picked from commit 44954e0)
nnictl experiment head
nnictl experiment tail