metrics: show: debug info on parsing problems #4370
Conversation
dvc/exceptions.py
Outdated
| def __init__(self, metrics): | ||
| lines = "\n".join( | ||
| f"{rev}: {', '.join(str(p) for p in paths)}" | ||
| for rev, paths in metrics.items() |
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.
dvc/repo/metrics/show.py
Outdated
| if isinstance(metrics, (int, float)): | ||
| return metrics | ||
|
|
||
| if not isinstance(metrics, dict): | ||
| logger.debug( | ||
| f"Could not parse metric '{path}' at '{rev}' due to its " | ||
| f"unsupported type: '{type(metrics).__name__}'" |
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
dvc/exceptions.py
Outdated
| @@ -178,6 +178,11 @@ def __init__(self): | |||
| ) | |||
|
|
|||
|
|
|||
| class NoMetricsParsedError(DvcException): | |||
| def __init__(self,): | |||
| super().__init__("Coul not parse any metric. Use `-v` to investigate.") | |||
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
dvc/exceptions.py
Outdated
| class NoMetricsParsedError(DvcException): | ||
| def __init__(self,): | ||
| super().__init__( | ||
| "Could not parse any metric. Use `-v` to investigate." |
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.
dvc/repo/metrics/show.py
Outdated
| if metrics_found: | ||
| raise NoMetricsParsedError() | ||
| else: | ||
| raise NoMetricsError() |
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
dvc/repo/metrics/show.py
Outdated
| f"Could not parse '{key}' metric from '{path}' at '{rev}' " | ||
| f"due to its unsupported type: '{type(val).__name__}'" |
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
❗ 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