Skip to content

plots and metrics: update new targets rules #1809

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

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Conversation

pared
Copy link
Contributor

@pared pared commented Sep 25, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

Addresses #1800
Related to iterative/dvc#4590

@shcheklein shcheklein temporarily deployed to dvc-landing-4446-metric-ro9f0b September 25, 2020 08:48 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-4446-metric-ro9f0b September 25, 2020 08:51 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:35 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:36 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:39 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:41 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:42 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:44 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:45 Inactive
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.

Done here! Thanks @pared

@jorgeorpinel jorgeorpinel had a problem deploying to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:46 Failure
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:48 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-4446-metric-ro9f0b September 28, 2020 16:52 Inactive
@jorgeorpinel
Copy link
Contributor

Please merge when you deem appropriate @pared (and feel free to include iterative/dvc#4620 (review) here).

@jorgeorpinel jorgeorpinel added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Sep 28, 2020
@shcheklein shcheklein temporarily deployed to dvc-landing-4446-metric-ro9f0b October 1, 2020 08:33 Inactive
@shcheklein
Copy link
Member

@pared looks good to me! let's resolve conflicts and ship it :)

@jorgeorpinel
Copy link
Contributor

From #1809 (review):

I think if explain in the plots concept itself that plots are not only dvc.yaml based people won't expect dvc.yaml to be present in the first.

And from #1809 (review):

It's better to specify at the top level that plots files are by definition could be anywhere.

I don't think we've done this yet though @shcheklein @pared. That would imply a change in the plots cmd ref index, no?

@pared pared force-pushed the 4446_metrics branch 3 times, most recently from 8c69691 to f4e6333 Compare October 14, 2020 07:57
@pared
Copy link
Contributor Author

pared commented Oct 14, 2020

@jorgeorpinel I agree that we need to update the description of the commands, but we need to finish up here first.
I think we are pretty clear that targets don't have to be in dvc.yaml. Its mentioned both in plots/show and plots/diff. Do you think we should stress it even more?

@jorgeorpinel
Copy link
Contributor

I think we are pretty clear that targets don't have to be in dvc.yaml. Its mentioned both in plots/show and plots/diff. Do you think we should stress it even more?

True. But I thought the idea was that we wouldn't need to mention it in those or at least not stress it much as long as it was clearly expressed in the index that DVC can work with any valid plots file whether or not in dvc.yaml.

jorgeorpinel
jorgeorpinel previously approved these changes Oct 16, 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.

That said I agree we can merge this with the notes so far, although the index still needs a small update so I unlinked this from #1800

Also the note in plots show could probably be in a different place: not sure why it's part of the first paragraph, seems disconnected with the other sentences there (see #1809 (review))

@jorgeorpinel jorgeorpinel mentioned this pull request Oct 16, 2020
1 task
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.

@jorgeorpinel jorgeorpinel dismissed their stale review October 17, 2020 00:09

Pleas see my last comment ^

@pared
Copy link
Contributor Author

pared commented Oct 19, 2020

@jorgeorpinel well, in this case, plot need to be defined in dvc.yaml - if we want to modify it, we need a place to store the modification information. So, while we can attempt to show and diff pure data with templates that we already have, modifying requires us to save some info. I suppose we could support that, but as of now, we don't do that. Trying to modify non-plot will result in error.
ERROR: Unable to find DVC-file with output '{path}' We should probably make it more informative. I will create an issue for that.

@jorgeorpinel
Copy link
Contributor

while we can attempt to show and diff pure data with templates that we already have, modifying requires us to save some info. I suppose we could support that

Ah, makes sense @pared . I'm not suggesting that we support that BTW, I just didn't think it through.

Thanks

p.s. I see you created iterative/dvc/issues/4741 about the error message 👍

@jorgeorpinel jorgeorpinel merged commit e8588ff into master Oct 20, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Oct 21, 2020
@jorgeorpinel jorgeorpinel deleted the 4446_metrics branch November 9, 2020 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌛ status: wait-core-merge Waiting for related product PR merge/release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants