Skip to content

Get rid of CSV/TSV metrics#1097

Merged
dmpetrov merged 8 commits into
masterfrom
remove_csv_metrics
Apr 7, 2020
Merged

Get rid of CSV/TSV metrics#1097
dmpetrov merged 8 commits into
masterfrom
remove_csv_metrics

Conversation

@dmpetrov
Copy link
Copy Markdown
Contributor

@dmpetrov dmpetrov commented Mar 28, 2020

Getting rid of TSV/CSV metrics formats. Part of treeverse/dvc#3409 (comment):

  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.

@dmpetrov dmpetrov requested a review from jorgeorpinel March 28, 2020 13:28
@shcheklein shcheklein temporarily deployed to dvc-landing-remove-csv--ml2br1 March 28, 2020 13:28 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-remove-csv--ml2br1 March 28, 2020 13:42 Inactive
Comment thread content/docs/command-reference/metrics/add.md
Comment on lines +23 to +26
While any text file can be tracked as a metric file, we recommend using JSON
formats. DVC provides a way to parse this formats to get to a specific value, if
the file contains multiple metrics. See the [options](#options) below and
`dvc metrics diff` for more info.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
While any text file can be tracked as a metric file, we recommend using JSON
formats. DVC provides a way to parse this formats to get to a specific value, if
the file contains multiple metrics. See the [options](#options) below and
`dvc metrics diff` for more info.
While any text file can be tracked as a metric file, we recommend using JSON
format. DVC provides a way to parse this format to get to a specific value, if
the file contains multiple metric values. See the [options](#options) below,
`dvc metrics show`, and `dvc metrics diff` for more info.

Notice I added dvc metrics show to the list which I understand is the only subcommand we're sure should keep the ---xpath option. Is this correct?

Comment thread content/docs/command-reference/metrics/add.md Outdated
Comment thread content/docs/command-reference/metrics/add.md
Comment thread content/docs/command-reference/metrics/diff.md Outdated
Comment thread content/docs/command-reference/metrics/diff.md
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-remove-csv--ml2br1 April 1, 2020 07:53 Inactive
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-remove-csv--ml2br1 April 1, 2020 08:08 Inactive
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/modify.md Outdated
Comment thread content/docs/command-reference/metrics/show.md Outdated
Comment thread content/docs/command-reference/metrics/add.md Outdated
Comment thread content/docs/command-reference/metrics/modify.md Outdated
Comment thread content/docs/command-reference/metrics/modify.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-remove-csv--ml2br1 April 1, 2020 08:41 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-remove-csv--ml2br1 April 4, 2020 00:03 Inactive
@dmpetrov
Copy link
Copy Markdown
Contributor Author

dmpetrov commented Apr 4, 2020

@jorgeorpinel I've made another commit with all the changes. PLease take a look.

@dmpetrov dmpetrov requested a review from jorgeorpinel April 4, 2020 00:05
Comment on lines +34 to +37
values are: `raw` (default), `json`. It will be saved into the corresponding
DVC-file, and used by `dvc metrics show` to determine how to handle displaying
metrics. `raw` means that no additional parsing is applied, and `--xpath` is
ignored.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see raw mentioned in a few places. I thought we are getting rid of it? Per #1097 (comment)

Comment thread content/docs/command-reference/metrics/add.md
@jorgeorpinel jorgeorpinel mentioned this pull request Apr 7, 2020
5 tasks
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-remove-csv--ml2br1 April 7, 2020 08:19 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-remove-csv--ml2br1 April 7, 2020 08:42 Inactive
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more thing (about raw metrics) and also #1097 (review) wasn't addressed, everything else is solved so I'm giving approval but please lmk if you'd like me to check again!

@dmpetrov dmpetrov merged commit 8c910f7 into master Apr 7, 2020
@dmpetrov dmpetrov deleted the remove_csv_metrics branch April 7, 2020 15:02
jorgeorpinel added a commit that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants