exp show: fails when dvc.yaml is invalid [QA]

[jorgeorpinel]

If after running experiments you edit dvc.yaml and (accidentally) make it invalid, exp show will

ERROR: failed to show experiments - unable to read: 'dvc.yaml', YAML file structure is corrupted: while scanning a simple key

(or some other dvc.yaml-related error). Why does exp show check dvc.yaml? Does it also check the consistency of all project dvc.yaml files like status/repro/exp run? (May hurt performance)

[pmrowla]

Why does exp show check dvc.yaml?

Collecting metrics and params for a repo requires collecting stages from dvc.yaml (since we need the list of metrics outs and params dependencies for all stages in the pipeline).

I'm pretty sure this same issue isn't experiments specific and would apply to any DVC command that requires collecting stages/metrics/params.

[shcheklein]

(Note: I didn't check the current implementation, so feel free to disregard if it's already done this waY)

My 2cs on this. It reminds me the problem that we had in the viewer. What if one of the commits is broken from the DVC perspective (raises something similar to the above, for example)? Should we fail? Should we show at least other commits?

In the viewer we've decided that we parse commits (and we don't deal with workspace there) in isolation. Each commit is independent and if it breaks we move to the next one. Each commit that viewer returns in its version of the "dvc exp show" has a status - ok/failed + error message. Otherwise it was almost impossible to deal with actual repos - people break things constantly.

Also, let's keep in mind the VS code extension - it'll depend on dvc exp show and it will keep running it as we change dvc.yaml. It might happen that we'll run it when workspace is broken - we need to handle this to my mind, we still should show the table, we can assign some special flag/state to the broken commits/experiments.

[dberenbaum]

Related convo in #5525 about showing as much as possible in plots/metrics/params/exp show, although I think a broken dvc.yaml is a bit different and tougher problem than the other examples discussed in either issue. Maybe better to leave this convo in #5525 and break out smaller issues as you QA @jorgeorpinel to keep each issue focused? It's our constant struggle to try to keep issues focused on a single topic 😄 .

[jorgeorpinel]

Collecting metrics and params for a repo requires collecting stages from dvc.yaml

Makes sense. But a main use of exp show is to see past experiment details, and past exps are guaranteed to have had valid dvc.yaml files, right?

let's keep in mind the VS code extension - it'll depend on dvc exp show ... we need to handle this to my mind, we still should show the table, we can assign some special flag/state

Good idea and again, these special state would only need apply to the workspace row I think. Unless what "breaks" old commits is a DVC update.

As for the state, what about INVALID? And probably also print a warning (or non-fatal error) to stderr with the details.

Related convo in #5525 about showing as much as possible in plots/metrics/params

Thanks for the link! Def. related but that one is too broad so I vote to keep this part here for now.

[jorgeorpinel]

past exps are guaranteed to have had valid dvc.yaml files, right?

Looking at https://discord.com/channels/485586884165107732/563406153334128681/822468979364593704 (via #5525 (comment)) I realize that even when experiments should have valid pipelines (otherwise they wouldn't exist), it's when you use exp show -aTA (which includes manual commits) that there can be a bad dvc.yaml in there somewhere. So yeah this would apply for those rows too.

[pmrowla]

It's also possible (although unlikely) for a user to manually add commits to DVC experiment refs, so this is something that just needs to be accounted for in general (for any git commit and the workspace).

But yeah, it's not exactly clear how we should handle displaying state for broken dvc.yaml/dvc.lock/params/metrics

[shcheklein]

But yeah, it's not exactly clear how we should handle displaying state for broken dvc.yaml/dvc.lock/params/metrics

keep them in the table, but mark somehow? Ideally, even show the message or command how to get the error message.

interestingly enough, sometimes these errors can be recovered. E.g. recently we had a lot of commits failing due to dvc.lock/dvc.yaml being broken for a simple reason - DVC was old. I would be bad if didn't show all commits at all I think.

[dberenbaum]

keep them in the table, but mark somehow? Ideally, even show the message or command how to get the error message.

Yes, I think all errors could likely be printed above or below the table. If we can include the experiment/revision id when doing so, even better.

Maybe the table itself can include every experiment/revision but can be blank (or otherwise marked/flagged) where metrics/params weren't successfully populated.

[pared]

Fixed by #5984