metrics: accept any viable target in DvcRepo #4590
Conversation
|
@jorgeorpinel Do you think its worth mentioning in docs that from this change on |
dvc/repo/metrics/show.py
Outdated
| target_infos = [PathInfo(os.path.abspath(target)) for target in targets] | ||
| if recursive: |
There was a problem hiding this comment.
Will recursive work with unofficial metrics? I don't see a test for it and the code seems like it won't work.
There was a problem hiding this comment.
I am not sure how that would work, I mean, in that case we would need to try to show all files for particular directory, since they are not marked as metrics. So the question here is whether we want to support that.
There was a problem hiding this comment.
@pared That seems like a natural approach. You have a test with official metrics dir in this PR, what changes if they are not declared as official metrics?
There was a problem hiding this comment.
On the other hand, it kinda makes sense to have special dir for metrics and check all of them out. Ill try to modify the change.
There was a problem hiding this comment.
Also note that we try to parse metric files, so if something is not a metric we'll just ignore it and won't show it.
dvc/repo/metrics/show.py
Outdated
|
|
||
| tree = RepoTree(repo) | ||
| for target_info in target_infos: | ||
| if tree.isfile(target_info): |
There was a problem hiding this comment.
What if a target is a file and recursive==True? Will it be added to the result twice?
There was a problem hiding this comment.
This will probably become obsoleted once you implement recursive for unofficial metrics above...
pared
force-pushed
the
4446_metrics
branch
2 times, most recently
from
11d1259 to
5978287
Compare
| "branch": {"metrics.yaml": {"foo": 2.2}}, | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Missing a test for recursive directory with unofficial metrics.
There was a problem hiding this comment.
Since both official and unofficial metrics are gathered the same way now, I would argue that test_non_metric_and_recurisve_show already covers that. Anyhow, I modified it so that it checks both official and unofficial metrics.
Yes! Very much 🙂 — thanks for submitting a docs PR BTW. |
❗ 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. 🙏