metrics: show: debug info on parsing problems#4370
Conversation
There was a problem hiding this comment.
output might become a bit long with this I guess. I wonder would be the best to just mention that -v is available? Also, considering that -v is still needed to understand what is actually going on, I guess.
In that case we could simplify the logic a bit - boolean flag below would be enough instead of a full dict.
There was a problem hiding this comment.
we should print key here, ideally even full xpath within the file.
pared
force-pushed
the
4356_no_metrics_problem
branch
from
4bd9d3e to
dd7b062
Compare
There was a problem hiding this comment.
@jorgeorpinel could you please review the message?
There was a problem hiding this comment.
Question: is this error output per bad metrics/plots file? Or only when all of the metrics/plots files are bad?
There was a problem hiding this comment.
How about something like "The metrics/plots file could not be parsed." for now?
Not sure we need the "use -v" part. What other info specific to this problem can the user get that way?
There was a problem hiding this comment.
@jorgeorpinel this error will only be thrown on occasion when there have been metrics found, but none could be parsed, so it should be rather rare. -v will allow user to see debug messages introduced in this change.
There was a problem hiding this comment.
If we keep the message as-is, there's a typo in Could
pared
force-pushed
the
4356_no_metrics_problem
branch
2 times, most recently
from
f8739d5 to
3c962c5
Compare
There was a problem hiding this comment.
Could not parse metrics files. Use -v option to see more details.
cc @jorgeorpinel - let's get this message right
There was a problem hiding this comment.
Hey sorry guys just noticed this mention. I did suggest "The metrics/plots file could not be parsed." before in #4370 (comment) though. I don't think we need to mention -v, that applies to every error message.
There was a problem hiding this comment.
Let's not create a yet another exception type here, since it is really the same error with just a tiny difference. Let's just raise NoMetricsError with an explicit message,
pared
force-pushed
the
4356_no_metrics_problem
branch
from
3c962c5 to
3b17185
Compare
There was a problem hiding this comment.
Let's use a lazy notation, since there might be a lot of metrics present.
pared
force-pushed
the
4356_no_metrics_problem
branch
from
71000b8 to
448fa3d
Compare
5 participants
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes #4356