Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics: separating scalar and continuous/graphs metrics #3409

Closed
1 of 3 tasks
dmpetrov opened this issue Feb 26, 2020 · 39 comments
Closed
1 of 3 tasks

Metrics: separating scalar and continuous/graphs metrics #3409

dmpetrov opened this issue Feb 26, 2020 · 39 comments
Assignees
Labels
discussion requires active participation to reach a conclusion feature request Requesting a new feature product: VSCode Integration with VSCode extension
Milestone

Comments

@dmpetrov
Copy link
Member

dmpetrov commented Feb 26, 2020

Problem

There are two types of metrics in ML projects: scalar metrics or just a number (like AUC) and continuous metrics or a sequence of numbers (like ROC curve - an array of numbers).

Today we support csv and json files as metrics without specifying what metrics are needed. In fact, DVC commands support only scalars: dvc metrics diff.

Solution

It is important to support both, understand the semantics of the metrics and provide more value.

  1. For scalar metrics, DVC should provide (and it does provide it now) dvc metrics diff functionality. We just need clearly file format in the documentation to make sure only scalar metrics are supported. Json/Ini are both good formats for scalar metrics.
  2. For continuous metrics, DVC should provide plots/graph visualization functionality. Like dvc viz show roc.json and dvc viz diff roc.json HEAD HEAD^^. Both of the commands should generate graphs. The second command - two graphs (not diff).

Types

Types might improve visualization a lot. There many possible types of graphs for dvc viz. It would be great to support a few common ones like regular plot, confusion matrix.

For scalar types, it is also important to specify a type. A scalar can be the result of the minimization of some function or maximization. If the user can specify this information it might help to show proper color: red if max meterics was decreased and green if increased.

We should not make the metrics file format complicated. So, it might be a better solution to specify metrics format in a separate file (like Dvcfile or special metrics file).

- scalars_types:
      AUC: max
      error_1: min
- metrics:
      roc.json: plot
      error.csv: plot
      mx.json: confusion_matrix

Open questions

  1. Should we include dvc viz in the core dvc package? Problem - most likely it will bring dependencies to some graphics libraries which can cause installation issues in OS without graphics (like default EC2 instances). What options do we have?
  2. How we should specify the format of the graphs. Do we need any customization? For example, mlflow supports only one type of graph and cannot show something more advance (like a confusion matrix). If we choose Vega-graph as the format we can support graph customization.
  3. The metrics can be related to experiment parameters (see Introduce hyper parameters and config #3393) because we are introducing new DVC file types and can use a single DVC file to preserve some information about metrics or metrics types.

Actions

  • Modify DVC documentation - only scalar metrics are supported in the current format and DVC commands.
  • Implement continuous metrics with dvc viz command.
  • Add support of types.
@dmpetrov dmpetrov added feature request Requesting a new feature discussion requires active participation to reach a conclusion labels Feb 26, 2020
@efiop efiop added the product: VSCode Integration with VSCode extension label Mar 3, 2020
@efiop
Copy link
Contributor

efiop commented Mar 3, 2020

From discussion: need to delete/hide -m option for dvc run so that metrics are in git. dvc metrics add stays though.

@efiop efiop self-assigned this Mar 19, 2020
@efiop efiop removed their assignment Mar 24, 2020
@pared pared self-assigned this Mar 25, 2020
@dmpetrov
Copy link
Member Author

In order to make this scalar/continuous metrics separation, the current scalar metrics need to be redesigned.

  1. CSV/TSV types are specific to continuous metrics. There is no need to keep them in current, scalar ones. Let's get rid of CSV/TSV/HCSV/HTSV metrics format.
  2. Add support for YAML since it is an often-used format - Introduce hyper parameters and config #3393 (comment)
  3. With two simple json/yaml formats, do we really need raw format? Let's get rid of raw metrics format.
  4. Do we really need xpath in dvc-files? I see a clear use case in metrics show as a filter but not in other commands. Let's consider removing xpath.
  5. Metrics type was mostly used for distinguishing xpath algorithms between hierarchical json and flat csv/tsv/hcsv/htsv. It looks like it is not needed if only json/yaml left. Let's remove metrics type.
  6. Change docs - we should be very explicit about supporting only JSON/YAML format.
  7. [Optional] Redesign metrics show. Transform it into a table-like format.

@jorgeorpinel
Copy link
Contributor

continuous/graphs

I would call these "series", "array", or "track" metrics. Not sure about term "continuous". Just a thought

Json/Ini are both good formats for scalar metrics.

ini=YAML?

Should we include dvc viz in the core dvc package?

Probably not. DVCx then? In this case x can also mean "eXperiment mgmt".

I have doubts about the command names though. So we're keeping dvc metrics only for scalars which have an entry called scalars_types in the "Metricsfile", and new command dvc viz for series with entry metrics? I think we can improve the naming, I'll come up with a suggestion 🙂

If we choose Vega-graph as the format we can support graph customization.

Sounds smart to leave this to a viz package like thar and just refer users to their formatting guidelines.

@jorgeorpinel
Copy link
Contributor

metrics can be related to experiment parameters (see #3393) because we are introducing new DVC file types and can use a single DVC file to preserve some information about metrics

Is there a decision on what the hyperparam metafiles are going to look like? It could be relevant to know for this point ^

do we really need raw format? Let's get rid of raw metrics format

What about metrics that just spit a single number into a text file. That's not really something we want to support? (It could even be stored directly in the DVC-file in the future.)

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Apr 1, 2020

Note: point 6. in #3409 (comment) above should be automatic with our process. It will be addressed in iterative/dvc.org/pull/1097 and in the docs PR derived from addressing 2. 🙂

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Apr 1, 2020

@efiop

From discussion: need to delete/hide -m option

So we would remove --metrics as well and keep --metrics-no-cache? I like the shorter option name better. How about doing so but renaming --metrics-no-cache -> --metric (singular)?

Changing the current --metrics/-m to behave like -M would be ideal, but breaks backward compatibility.

@jorgeorpinel
Copy link
Contributor

One last comment for now, sorry:

Implement continuous metrics with dvc viz command.

Keep in mind some of the text deleted in #1097 could be restored when writing docs for dvc viz (or whatever the command name ends up being). E.g. https://github.com/iterative/dvc.org/pull/1097/files#diff-5941ad4a74935aeef222249cb407d385L72-L74

@dmpetrov
Copy link
Member Author

dmpetrov commented May 13, 2020

It seems like we agreed on most of the items and can finalize the scalar and continuous metrics change.

ToDo:

  • Introduce plots in dvc.yaml and -l/-L option in dvc run. (we need to decide on graphs/-g during the process - not a blocker) plots: add plot markers to DVC files #3807
  • Replace metrics_no_cache by flag cache: false. The same for plots. dvc.yaml: remove outs_no_cache, etc. keys, merge inside outs #3785
  • dvc metrics diff should take the default target files from dvc.yaml. Clarification: Dmitry meant that we need to support dvc.yaml, meaning that we need to collect it as well. So this is already fixed. Same with plot down below (or will be in the near future after @efiop 's PR).
  • dvc plot diff should plot all the plot files by default the same way as metrics diff does (same for plot show if we meet it - see below).
  • unify dvc plots/metrics/params show/diff to a reasonable extent. (e.g. --target vs -d)
  • -xpath was removed from metrics. it needs to be replaced by filter\selection (like --select in plot). It should support the same syntax as params (comma separated list) or --select in plots. Edit: decided to deal with it after 1.0
  • (Nice to have) Generate a single output file if there are many plots in dvc.yaml.

To discuss (we should decide after the first iteration is implemented):

  • Consider renaming plot to plots dvc: rename plot to plots #3802
  • Replace metrics and plot/graph in dvc.yaml by flag metrics: true and plot: true. Need to use top-level outs/metrics/plots with cache/persist: bool
  • Consider moving dvc plot diff under metrics umbrella dvc metrics plot (no diff) and deprecating dvc metrics show since it does the same thing as diff.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 13, 2020

Alternatively, we can use all as outputs and metrics\plot flag

On this I think maybe not because metrics/plot support files tracked by Git (not outputs).

Nice to-do list! Please note there's a corresponding docs issue here: iterative/dvc.org#1175 — its checkbox list may need updating.

@efiop efiop self-assigned this May 14, 2020
@efiop efiop mentioned this issue May 14, 2020
3 tasks
efiop added a commit to efiop/dvc that referenced this issue May 14, 2020
Follows `dvc params/metrics` convention and is needed in preparation for new
commands and some refactoring.

Part of iterative#3409
@efiop
Copy link
Contributor

efiop commented May 14, 2020

To discuss (we should decide after the first iteration is implemented):
Consider renaming plot to plots

Kinda jumped the gun here #3802 , but it hurts my OCD and will simplify the new commands and set the terminology for the rest of the points.

efiop added a commit that referenced this issue May 14, 2020
* dvc: rename plot to plots

Follows `dvc params/metrics` convention and is needed in preparation for new
commands and some refactoring.

Part of #3409

* completion: rename plot -> plots
efiop added a commit to efiop/dvc that referenced this issue May 14, 2020
More explicit CLI-like options `metrics/persist_no_cache` are still supported,
but we use flags when generating dvc.yaml.

Part of iterative#3409, prerequisite for `plot`.
efiop added a commit to efiop/dvc that referenced this issue May 19, 2020
efiop added a commit that referenced this issue May 20, 2020
* run: add --plot/--plot-no-cache

Part of #3409

* plot: move show/diff to API

* plots: show marked plots by default

Involves some refactoring to make plots comply with the rest of the code
base.

* dvc.yaml: use top-level metrics and plots

* dvc.yaml: add support for plot template

* plots: add support for templates in dvc.yaml

* dvc.yaml: allow metrics/plots to have persist/cache

* plots: render single page by-default and introduce --show-json

* plot: use "working tree" instead of "workspace"

Just to sync this with the rest of the codebase. We should consider
renaming it to "workspace" everywhere, but for now I opt to not touch
it to not break something that depends on it.

* plots: rename -f|--file to -o|--out

Better corresponds to other commands like `import`.

* plots: use RepoTree and brancher instead of api

* stage: get rid of OutputParams

* tests: plots: add simple diff test

* plots: use `--targets` instead of `--datafile`

Makes it comply with `dvc metrics/params` CLI options.

* tests: unit: plots: add --show-json test

* stage: cleanup exception for format error

Based on feedback from the users.

* lockfile: use relpath in corruption error
@dmpetrov
Copy link
Member Author

dmpetrov commented Jun 3, 2020

Should we close this issue? It looks like everything is already implemented (probably except xpath/filer) - but it s fine for now.

@efiop
Copy link
Contributor

efiop commented Jun 3, 2020

Closing in favor of #3945

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion requires active participation to reach a conclusion feature request Requesting a new feature product: VSCode Integration with VSCode extension
Projects
None yet
Development

No branches or pull requests

6 participants