brancher: make brancher part of scm #4562
Conversation
|
@Suor does the viewer interact with brancher in any way? In this change I intended to unify the branches behaviour and return "workspace" instead of ""/"workspace" depending on repository status, not sure if that won't interfere with something at |
| @@ -17,23 +17,25 @@ def test_show_simple(tmp_dir, dvc, run_copy_metrics): | |||
| run_copy_metrics( | |||
| "metrics_t.yaml", "metrics.yaml", metrics=["metrics.yaml"], | |||
| ) | |||
| assert dvc.metrics.show() == {"": {"metrics.yaml": 1.1}} | |||
| assert dvc.metrics.show() == {"workspace": {"metrics.yaml": 1.1}} | |||
There was a problem hiding this comment.
This is a breaking API change, people might rely on this, so normally we can't just break it like that, unless we have a very good reason.
| all_tags=False, | ||
| all_commits=False, | ||
| ): # pylint: disable=no-self-use, unused-argument | ||
| yield "workspace", LocalTree( |
There was a problem hiding this comment.
Do we need to return workspace from here? It looks like it makes mess, i.e. we need to skip it later.
There was a problem hiding this comment.
Also LocalTree has absolutely nothing to do with Base SCM
|
Another thing is a branch or tag might be named workspace. In this case on metric show that branch is indistinguishable from actual workspace. |
|
Seems like starting from redesigning the brancher was wrong idea. Not only it would break the current functionality, but also the end result could be achieved in other way. Closing for now. |
❗ 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. 🙏
Prerequisite for #4446
In order to make plots/metrics/params
diffaccept any viable target, we need to make brancher repo agnostic. Considering thatdiffmakes sense only in case of some kind ofscm, it makes sense to make brancher part ofscm.