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

Refactor plots #3994

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Refactor plots #3994

merged 1 commit into from
Jun 9, 2020

Conversation

Suor
Copy link
Contributor

@Suor Suor commented Jun 9, 2020

  • separate data collection and rendering
  • provide a way to use custom template loader
  • drop hard-coding datafiles in template feature (discussed this with @dmpetrov)

Closes #3818.

- separate data collection and rendering
- provide a way to use custom template loader
- drop hard-coding datafiles in template feature
Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

LGTM

@efiop efiop merged commit 7230d87 into iterative:master Jun 9, 2020
Copy link
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.

Quick review on a couple output strings

def _plot_props(out):
if not out.plot:
raise DvcException(
f"'{out}' is not a plot. Use `dvc plots modify` to change that."
Copy link
Contributor

Choose a reason for hiding this comment

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

plots modify can change a file that is not a plot into a plot? Or maybe I don't understand this message.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it can. It's a feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thanks. What I meant is that I didn't understand what the sentence meant. Thinking about it now I think I get it: a regular output is being given to a plots command right? And you can make it into a plot with plots modify. So 2 things:

  1. I recommend something like this for message for now: "'{out}' is found in stage '{stg}' as an output but not as a plot. You can make it into a plot with `plots modify ...`
  2. Actually, what's the command to make an output into a plot with plots modify? I'd like to document this feature.
  3. Why not just try to plot the output instead and throw a formatting error if it's not valid?

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

  1. Too long, and not any useful information than the one provided already.
  2. dvc plots modify output-file-name-that-is-not-a-plot.file_ext
    You can provide any other flags on the same command, like --template too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The existing string is hard to read and understand. Can we come up with a better suggestion if mine isn't liked? "to change that" to change what?

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. The given output file is not a known plot. Use `dvc plots modify {out}` to turn it into one.

p.s. why is it bad that a message to the user is relatively long? (2 sentences)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can plots modify make ANY file into a plot, or only other outputs? What about existing metrics?
Does it validate plot file format?
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.s. why is it bad that a message to the user is relatively long? (2 sentences)

The longer the message the smaller percentage of people will read it.

Also, can plots modify make ANY file into a plot, or only other outputs?

Only outputs. In current dvc-file design only outputs may become metrics or plots. Metric might become a plot. Not sure whether it will stop being a metric.

Does it validate plot file format?

No. dvc run doesn't validate it either. Strictly speaking we don't have such thing as validation here, we can check whether loading it results in an error though.

Copy link
Contributor

Choose a reason for hiding this comment

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

The longer the message the smaller percentage of people will read it.

Arguable. But if the text is hard to understand, it doesn't matter how many people read it. I guess I'll just sent a PR with a suggested text instead of arguing here.

Thanks for the other info. TBH it seems a little obscure/ arbitrary behavior but I'm not opposed to it. I just think it would be best to re-think it. Not really my realm so I'll just ping @shcheklein and @efiop here 🙂

dvc run doesn't validate it... we can check whether loading it results in an error

Please note that coincidentally, I opened an issue about this (not related to this conversation originally): #4068

dvc/repo/plots/__init__.py Show resolved Hide resolved
jorgeorpinel added a commit that referenced this pull request Jun 25, 2020
efiop added a commit that referenced this pull request Jun 25, 2020
* plot: link to cmd ref in output

* plots: change non-plot output error msg
per #3994 (comment)

* Update dvc/repo/plots/__init__.py

Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
@skshetry skshetry mentioned this pull request Jul 18, 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.

Make plots reading and rendering API friendly
5 participants