Closed amcadmus closed 3 years ago
@marian-code Please take a look.
The unittest for parser_log
does not find the issue which means that the unittest should be improved.
OK, that behaviour is a really unexpected and unintuitive, I will take a look at it. EDIT: It makes sence when one looks at the code but this is not really mentioned in the docs. I will have to make some other system of passing in log level although I really liked this one as it is quite common in linux utilities and thus should be quite familiar to lot of people.
I have realized the current solution is a complete mess. For instance there is no way of setting log level to 0 because if you do not pass any -v
arguments it just defaults to 2.
I see here few possible reasonalbe solutions.
default=0
and nothing has to be changed. But downside is that by default user would not see any output unless he passes at least one -v
argument. You would need to pass -vv
to see logs that are now considered "default" - the INFO level. This is unfortuantelly not really intuitive.parser.add_argument("-l", "--log-level", choices=['DEBUG', 'INFO', 'WARNING', 'ERROR'], default="INFO")
Which is quite clear and consise but lacks the familiarity of -v
style setting as is custom for linux tools.
The third solution is a bit hack-y but preserves the current behaviour with one minor change (the -v
count is shifted so you
need one more v
for the same level as before):
parser.add_argument("-v", default=1, action="count", dest="level", help="1(-v)=ERROR, 2(-vv)=WARNING, 3(-vvv)=INFO "
"and 4(-vvvv)=DEBUG. Default is INFO")
args = parser.parse_args()
# levels: 2=ERROR, 3=WARNING, 4=INFO, 5=DEBUG
# adjust if `-v` was not passed in
if args.level == 1:
args.level = 4
args.level -= 2 # this gives results excatly as they are expected in current state i.e. levels from 0 to 3
Please let me now what you think is the best solution.
1 is very good but we may want a default behavior of log_level == 2
I do not like the logic used by solution 3
Thus I prefer the second way, it may be improved as
parser.add_argument("-l", "--log-level", choices=['DEBUG', 3, 'INFO', 2, 'WARNING', 1, 'ERROR', 0], default="INFO")
so one can quickly set the log level to for example ERROR
by
dp train -l 0 input.json
I want to discuss the determination of the
log_level
byargparse
, the code is in the following https://github.com/deepmodeling/deepmd-kit/blob/18ba5b19d8d2d237e600c6478ba8c8ba59e48d7b/source/train/main.py#L42-L50Surprisingly, the action of
"count"
is to ADD the number of flags to the default value oflog_level
. Then if one sets-vvv
, he/she will have alog_level
of5
rather than3
. To reproduce this behavoior,