metrics: show: do not throw on no metric present on branch #1937
Conversation
dvc/repo/metrics/show.py
Outdated
There was a problem hiding this comment.
This doesn't match ENOENT error, which means that there is no such file and your "COULD NOT READ" might be interpreted as formatting issue. Formatting problems are captured in the _read_metric itself.
Also, take a look at _read_metrics_filesystem, which simply returns None if metric doesn't exist. It is crutial that they both behave the same. Should we ignore it here as well?
There was a problem hiding this comment.
I think that returning None is ok, but I think that it would be good to somehow pass information that file could not be opened. None as value can be interpreted in many ways. Though, in such case we should probably inform user upon _read_metrics_filesystem too.
tests/func/test_metrics.py
Outdated
There was a problem hiding this comment.
if you are using "COULD NOT READ" as a metric, then you better use
self.dvc.metrics.show()
which will return a dict that you could easily check without messing around with logs.
There was a problem hiding this comment.
In updated version we warn user about lack of metric file, now caplog is required
dvc/repo/metrics/show.py
Outdated
There was a problem hiding this comment.
maybe let's move it into a separate function?
dvc/repo/metrics/show.py
Outdated
There was a problem hiding this comment.
path is the cache file, so it should be Cache file '{}...`.
There was a problem hiding this comment.
Or maybe you could generalize this and what you've done down below?
pared
force-pushed
the
1917
branch
3 times, most recently
from
950ef6e to
eecfae5
Compare
tests/func/test_metrics.py
Outdated
There was a problem hiding this comment.
Pytest by default does not collect classes that have __init__()
https://docs.pytest.org/en/latest/goodpractices.html#conventions-for-python-test-discovery
There was a problem hiding this comment.
Didn't know that, thanks for clarifying! 🙂
dvc/repo/metrics/show.py
Outdated
There was a problem hiding this comment.
But in this case Metrics file '{}' does not exist at branch: '{}' doesn't really fit, since path is a cache file for metric file 🙂
There was a problem hiding this comment.
Not exactly, we pass out.relpath to warning message, which is "common part" for both use cases. We always get relative path of our out.
dvc/repo/metrics/show.py
Outdated
There was a problem hiding this comment.
Btw, it is a reoccurring issue within this whole file, but it is not always a branch that we are getting, it could be a tag as well. Should we generalize it and call it reference or something? Would that make sense?
There was a problem hiding this comment.
It is not really related to your patch, but I've just noticed it and had to mention 🙂
2 participants
Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it.
Fixes #1917