plots/metrics/params(?): stop throwing exceptions

[pared]

After writing fix for #5490 It seems to me that we should be handling exceptions thrown during plots/metrics/params differently.

For some time now we have been drifting towards approach that communicates problems with warnings instead of exceptions. And in case of plots/metrics/params commands it does make sense, as its ok for those commands to do "as much as possible" (eg, show all metrics besides that faulty one, instead of crippling whole command and not allowing user to see anything). This approach is also not ideal, every now and then I have discussion with @dmpetrov on why plots/metrics flood terminal with warnings in some cases, which reminds me of time when we used a lot of warn calls in other parts of code, and had the same problem.

I think that we should gather the exceptions preventing us from displaying particular file at particular revision and return them as part of result and let the error handling happen after doing as much as possible. Errors can still be logged to debug, and in case of dvc plots/metrics command we could issue single warning, like "10 out of 1000 plots could not be shown. Use -v to see why".

[dberenbaum]

Candidate to convert from issue -> discussion?

[dberenbaum]

We have had a couple people in Discord try to use dvc exp show to compare metrics across all commits, but it breaks because they have some old commit that throws an error, so I think this is probably higher priority now with experiments. See https://discord.com/channels/485586884165107732/563406153334128681/822468979364593704 for context.

[jorgeorpinel]

its ok for those commands to do "as much as possible"

Agree! In general commands that print any kind of report should try to recover from individual errors and still give a report with some MISSING/INVALID etc. fields ( + printing detailed warnings or non-fatal errors to stdout perhaps).

Candidate to convert from issue -> discussion?

For now (if there's consensus on the proposal) I'd suggest breaking this into several product issues, one per reporting-command (diff, metrics/plots ..., params ..., list, exp show/diff) so we can QA each one. And/or keep this one as a product epic, and add a bunch of checkbox tasks.

couple people in Discord try to use dvc exp show to compare metrics across all commits, but it breaks

Can you maybe explain how existing commits can break exp show in #5667 @dberenbaum? Please 🙂

[dberenbaum]

Can you maybe explain how existing commits can break exp show in #5667 @dberenbaum? Please 🙂

Here's the discord context:

my only issue is that the first time i ran the dvc pipeline I had an error in my metrics output json file because it had duplicate keys in the json file

For now (if there's consensus on the proposal) I'd suggest breaking this into several product issues, one per reporting-command (diff, metrics/plots ..., params ..., list, exp show/diff) so we can QA each one. And/or keep this one as a product epic, and add a bunch of checkbox tasks.

I know @skshetry is working on unifying some of the UI. Unclear on whether that would allow us to consolidate this into one issue that can be addressed across all of these commands.

@skshetry @pared What are your thoughts?

[pared]

I would be against splitting it into several issues (at least not for metrics/plots/params). Main argument against it is that our usual workflow for metrics/plots/params is that we introduce some feature/error handlings as per user request and then, in some time we unify the behaviour among other related commands. Also, if we want to keep some consistency even between dvc diff, dvc params/metrics/plots diff dvc exp diff which seems to be the aim of iterative/enhancement-proposals#4 we should probably not split this issue.

[dberenbaum]

@pared One specific use case here is where one of the commits in the diff has no dvc.lock. Can we make sure that diff doesn't throw an exception in that case?

[pared]

Ill inspect that use case and add it to solution requirements.

[pared]

@dberenbaum It seems that lack of dvc.lock should not be a problem now. I checked on current master. If you had some test repo for this use case, please check it on current master.

[pared]

Fixed by #5984