plots/metrics: accept any viable target

[pared]

Running:

#!/bin/bash

rm -rf repo
mkdir repo

pushd repo

git init --quiet
dvc init --quiet


echo "{'metric':0.95}" >> metric.json
git add -A
git commit -am "initial"
git tag v1


echo "{'metric':0.96}" >> metric.json
git add -A
git commit -am "second"
git tag v2

dvc metrics diff v1 v2 -v --targets metric.json

yields no result. metrics diff/show should be able to handle non-metrics targets that can be interpreted by our parsers.

[efiop]

For the record: same for plots

[dmpetrov]

dvc params diff should follow the same logic. But first, --targets needs to be introduced for params.

[dmpetrov]

Metrics/plots/params difference for Git-files is a useful scenario even outside of the data-files or pipeline context. It feels like it is a separate functionality in DVC that needs to be fully supported.

For example, it might be used in CI/CD to compare metrics/plots/params with master or previous commits in a repo when DVC is not used to generate reports like this - iterative/cml_dvc_case#4

Additional scenarios to support:

1. metrics/plots/params diff for not-DVC repos (with --targets specified).
2. metrics/plots/params diff for DVC repos without any pipelines (without --targets specified). So, a dummy\phony stage might be needed.

[pared]

@dmpetrov I see here following scenarios (lets take plots as example command):

1. DvcRepo, file that is not marked as plot, but we are able to dvc plots show and dvc plots diff - in this case providing targets is required - because we need to know what we are trying to parse.

2. DvcRepo, some file is marked as plot, but not using dvc run --plots (but, for example dvc plots add {file}). Later we are able to dvc plots show/diff without targets, and dvc is able to find it by itself.

3. GitRepo, we are able to dvc plots show/diff - in this case there is no way to mark file as plot beforehand, and that's why we need to provide targets.

4. Not a repo, we are able to dvc plots show - targets required

Is there any prioritization between those scenarios?

[dmpetrov]

@pared All looks good!

Re (4). It is not a high-priority scenario. But it would be amazing to support it if not a big implementation overhead. dvc plots show makes sense for just a file outside of any repo. The diffs - probably not (we can imaging a scenario with two files diff, but cmd signature needs to be changed. it is probably do not worth it).

Re (1)&(2) - it should support data files even not plot\metrics ones.

An important thing about this issue - we need to pay attention to the UI part. All the commands should be unified including params one.

[pared]

So, starting this issue I made a mistake and wanted to start with moving and unifying collection for plots/metrics/diff into the SCM. It will be probably required at some point, thought its definitely not a good thing to start with as it is a big change.

So my plan is as follows:

• make metrics accept non metric files - metrics: accept any viable target in DvcRepo #4590
• make plots accept non plots files
• unify collection of targets for metrics/plots/diff
• support purely GitRepos
• support plot show for no repo at all

[pared]

Summary on current state of the issue, and what still needs to be done here:

• metrics accept non-metrics file - doesn't matter if we are in DVC repo, Git repo, or just a folder - targets required
• plots accept non-plots file - doesn't matter if we are in DVC repo, Git repo, or just a folder - targets required
• collection of metrics plots and params between branches is unified - we will get the same log message if given file is not found at particular revision, no matter command used

TODO:
4 scenarios of unified error handling:

• bad file format
• missing target
• missing target on other revision
• no metric/plots in repo at all

(They should be fixed, leaving this list to double check)

• introduce --targets for params command
• params accept non-params file - not matter DVC, GIT, or no repo case

[pared]

@dmpetrov
There are 2 things left here to be handled:

1. In the beginning of this issue you mentioned following use case:

metrics/plots/params diff for DVC repos without any pipelines (without --targets specified). So, a dummy\phony stage might be needed

How would that work? If we don't have pipelines we don't know which files are metrics/plots/params (with one exception, the default params.yaml file) How are we going to recognize which files in repo are metrics/plots/params?

2. Scenario 2 - when show non existing file on current revision:

result for plots:

WARNING: 'data.json' was not found at: 'workspace'.
ERROR: File: 'data.json' does not exist.

result for metrics:

WARNING: 'data.json' was not found at: ''.                            
ERROR: File: 'data.json' does not exist.

In case of metrics we display empty string, in case of plots it is workspace - It seems to me that we should unify that too. I think in both cases we should display not found at :'workspace' as no string at all is confusing. Or we could even make this "placeholder" string for current repo state even more obvious (eg current_repo).

[dmpetrov]

If we don't have pipelines we don't know which files are metrics/plots/params (with one exception, the default params.yaml file) How are we going to recognize which files in repo are metrics/plots/params?

@pared good point! By "without any pipelines" I meant without stages (that contain commands) - "dummy\phony stage might be needed".
I see this as a dummy stage with no command and deps but with params and metrics files as outputs.

In case of metrics we display empty string, in case of plots it is workspace - It seems to me that we should unify that too.

Absolutely! Do we really need the warning messages? Why not just ERROR: File 'data.json' does not exist. (without semicolumn in File:)

[pared]

Absolutely! Do we really need the warning messages? Why not just ERROR: File 'data.json' does not exist. (without semicolumn in File:)

So, the error will occur when we don't collect any other plots/params/metrics. The problem is that this warning is happening before we are done with collecting. So we need to take into account situation when we are, for example dvc plots diff 10 different rev's, and one of them does not contain given target. It doesn't make sense to error out - as we are able to produce the plot for all the other 9, but we still need to tell the user we were not able to find the target on this single rev. Hence the warning.