Brancher: error handling

[pared]

In #5984 I am implementing behaviour that intends to prevent some commands (plots/metrics/params/exp show) from throwing faced exceptions, and rather put them into debug/trace log. Main reasoning for that is that those commands are more useful trying to do as much as possible (eg show the metrics from non-failing branches and/or files) and errors that might occur during parsing different revisions and paths are not likely to break anything in our repo.

However it seems that skipping some errors could be useful not only for mentioned commands. For example:

• Add context about failing branches during error in used_cache() #5038 is a result of error when using dvc gc -A and one commit having malformed dvc.yaml

It seems that any command utilizing the brancher will suffer from a this problem. When there is some error for given revision (for sake of example lets assume malformed .dvc or dvc.yaml file in some historical commit), command using brancher will fail to finish the execution.

After discussions in #5984 and #5038 it seems the best way would be to make brancher provide onerror functionality.

The range of influenced commands I see so far:

• plots
• params
• metrics
• exp : show, gc, diff
• diff
• pull
• push
• fetch
• gc

While in case of plots/params/metrics/exp show it seems like we could safely ignore the errors, because those are operations that do not edit the repository, in case of others gc (mainly, but pull, push, fetch too) ignoring might not be the option. (Yet we have some ideas how to handle that: #5037 or #5066).

As we are starting to see this problem from few different issues we need to discuss whether providing onerror capabilities would let us close all of them in long term. From my perspective it looks like both #5037 and #5066 could be solved with proper onerror implementation, no matter which one we will choose as a fix. @skshetry what do you think?
As for #5984 the idea seems to work well, but as of today its implemented in a way non-generic way, avialable only to plots/params/exp show/metrics. I think we could make this change the first one to handle brancher errors, and keep behavior for other commands as it used to be. If this works out we could schedule #5037 and #5066 to fix in upcoming sprints.

TLDR:
Do we want to implement onerror handling for brancher?

cc @efiop @dberenbaum

[skshetry]

@pared, AFAICT brancher is just a generator, so it's not where things fail, so it might be tricky to implement there.

I have been thinking of Index for quite some time now (Index could be considered as a collection of stages of a single commit or workspace). It might not be quite intuitive, but it is another alternative that I see.

Please see patch (please ignore the WorkTree for now 😄) and example usage. Let me know what you think.

[pared]

So, if I understand this correctly, the essence of this propsal is to separate revision traversing capabilities from repo, right?
Now when we iterate over brancher, we deal with same repo, and change underlying fs when yielding particular revision. Index could potentially allow us to handle each revision individually, so not only we could catch and handle errors per rev but actually parallelize all commands using brancher?

[skshetry]

the essence of this propsal is to separate revision traversing capabilities

Your understanding is correct, but to be nitpicky, I would not say "revision traversing" as it is already provided by LocalFS and GitFS. The Index could be considered as the Repo's state at one point in time.

Now when we iterate over brancher, we deal with same repo, and change underlying fs when yielding particular revision. Index could potentially allow us to handle each revision individually, so not only we could catch and handle errors per rev but actually parallelize all commands using brancher?

yeah, I am quite surprised that we never had any issues with changing the fs (it's easy to miss and keep an old reference). And, we might be able to collect stages/metrics, etc. concurrently with a newer (fat) abstraction. This is quite opposite to simple brancher, but it seems that we have outgrown it and need more controls.

separate capabilities frome repo

But you are right about trying to separate things from repo. When I was thinking of reducing coupling between business logic and UI, I noticed that:

1. Repo being a god-object, was making it hard to separate and abstract.
2. The bifurcation of each of the Repo's API into Stages and then to the Output and Dependency is making it harder to parallelize (as we only parallelize objects) and reduce coupling between presentational logic and business logic (each API needs to create their own nested progress bar for example).
   For example, in checkout, you only care about "objects" or outputs, but right now you need to go through repo.checkout -> stage.checkout -> out.checkout.

So, the conclusion that I came to was that we should split Repo as much as possible into smaller blocks and try to reduce hierarchy.

This way we might be able to push the IO to the edge as much as possible. 🙂 The same might be the case with error handling.

[pared]

I like this idea. Brancher has been doing its purpose but every now and then we have been discussing about improving it. I wonder whether we should be implementing it before handling #5984. It seems intuitive to do that, though the implementation of index will be probably a bigger issue than it. Also I guess we need to discuss it with whole team because this change will have impact on every part of DVC that uses brancher.

[dberenbaum]

Can we address #5984 first and then revisit, especially since there's an implementation started already? It would help solve the error handling for a lot of use cases, and it would help define the UI and maybe clarify other issues. Also, this discussion raises additional questions about how best to generalize and how to handle errors for commands that aren't read-only.

[pared]

@dberenbaum yes, I am currently in process of polishing #5984, there are too many things to do here to handle it all in one issue.

[mnrozhkov]

There are use-cases when we need to run dvc push --all-commits and this command fails due to a syntax error in dvc.yaml in previous commits.

'./dvc.yaml' is invalid.                                                                                                                                                                                   
...                                                                                                                                                                                                  
Could not find expected ':', in line 5, column 5                                                                                                                                                           
  5 │   --input_dataset_dir datasets/name  `

If there is either:
a) a way to skip past broken commits or
b) have the broken commit hash listed out when it fails so that I can find it more easily?

[skshetry]

I have one proposal for skipping commits: #5066, the other is skipping broken commits by default (somewhat related #5037).