plots diff: don't make one plot break all the others

[dberenbaum]

In treeverse/vscode-dvc#3222, one mis-configured plot for one revision breaks the entire dvc plots diff command. in #5984, we made it so that an invalid dvc.yaml doesn't break other revisions in dvc plots diff. However, even if the dvc.yaml is valid, a single revision or plot within one revision can break dvc plots diff if there are other errors encountered while rendering one of the plots.

To do:

1. Skip revisions if any errors are encountered rendering plots for that revision.
2. Skip only the specific plots within a revision that fail to render.

Related: #7787

Edit: The problem occurs because the workspace dvc.yaml config is applied to all other revisions, and if the data structure has changed, it may be impossible to render an old revision with the current dvc.yaml config.

[daavoo]

the workspace dvc.yaml config is applied to all other revisions,

That doesn't sound right

[dberenbaum]

the workspace dvc.yaml config is applied to all other revisions,

That doesn't sound right

I think it's intentional -- in the happy path where the underlying data structure doesn't change, it's common to want to propagate a new axis label or template to all prior revisions.

[skshetry]

We merge all of the plot props, so it's not just limited to workspace dvc.yaml file it seems.
https://github.com/iterative/dvc/blob/37761a2627c1e41c35c03e55cd5284d2f4f1386d/dvc/render/match.py#L19

[dberenbaum]

So it merges all revisions into a single set of properties, preferring the more recent revisions when they conflict, right?

[skshetry]

It does not merge in terms of recency, but in the order of revisions passed, so the revision on the left side is preferred (except workspace where it always gets higher priority).

I guess the arguments are usually passed in the order of latest to oldest, so in that case, it will prefer recent revisions.

[dberenbaum]

@pmrowla mentioned that it was discussed whether we should be merging all the plots properties. I looked through some of the old issues and PRs and can't find a good rationale for why we do this.

It does seem like it would be much simpler to use a single set of properties (either the leftmost revision or workspace in cases where it's implied). Seems like that would resolve some related issues like #9188 (since it would not try to render the old custom template) and #7193.

Edit: Meant to reference #7913, not #7193.

[daavoo]

It does seem like it would be much simpler to use a single set of properties (either the leftmost revision or workspace in cases where it's implied). Seems like that would resolve some related issues like #9188 (since it would not try to render the old custom template) and #7193.

I would say, let's agree on expected behavior and implement it.

---

To clarify, when I said:

the workspace dvc.yaml config is applied to all other revisions,
That doesn't sound right

I meant that it didn't sound like what I thought the code was doing (because of #9025 (comment)), not that I was strongly against the idea of having a single source of truth for properties.

[mattseddon]

This comment is also related to #8786.

The extension caches revision data (images + datapoints) which means that we should only call plots diff once for each revision (except workspace). It is looking less likely that this approach (caching on the extension side) actually works well. For example, after reading this from #9025 (comment)

It does seem like it would be much simpler to use a single set of properties (either the leftmost revision or workspace in cases where it's implied). Seems like that would resolve some related issues like #9188 (since it would not try to render the old custom template) and #7193.

It seems like revision data is actually mutable depending on the combination of revisions passed to plots diff. This becomes even more error-prone when we throw FileNotFound errors into the mix and whether or not these errors are important/should be displayed. In order to cache data on the VS Code side we need revisions to be static.

I would say, let's agree on expected behavior and implement it.

So after a fair bit of deliberation triggered by the ongoing work with errors I think that the extension should only attempt to display templates/images that are available in the workspace's dvc.yamls. Everything else should be disregarded. Old(er) revision should be applied to the current workspace templates and that's it. The user won't lose much but this reduces/limits complexity greatly.

Even doing this we will still get edge cases where the workspace's dvc.yamls get updated and we have to "refresh" all of the cached revisions on the extension side.

If that means every time we call plots diff we call it with the workspace as the leftmost argument then no worries.

Ideally, we would get plots diff performance to a place where we can call it with up to 7 revisions and it returns in a timely enough manner to drop all of the caching (mess) that is currently in the extension (will mention this in #8786 too).

Please LMK what you think.

[pmrowla]

either the leftmost revision or workspace in cases where it's implied

I would say that even when workspace is not implied it still makes sense to be using the workspace config (and not the leftmost revision). If we are using the leftmost revision's config, the user can never modify plots unless they also include the workspace data. Even if the user adds a new template, or modifies/fixes something in an existing template, they would not be able to populate it with purely historical data (they would have to also include workspace data).