repo: Add ignore_revs.
#8098
repo: Add ignore_revs.
#8098
Conversation
daavoo
force-pushed
the
ignore-revs
branch
3 times, most recently
from
947b624 to
d6c90c6
Compare
| @@ -381,6 +381,7 @@ def _ignore(self): | |||
| def brancher(self, *args, **kwargs): | |||
| from dvc.repo.brancher import brancher | |||
|
|
|||
| kwargs["ignore_revs"] = self.ignore_revs | |||
| return brancher(self, *args, **kwargs) | |||
There was a problem hiding this comment.
How about
return brancher(self, *args, ignore_revs=self.ignore_revs, **kwargs)|
|
||
|
|
||
| @pytest.fixture | ||
| def broken_rev(tmp_dir, scm, dvc): |
There was a problem hiding this comment.
I don't think we need a broken rev and the whole functional test. Instead, just check that if the config file made the desired revision disappear from iter_revs with a unit test is enough.
There was a problem hiding this comment.
disappear from iter_revs with a unit test is enough
I think it's not enough in this case.
If the underlying functionality of / gc / pull/ push` is changed (i.e. brancher usage is removed) there won't be any test indicating that the ignore_revs have stopped working for those commands.
| return self.fs.path.join(self.dvc_dir, "ignore_revs") | ||
|
|
||
| @property | ||
| def ignore_revs(self): |
There was a problem hiding this comment.
We should clarify what is ignore_revs file for an external repository. Do we need to read from the default branch or just the one we checked out? If we need to read from default branch, it might not be possible with shallow clones AFAICT.
Also what should happen if user asks for that particular revision (eg: plots diff <ignored_rev>)?
There was a problem hiding this comment.
We should clarify what is ignore_revs file for an external repository.
Not sure I follow. Could you elaborate the use case?
Do we need to read from the default branch or just the one we checked out? If we need to read from default branch, it might not be possible with shallow clones AFAICT.
I believe we should read from the current revision. Treat ignore-revs as if it was part of config.
Also what should happen if user asks for that particular revision (eg: plots diff <ignored_rev>)?
We will skip the revision.
The use case is to use ignore_revs for broken revisions.
If a revision is broken, plots diff won't work (see #6150) so if we don't skip user will encounter the error?
There was a problem hiding this comment.
So regarding our yesterday discussion:
I believe that this approach is better than trying to incorporate error_handler to usual workflow. Mainly because when using ignore_revs we are making conscious decision that we know some revisions will fail and we don't care. If we were to use error_handler, it would ignore errors without this conscious decision.
Could we have a flag on how to handle failed revisions? I think having to write out a list of all failed revisions is cumbersome. |
|
@dberenbaum then it seems the best we can do is some hybrid solution - use
|
|
That could work @pared. I have no opposition to this PR, but I don't think it solves all our error handling issues or makes a general error handler unnecessary. |
|
Closing for now |
Support to write a list of line-separated revisions to
.dvc/ignore_revs.If
.dvc/ignore_revsexists, parse the revisions and skip them when usingbrancher.Allows to skip broken commits in
gc,pull, andpush.Closes #5037
Closes #5066
Closes #7585
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏