diff --git a/dvc/cli.py b/dvc/cli.py index 106e840ec..8f2215797 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -417,23 +417,23 @@ def parse_args(argv=None): workflow_parser = show_subparsers.add_parser( 'workflow', parents=[parent_parser], - help='Show workflow image') + help='Show workflow image. It collapses DVC repro commits if possible.') workflow_parser.add_argument( 'target', nargs='?', help='Target metric data file') + workflow_parser.add_argument( + '-d', + '--dvc-commits', + action='store_true', + default=False, + help='Show DVC repro commits.') workflow_parser.add_argument( '-a', '--all-commits', action='store_true', default=False, help='Show all commits') - workflow_parser.add_argument( - '-d', - '--deltas', - action='store_true', - default=False, - help='Show only metric deltas') workflow_parser.set_defaults(func=CmdShowWorkflow) diff --git a/dvc/command/show_workflow.py b/dvc/command/show_workflow.py index 8008939a4..37b6bdc2e 100644 --- a/dvc/command/show_workflow.py +++ b/dvc/command/show_workflow.py @@ -21,7 +21,7 @@ class CmdShowWorkflow(CmdBase): Logger.debug(u'Set show workflow target as {}'.format(target)) wf = self.git.get_all_commits(target, self.settings) - wf.build_graph(self.settings.parsed_args.all_commits, self.settings.parsed_args.deltas) + wf.build_graph(self.settings.parsed_args.dvc_commits, self.settings.parsed_args.all_commits) return 0 @property diff --git a/dvc/graph/commit.py b/dvc/graph/commit.py index e22d2d56a..754c3bad3 100644 --- a/dvc/graph/commit.py +++ b/dvc/graph/commit.py @@ -41,20 +41,17 @@ class Commit(object): def text(self): branch_text = '' if self._branch_tips: - branch_text = 'Branch tips: {}\n'.format(', '.join(self._branch_tips)) + branch_text = 'BRANCH TIPS: {}\n'.format(', '.join(self._branch_tips)) - # text = self._text_comment(self.hash, self._comment, self._is_collapsed) - text = '' - return self._text_metrics_line() + branch_text + text + self._text_added_comments + return self._text_metrics_line() + branch_text + self._comments_text - def _text_hash(self): - return 'Commit: ' + self.hash - - # @property @staticmethod - def _text_comment(hash, comment, is_collapsed): - text = Commit.COLLAPSED_TEXT if is_collapsed else Commit._limit_text(comment) - return '{}: {}'.format(hash, text) + def _text_comment(commit): + if commit._is_collapsed: + text = Commit.COLLAPSED_TEXT + else: + text = Commit._limit_text(commit._comment) + return '[{}] {}'.format(commit.hash, text) @staticmethod def _limit_text(comment): @@ -64,12 +61,17 @@ class Commit(object): return comment[:Commit.TEXT_LIMIT-3] + '...' @property - def _text_added_comments(self): - res = [self._text_comment(self.hash, self._comment, self._is_collapsed)] + def _comments_text(self): + res = [] + if not self.is_repro: + res.append(self._text_comment(self)) for commit in self._collapsed_commits: - text = self._text_comment(commit.hash, commit._comment, commit.is_repro) - res.append(text) + if not commit.is_repro: + text = self._text_comment(commit) + res.append(text) + if len(res) == 0: + return self._text_comment(self) max_len = max(map(lambda x: len(x), res)) res_extended_len = map(lambda x: x.ljust(max_len), res) @@ -78,7 +80,7 @@ class Commit(object): def _text_metrics_line(self): result = '' if self._is_target and self._target_metric: - result = 'Target: {}'.format(self._target_metric) + result = 'TARGET: {}'.format(self._target_metric) if self._target_metric_delta is not None: result += ' ({:+f})'.format(self._target_metric_delta) result += '\n' diff --git a/dvc/graph/workflow.py b/dvc/graph/workflow.py index 4b0bb1914..2fd6d4ab2 100644 --- a/dvc/graph/workflow.py +++ b/dvc/graph/workflow.py @@ -36,9 +36,9 @@ class CommitCollapseStrategy(object): child.add_branch_tips(commit.branch_tips) -class CollapseDvcCommitsStrategy(CommitCollapseStrategy): +class CollapseDvcReproCommitsStrategy(CommitCollapseStrategy): def __init__(self, workflow): - super(CollapseDvcCommitsStrategy, self).__init__(workflow) + super(CollapseDvcReproCommitsStrategy, self).__init__(workflow) def is_change_needed(self, commit, hash): return commit.is_repro @@ -57,7 +57,7 @@ class CollapseNotMeticsCommitsStrategy(CommitCollapseStrategy): def is_remove_needed(self, commit): child_commits = self._workflow.child_commits(commit.hash) - return len(child_commits) == 1 and child_commits[0].has_target_metric + return len(child_commits) == 1 and not commit.has_target_metric def upstream_to_child(self, commit, child): super(CollapseNotMeticsCommitsStrategy, self).upstream_to_child(commit, child) @@ -109,19 +109,15 @@ class Workflow(object): return [] return [self._commits[h] for h in self._back_edges[hash]] - def build_graph(self, all_commits, deltas): + def build_graph(self, show_dvc_commits, show_all_commits): g = nx.DiGraph(name='DVC Workflow', directed=False) self.derive_target_metric_deltas() - # if not all_commits: - # self.collapse_commits(CollapseDvcCommitsStrategy(self)) - # elif deltas: - # self.collapse_commits(CollapseNotMeticsCommitsStrategy(self)) - # self.deltas_only() - - self.collapse_commits(CollapseDvcCommitsStrategy(self)) - self.collapse_commits(CollapseNotMeticsCommitsStrategy(self)) + if not show_all_commits: + self.collapse_commits(CollapseDvcReproCommitsStrategy(self)) + if not show_dvc_commits: + self.collapse_commits(CollapseNotMeticsCommitsStrategy(self)) for hash in set(self._edges.keys() + self._back_edges.keys()): commit = self._commits[hash] @@ -163,10 +159,6 @@ class Workflow(object): self._build_graph(next, g) - def deltas_only(self): - - pass - def collapse_commits(self, strategy): hashes_to_remove = [] for commit in self._commits.values(): @@ -176,15 +168,17 @@ class Workflow(object): for hash in hashes_to_remove: del self._commits[hash] - del self._edges[hash] - del self._back_edges[hash] + if hash in self._edges: + del self._edges[hash] + if hash in self._back_edges: + del self._back_edges[hash] pass def _remove_or_collapse(self, commit, strategy): parent_commit_hashes = self._back_edges.get(commit.hash) child_commit_hashes = self._edges.get(commit.hash) - if parent_commit_hashes is None or child_commit_hashes is None: + if child_commit_hashes is None: return False if strategy.is_remove_needed(commit): @@ -203,20 +197,13 @@ class Workflow(object): return False pass - # def _upstream_metrics_and_branch_tips(self, child_commit_hashes, commit): - # for hash in child_commit_hashes: - # if commit.has_target_metric: - # if not self._commits[hash].has_target_metric: - # self._commits[hash].set_target_metric(commit.target_metric) - # if commit.branch_tips: - # self._commits[hash].add_branch_tips(commit.branch_tips) - # pass - @staticmethod def _redirect_edges(edges, child_commit_hashes, hash, commit_hashes): - for h in commit_hashes: - edges[h].remove(hash) - edges[h] |= child_commit_hashes + if commit_hashes: + for h in commit_hashes: + edges[h].remove(hash) + if child_commit_hashes: + edges[h] |= child_commit_hashes def derive_target_metric_deltas(self): if not self._target: diff --git a/tests/test_workflow.py b/tests/test_workflow.py index a9a6f0aa5..db0997745 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -1,6 +1,7 @@ from unittest import TestCase -from dvc.graph.workflow import Workflow +from dvc.graph.workflow import Workflow,\ + CollapseDvcReproCommitsStrategy, CollapseNotMeticsCommitsStrategy from dvc.graph.commit import Commit @@ -11,6 +12,13 @@ class TestWorkflow(TestCase): self._commit2 = Commit('2', '1', 'name1', 'today', 'DVC repro-run ...') self._commit1 = Commit('1', '', 'name1', 'today', 'comment1') + self._showed_text1 = '[' + self._commit1.hash + '] ' + self._commit1._comment + self._showed_text2 = '[' + self._commit2.hash + '] ' + self._commit2._comment + self._showed_text3 = '[' + self._commit3.hash + '] ' + self._commit3._comment + self._showed_text4 = '[' + self._commit4.hash + '] ' + self._commit4._comment + + self._showed_text3_collapsed = '[' + self._commit3.hash + '] ' + Commit.COLLAPSED_TEXT + def commits_basic_test(self): self.assertFalse(self._commit1.is_repro) self.assertTrue(self._commit2.is_repro) @@ -27,10 +35,10 @@ class TestWorkflow(TestCase): self.assertEqual(len(wf._commits), 4) - self.assertEqual(wf._commits['1'].text, self._commit1._comment + '\n' + self._commit1.hash) - self.assertEqual(wf._commits['2'].text, self._commit2._comment + '\n' + self._commit2.hash) - self.assertEqual(wf._commits['3'].text, self._commit3._comment + '\n' + self._commit3.hash) - self.assertEqual(wf._commits['4'].text, self._commit4._comment + '\n' + self._commit4.hash) + self.assertEqual(wf._commits['1'].text, self._showed_text1) + self.assertEqual(wf._commits['2'].text, self._showed_text2) + self.assertEqual(wf._commits['3'].text, self._showed_text3) + self.assertEqual(wf._commits['4'].text, self._showed_text4) pass def collapse_test(self): @@ -40,11 +48,11 @@ class TestWorkflow(TestCase): wf.add_commit(self._commit3) wf.add_commit(self._commit4) - wf.collapse_commits() + wf.collapse_commits(CollapseDvcReproCommitsStrategy(wf)) self.assertEqual(len(wf._commits), 3) - self.assertEqual(wf._commits[self._commit1.hash].text, self._commit1._comment + '\n' + self._commit1.hash) - self.assertEqual(wf._commits[self._commit3.hash].text, Commit.COLLAPSED_TEXT) + self.assertEqual(wf._commits[self._commit1.hash].text, self._showed_text1) + self.assertEqual(wf._commits[self._commit3.hash].text, self._showed_text3_collapsed) self.assertTrue('2' not in wf._commits) self.assertFalse('2' in wf._edges) @@ -58,29 +66,53 @@ class TestWorkflow(TestCase): wf.add_commit(self._commit3) # Dead end which cannot be collapsed self.assertEqual(len(wf._commits), 3) - wf.collapse_commits() + wf.collapse_commits(CollapseDvcReproCommitsStrategy(wf)) self.assertEqual(len(wf._commits), 2) - self.assertEqual(wf._commits[self._commit1.hash].text, self._commit1._comment + '\n' + self._commit1.hash) - self.assertEqual(wf._commits[self._commit3.hash].text, Commit.COLLAPSED_TEXT) + self.assertEqual(wf._commits[self._commit1.hash].text, self._showed_text1) + self.assertEqual(wf._commits[self._commit3.hash].text, self._showed_text3) self.assertTrue('2' not in wf._commits) pass def collapse_metric_commit_test(self): value = 0.812345 branches = ['master', 'try_smth'] - metric_commit3 = Commit('2', '1', 'name1', 'today', 'DVC repro-run ...', + metric_commit2 = Commit('2', '1', 'name1', 'today', 'DVC repro-run ...', True, value, branch_tips=branches) wf = Workflow('', '') wf.add_commit(self._commit1) - wf.add_commit(metric_commit3) + wf.add_commit(metric_commit2) wf.add_commit(self._commit3) self.assertEqual(len(wf._commits), 3) - wf.collapse_commits() + wf.collapse_commits(CollapseDvcReproCommitsStrategy(wf)) self.assertEqual(len(wf._commits), 2) self.assertEqual(wf._commits['3']._target_metric, value) self.assertEqual(wf._commits['3'].branch_tips, branches) - pass \ No newline at end of file + pass + + def collapse_all_commits_test(self): + value = 0.812345 + branches = ['master', 'try_smth'] + not_metric_commit2 = Commit('2', '1', 'name1', 'today', 'change smth 2...', + True, branch_tips=branches) + metric_commit3 = Commit('3', '2', 'name3', 'today3', 'hello3', + True, value) + + wf = Workflow('', '') + wf.add_commit(self._commit1) + wf.add_commit(not_metric_commit2) + wf.add_commit(metric_commit3) + wf.add_commit(self._commit4) + + self.assertEqual(len(wf._commits), 4) + wf.collapse_commits(CollapseDvcReproCommitsStrategy(wf)) + self.assertEqual(len(wf._commits), 4) + wf.collapse_commits(CollapseNotMeticsCommitsStrategy(wf)) + self.assertEqual(len(wf._commits), 2) + + self.assertEqual(wf._commits['3']._target_metric, value) + self.assertEqual(wf._commits['3'].branch_tips, branches) + pass