pipeline: show: outs: eliminate extra edges in DAG #3217

fabiosantoscode · 2020-01-22T21:35:29Z

This fixes extra nodes edges appearing in pipeline show --outs, by using networkx to traverse the graph instead of listing all edges.

I spent a lot of time trying to figure out how to test this, so I thought I'd either leave it untested or take suggestions from the PR review.

Fixes #1744

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Suor

The idea was to connect outputs to deps, this is not changed here. The code still connects outs to outs.

fabiosantoscode · 2020-01-23T11:14:00Z

@Suor I initially looked at the issue description on Discord and tried to reproduce the same issue. This fixes that diamond problem, where A generates B and C, and B doesn't go on to be a dependency of the ultimate target but is still in the graph.

I'll try to reproduce the more general issue and see what fixes it.

fabiosantoscode · 2020-01-23T11:48:09Z

@Suor there. It's getting pretty nested and I could use itertools.product() to make those two for loops just one, but it's probably unnecessarily complicated.

fabiosantoscode · 2020-01-23T11:49:37Z

Wait, that implementation is incorrect.

fabiosantoscode · 2020-01-23T12:06:04Z

This should do it.

Suor

Maybe we need a test to clear this out.

dvc/command/pipeline.py

efiop · 2020-01-24T21:55:19Z

tests/func/test_pipeline.py

+    )
+
+    args = ["pipeline", "show", "--outs", "--dot", final_stage.path]
+    with tmp_dir.chdir():


You are already in tmp_dir, so no need to chdir. Or am I missing something here?

That is correct! I didn't know about that. Updating.

pared · 2020-01-29T11:13:38Z

tests/func/test_pipeline.py

+        final_stage.path, commands=False, outs=True
+    )
+
+    assert set(nodes) == set(["final", "derived1", "base"])


no need for converting list to set, set(["final", "derived1", "base"]) == {"final", "derived1", "base"}

pared · 2020-01-29T11:18:19Z

tests/func/test_pipeline.py

+        deps=["derived1"], outs=["final"], cmd="echo final > final"
+    )
+
+    args = ["pipeline", "show", "--outs", "--dot", final_stage.path]


If we are calling __build_graph anyway, why would we provide those args? You can simply initialize command with empty list. :)

pared · 2020-01-29T11:18:49Z

tests/func/test_pipeline.py

@@ -98,6 +99,28 @@ def test_dot_commands(self):
        self.assertEqual(ret, 0)


+def test_disconnected_stage(tmp_dir, dvc, caplog):


caplog seems to be unused

pared

Little suggestions for tests.

Another thing, not necessarily in this change: @efiop, do you think that we should keep the name of the method __build_graph? It has special meaning in python, and at the same time we do not leverage the functionality it provides (preventing method name collisions with child classes), so maybe its better to just replace it with _build_graph and access it by command._build_graph?

efiop · 2020-01-29T14:05:12Z

Another thing, not necessarily in this change: @efiop, do you think that we should keep the name of the method __build_graph

Yeah, that hurts my eyes too, it was introduced a few years back by one of the contributors and we didn't touch that since. We should change it, but not necessarily in this PR (though, might as well) 🙂

pared · 2020-01-29T15:51:29Z

@efiop that is very tactful of you, to say "one of our contributors" and not "you did that, remember?" :D
@fabiosantoscode If it's not a problem, I would suggest refactoring the name __build_graph -> _build_graph. If you mind, it can stay as is.

efiop · 2020-01-29T16:04:22Z

@efiop that is very tactful of you, to say "one of our contributors" and not "you did that, remember?"

Wait, I didn't mean that, though. I don't think it was you, was it? 👀

EDIT: oh, it was https://github.com/iterative/dvc/pull/1141/files . I didn't mean to shame anyone. But at least we are now clear that we shouldn't have done that, so that is what really matters. We've all come a long way 🙂

Suor

Some cosmetic things, but LGTM overall.

Suor · 2020-02-03T14:41:23Z

dvc/command/pipeline.py


-        return nodes, edges, networkx.is_tree(G)
+        if outs and not commands:


Aren't this two mutually exclusive?

Yes they seem to be!

fabiosantoscode · 2020-02-03T18:38:13Z

I'll remove the __ for a regular _. I agree that it's a bit uncalled for.

fabiosantoscode · 2020-02-03T18:46:19Z

I believe I've addressed all of the feedback. Additionally, I found a __write_dot method and turned it into _write_dot.

efiop

Thank you so much @fabiosantoscode ! 🙏

pipeline: show: outs: eliminate extra nodes in DAG

058e19f

fabiosantoscode requested a review from efiop January 22, 2020 21:35

Suor suggested changes Jan 23, 2020

View reviewed changes

connect deps to outs

a299fba

connect outs to deps

b635fdd

add missing nodes

d827fef

Suor suggested changes Jan 23, 2020

View reviewed changes

dvc/command/pipeline.py Outdated Show resolved Hide resolved

add test and traverse nodes when showing outs

233790c

efiop reviewed Jan 24, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Jan 26, 2020

Weekly Digest (19 January, 2020 - 26 January, 2020) #3237

Closed

remove unnecessary chdir

a7bad88

efiop requested review from Suor and pared January 28, 2020 22:21

pared reviewed Jan 29, 2020

View reviewed changes

pared suggested changes Jan 29, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Feb 2, 2020

Weekly Digest (26 January, 2020 - 2 February, 2020) #3272

Closed

Suor approved these changes Feb 3, 2020

View reviewed changes

fabiosantoscode added 2 commits February 3, 2020 18:41

remove __

11ad0a9

PR feedback

758205a

efiop approved these changes Feb 4, 2020

View reviewed changes

efiop merged commit 93d1b6d into iterative:master Feb 4, 2020

fabiosantoscode deleted the feature/1744-pipeline-eliminate-extra-connection branch February 4, 2020 00:45

weekly-digest bot mentioned this pull request Feb 9, 2020

Weekly Digest (2 February, 2020 - 9 February, 2020) #3289

Closed

efiop mentioned this pull request Feb 26, 2020

dvc pipeline show --ascii - visual bug since version 0.82.9 #3410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pipeline: show: outs: eliminate extra edges in DAG #3217

pipeline: show: outs: eliminate extra edges in DAG #3217

fabiosantoscode commented Jan 22, 2020

Suor left a comment

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

Suor left a comment

efiop Jan 24, 2020

fabiosantoscode Jan 28, 2020

pared Jan 29, 2020

pared Jan 29, 2020

pared Jan 29, 2020

pared left a comment

efiop commented Jan 29, 2020 •

edited

Loading

pared commented Jan 29, 2020 •

edited

Loading

efiop commented Jan 29, 2020 •

edited

Loading

Suor left a comment

Suor Feb 3, 2020

fabiosantoscode Feb 3, 2020

fabiosantoscode commented Feb 3, 2020

fabiosantoscode commented Feb 3, 2020

efiop left a comment

		@@ -98,6 +99,28 @@ def test_dot_commands(self):
		self.assertEqual(ret, 0)


		def test_disconnected_stage(tmp_dir, dvc, caplog):


		return nodes, edges, networkx.is_tree(G)
		if outs and not commands:

pipeline: show: outs: eliminate extra edges in DAG #3217

pipeline: show: outs: eliminate extra edges in DAG #3217

Conversation

fabiosantoscode commented Jan 22, 2020

Suor left a comment

Choose a reason for hiding this comment

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

fabiosantoscode commented Jan 23, 2020

Suor left a comment

Choose a reason for hiding this comment

efiop Jan 24, 2020

Choose a reason for hiding this comment

fabiosantoscode Jan 28, 2020

Choose a reason for hiding this comment

pared Jan 29, 2020

Choose a reason for hiding this comment

pared Jan 29, 2020

Choose a reason for hiding this comment

pared Jan 29, 2020

Choose a reason for hiding this comment

pared left a comment

Choose a reason for hiding this comment

efiop commented Jan 29, 2020 • edited Loading

pared commented Jan 29, 2020 • edited Loading

efiop commented Jan 29, 2020 • edited Loading

Suor left a comment

Choose a reason for hiding this comment

Suor Feb 3, 2020

Choose a reason for hiding this comment

fabiosantoscode Feb 3, 2020

Choose a reason for hiding this comment

fabiosantoscode commented Feb 3, 2020

fabiosantoscode commented Feb 3, 2020

efiop left a comment

Choose a reason for hiding this comment

efiop commented Jan 29, 2020 •

edited

Loading

pared commented Jan 29, 2020 •

edited

Loading

efiop commented Jan 29, 2020 •

edited

Loading