Skip to content
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

Confine collection errors at lower level possible #6042

Closed
Suor opened this issue May 21, 2021 · 4 comments
Closed

Confine collection errors at lower level possible #6042

Suor opened this issue May 21, 2021 · 4 comments

Comments

@Suor
Copy link
Contributor

Suor commented May 21, 2021

There are situations when a DVC repo may be broken, sometimes retroactively, i.e. old commits are broken by new version of DVC. In this situation DVC simply refuses to work when we try to do anything with a broken rev. For many operations, however, we don't need things to be completely correct, i.e. we may still do dvc metrics show when stage.cmd has a broken interpolation lookup like here, we can even show git tracked things in the absense of a lock file like here. This could be used to make DVC do best effort but also is especially important for handling errors in Studio.

There is a draft PR #5984, which seeks to confine this to a single revision, i.e. dvc exp show will show one line as errored out but correctly display the other ones. This is a good start, but not granular as it might be. Ideally one needs a way to stop error propagation on key level, out level, stage level and file level so that the rest of the repo will be usable. Of cause "usable" is defined by task:

  • to show params we only need params files be present,
  • to show metrics we need those metrics be referenced from dvc.yaml and hence certain keys readable (but not all of the file)
  • to show cache stored metrics we also need checksums, i.e. we need dvc.lock file

To make this error handling appropriate for each task we need to make it customizable, i.e. having an error handler on all of the aforementioned levels and provide it a way to decide how to proceed.

@skshetry
Copy link
Member

skshetry commented May 28, 2021

The way to provide error handling at this granular level might be tricky. I wonder if we can solve this architecturally by detaching ParamDeps/outputs/metrics, etc. at the repo level.

As an example, we could build dependencies/Output and track them separately, and add them to the stage later on:

dep = ParamDeps.from_dict()
repo._params.append(dep)

stage = Stage(command=Command("echo foo"), deps=[dep]) 
repo.stages.append(stage)

Then, repo.metrics.show and repo.params.show could use repo._metrics and repo._params directly rather than going through repo.stages.

Fixing these in repo.stages itself might be risky as we cache them.

@Suor
Copy link
Contributor Author

Suor commented May 31, 2021

I think there might be a way to provide that without altering collection process that much. If we allow the error handler to decide how things are settled. Something like conditions and restarts maybe.

@Suor
Copy link
Contributor Author

Suor commented May 31, 2021

Handled #6082 in Studio with some monkey patches, which hints how outs might be ignored by a custom error handler:

from funcy import monkey, post_processing


@monkey(dvc.output, "_get")
def _get_out(stage, p, info=None, **options):
    try:
        return _get_out.original(stage, p, info, **options)
    except KeyError:  # See https://github.com/iterative/dvc/issues/6082
        logger.warning("Skipping out '%s' cause no remote found", p)


@monkey(dvc.dependency, "_get")
def _get_dep(stage, p, info):
    try:
        return _get_dep.original(stage, p, info)
    except KeyError:  # See https://github.com/iterative/dvc/issues/6082
        logger.warning("Skipping dep '%s' cause no remote found", p)


# Skip failed outputs and deps
removing_nones = post_processing(lambda xs: [x for x in xs if x is not None])
for func in ["load_from_pipeline", "loadd_from", "loads_from"]:
    setattr(dvc.output, func, removing_nones(getattr(dvc.output, func)))
for func in ["loadd_from", "loads_from"]:
    setattr(dvc.dependency, func, removing_nones(getattr(dvc.dependency, func)))

What I mean there are some bordering points at which we can stop errors from propagating. So it is a question of how we pass a handler down there.

@skshetry
Copy link
Member

Closing as stale.

@skshetry skshetry closed this as not planned Won't fix, can't repro, duplicate, stale May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants