plots: add plot markers to DVC files by efiop · Pull Request #3807 · treeverse/dvc

efiop · 2020-05-17T00:51:56Z

❗ 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. If the CLI API is changed, I have updated tab completion scripts.

treeverse/dvc.org#1255

❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

skshetry · 2020-05-17T12:09:29Z

Let's do this in a single loop in _get_outs(). What do you say?

efiop · 2020-05-18T02:18:50Z

Just copying help from the options above. Clearly this is not great.

efiop · 2020-05-18T02:34:02Z

Will need to revisit this helper at some point. It was (and is) a bit hackish.

Suor · 2020-05-18T13:08:05Z

Do we plan to add default metric in a plot there? Current "strategy" of selecting a last key doesn't look well.

efiop · 2020-05-18T13:11:31Z

@Suor Could you elaborate?

Suor · 2020-05-18T14:05:48Z

@efiop plot data file contains several plots, i.e. values of several metrics over something. Here it's plots.csv with several columns for example. When plot diff shows the plot it selects, which column to use by selecting last column. This is not a good heuristic most of the time.

I think we should have a way to set default metric to build plot from, probably within a dvc file.

pared · 2020-05-18T14:23:53Z

@Suor
We discussed with @efiop that for starters, we should let the user point in stage whether the given metric is plot or not. You are right that the particular parameters should be included, but we should probably do that as next "stage" of this issue.

Suor · 2020-05-18T14:23:59Z

Adding to the above, there is a -s, --select option to do that on command line. That one accepts either field name or jsonpath.

Suor · 2020-05-18T14:26:33Z

@pared knowing default metric to select from plot data would be very valuable to the corresponding viewer feature. Now there is no interface to change anything, so default matters, it will matter even when some interface will be added. Will also improve cli experience.

pared · 2020-05-18T14:38:05Z

@Suor to not extend this PR scope, maybe we should create p0 for that?

Suor · 2020-05-18T15:03:17Z

@pared I have no preference on where it should go. Let @efiop decide whether he wants to bundle or separate it.

Suor · 2020-05-18T15:06:25Z

Another issue I see is that default metric (y axis) is thing across commits, but the dvc is stored within commit. So where do we get that? From master, some of the commits involved?

skshetry · 2020-05-19T05:38:28Z

Can you please point me where this is being set as dict? I couldn't find it. 😕

@skshetry It is actually not set anywhere programmatically (we might introduce dvc plots modify later, but for now it should be set by-hand).

Part of treeverse#3409

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

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.

Better corresponds to other commands like `import`.

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

Based on feedback from the users.

efiop · 2020-05-20T01:17:01Z

@Suor Great questions! For now going only with templates, but we'll definitely introduce additional options for it (like labels and stuff) later.

efiop · 2020-05-20T01:48:49Z

Ok, merging for now. Will have a followup PR.

dmpetrov mentioned this pull request May 17, 2020

Metrics: separating scalar and continuous/graphs metrics #3409

Closed

3 tasks

efiop force-pushed the 3409 branch from ef0030a to 0e80a3b Compare May 17, 2020 01:38

efiop commented May 17, 2020

View reviewed changes

Comment thread dvc/repo/plots/show.py Outdated

skshetry reviewed May 17, 2020

View reviewed changes

Comment thread dvc/stage/params.py Outdated

skshetry reviewed May 17, 2020

View reviewed changes

efiop force-pushed the 3409 branch 4 times, most recently from 0a2b430 to 4feffd0 Compare May 18, 2020 02:18

efiop commented May 18, 2020

View reviewed changes

Comment thread dvc/command/run.py Outdated

Copy link
Copy Markdown

Contributor Author

efiop May 18, 2020

Choose a reason for hiding this comment

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

Just copying help from the options above. Clearly this is not great.

efiop force-pushed the 3409 branch from 4feffd0 to eb86f35 Compare May 18, 2020 02:28

efiop commented May 18, 2020

View reviewed changes

efiop force-pushed the 3409 branch from eb86f35 to 82144d6 Compare May 18, 2020 12:17

pared self-requested a review May 18, 2020 14:39

efiop force-pushed the 3409 branch from 968af8b to aa3382f Compare May 18, 2020 14:55

pared suggested changes May 18, 2020

View reviewed changes

Comment thread dvc/command/plots.py Outdated

efiop force-pushed the 3409 branch 5 times, most recently from 4f4dbb5 to d51b288 Compare May 19, 2020 00:31

skshetry reviewed May 19, 2020

View reviewed changes

skshetry mentioned this pull request May 19, 2020

A notion of plot #3826

Closed

Ruslan Kuprieiev added 5 commits May 19, 2020 23:40

run: add --plot/--plot-no-cache

075334e

Part of treeverse#3409

plot: move show/diff to API

e21f8a7

plots: show marked plots by default

9498de0

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

dvc.yaml: use top-level metrics and plots

c58ba4e

dvc.yaml: add support for plot template

61262a4

efiop force-pushed the 3409 branch from d51b288 to e3afc0a Compare May 19, 2020 20:40

Ruslan Kuprieiev added 3 commits May 20, 2020 00:32

plots: add support for templates in dvc.yaml

0875133

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

214628e

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

b6765cd

efiop force-pushed the 3409 branch 2 times, most recently from 77be08a to cd43bcc Compare May 19, 2020 21:57

Ruslan Kuprieiev added 5 commits May 20, 2020 02:11

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

90510ab

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

a470da3

Better corresponds to other commands like `import`.

plots: use RepoTree and brancher instead of api

0ecc8ea

stage: get rid of OutputParams

238946a

tests: plots: add simple diff test

525134f

efiop force-pushed the 3409 branch from 7ba9c52 to 525134f Compare May 19, 2020 23:11

efiop changed the title ~~[WIP] plots: add plot markers to DVC files~~ plots: add plot markers to DVC files May 19, 2020

efiop changed the title ~~plots: add plot markers to DVC files~~ [WIP] plots: add plot markers to DVC files May 19, 2020

Ruslan Kuprieiev added 4 commits May 20, 2020 03:14

plots: use --targets instead of --datafile

027c94e

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

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

c70b580

stage: cleanup exception for format error

402d2c7

Based on feedback from the users.

lockfile: use relpath in corruption error

419d70f

efiop changed the title ~~[WIP] plots: add plot markers to DVC files~~ plots: add plot markers to DVC files May 20, 2020

efiop merged commit 09136f2 into treeverse:master May 20, 2020

efiop deleted the 3409 branch May 20, 2020 01:49

Conversation

efiop commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skshetry May 17, 2020

Choose a reason for hiding this comment

Uh oh!

efiop May 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efiop May 18, 2020

Choose a reason for hiding this comment

Uh oh!

Suor commented May 18, 2020

Uh oh!

efiop commented May 18, 2020

Uh oh!

Suor commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pared commented May 18, 2020

Uh oh!

Suor commented May 18, 2020

Uh oh!

Suor commented May 18, 2020

Uh oh!

pared commented May 18, 2020

Uh oh!

Suor commented May 18, 2020

Uh oh!

Suor commented May 18, 2020

Uh oh!

Uh oh!

skshetry May 19, 2020

Choose a reason for hiding this comment

Uh oh!

efiop May 19, 2020

Choose a reason for hiding this comment

Uh oh!

efiop commented May 20, 2020

Uh oh!

efiop commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

efiop commented May 17, 2020 •

edited

Loading

Suor commented May 18, 2020 •

edited

Loading