FIX: Node repr and detailed graph expansion #2669

djarecka · 2018-08-02T04:22:30Z

Summary

fixing the detailed graph and printing workflow.run().nodes() for workflows with iterables, the issue was due to the changes in the node __str__/__repr__

Fixes #2668

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

djarecka · 2018-08-02T12:32:09Z

@oesteban - yesterday with @satra we were fixing the graph plotting, but Travis is failing for test_deep_nested_write_graph_runs, since Workflow doesn't have itername. I've noticed that in #2376 you moved itername, _id, from EngineBase to Node, so Workflow doesn't have it. Do you remember what was the motivation?

codecov-io · 2018-08-02T12:57:08Z

Codecov Report

Merging #2669 into master will decrease coverage by 3.49%.
The diff coverage is 91.66%.

@@            Coverage Diff            @@
##           master    #2669     +/-   ##
=========================================
- Coverage   67.62%   64.13%   -3.5%     
=========================================
  Files         340      338      -2     
  Lines       43056    43026     -30     
  Branches     5329     5330      +1     
=========================================
- Hits        29116    27593   -1523     
- Misses      13238    14376   +1138     
- Partials      702     1057    +355

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.13% <91.66%> (-0.96%)`	⬇️

Impacted Files	Coverage Δ
nipype/pipeline/engine/nodes.py	`77.46% <ø> (-6.84%)`	⬇️
nipype/pipeline/engine/base.py	`88.13% <100%> (-0.33%)`	⬇️
nipype/pipeline/engine/utils.py	`75.25% <66.66%> (-7%)`	⬇️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/nitime/analysis.py	`43.15% <0%> (-36.85%)`	⬇️
nipype/interfaces/freesurfer/base.py	`49.59% <0%> (-30.9%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35.39% <0%> (-29.21%)`	⬇️
nipype/interfaces/spm/base.py	`58.41% <0%> (-29.05%)`	⬇️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69dce12...670c39e. Read the comment docs.

djarecka · 2018-08-02T17:17:34Z

I'm trying to write tests for dot files, so reading the write_graph method and have problem with simple_form. I can see that when this is False the graph.dot has longer names (e.g., "pipe.mod1.EngineTestInterface.engine" instead of "mod1 (engine)"), but graph_detailed has the same short names (so doesn't care about simple_form). Is this OK? Let me know if you think that this is a bug, otherwise I'll just keep it without any changes.
(cc: @satra , @oesteban , @effigies , @mgxd )

effigies · 2018-08-02T18:05:08Z

I think the detailed graph has never made a distinction wrt simple_form that I've been aware of, so it seems fine to leave it as is unless it was supposed to. I don't have strong opinions about the graphs.

…ifferent order

effigies · 2018-08-03T19:46:20Z

Is this ready for review?

djarecka · 2018-08-03T19:48:19Z

yes

oesteban · 2018-08-03T21:45:51Z

nipype/pipeline/engine/base.py

@@ -36,11 +36,11 @@ def __init__(self, name=None, base_dir=None):

        """
        self._hierarchy = None
-        self._name = None
+        self.name = name


Doesn't this break the property definition below? (line 46)

why? Before we had two lines: the first one self._name = None and 3 lines later self.name = name. I thought that one was left there forgotten.

This change looks reasonable to me. There's no instance variable used by name.setter that is set between lines 39 and 43, and nothing in these lines uses self._name in any way, so it's hard to see where this would cause a problem.

I'm not sure what @oesteban means by "break[ing] the property definition". Perhaps you could clarify?

effigies · 2018-08-03T19:49:44Z

nipype/pipeline/engine/tests/test_engine.py

@@ -9,7 +9,7 @@
 from builtins import open
 from copy import deepcopy
 from glob import glob
-import os
+import os, pdb


You don't seem to use pdb.

effigies · 2018-08-03T19:51:31Z

nipype/pipeline/engine/utils.py

@@ -507,7 +507,7 @@ def _write_detailed_dot(graph, dotfilename):
    # write nodes
    edges = []
    for n in nx.topological_sort(graph):
-        nodename = str(n)
+        nodename = str(n.itername)


itername is a str, so this could just be nodename = n.itername.

effigies · 2018-08-06T12:55:16Z

nipype/pipeline/engine/base.py

@@ -36,11 +36,11 @@ def __init__(self, name=None, base_dir=None):

        """
        self._hierarchy = None
-        self._name = None
+        self.name = name


This change looks reasonable to me. There's no instance variable used by name.setter that is set between lines 39 and 43, and nothing in these lines uses self._name in any way, so it's hard to see where this would cause a problem.

I'm not sure what @oesteban means by "break[ing] the property definition". Perhaps you could clarify?

effigies · 2018-08-06T13:00:50Z

nipype/pipeline/engine/tests/test_engine.py

+        graph_str = f.read()
+
+    if simple:
+        for str in eval("dotfile_{}".format(graph_type)):


Could we avoid evals unless absolutely necessary? We could do this with a dict:

dotfiles = { 'hierarchical': dotfile_hierarchical, ... } ... for contents in dotfiles[graph_type]: ...

Also, please do not use str as a variable name, as it is a built-in class. (I know we don't follow this rule in the entire code base, but I'd rather not introduce more cases of this.)

sure, will remove str. Can you comment why you think I should not use eval here? Or in general you're against of using eval anywhere?

I'm against the use of eval at all, unless there is no other way to do what needs doing (and there is almost always another way).

eval makes it more difficult to reason about programs, including static analysis like flake8 or pylint, because the code to be executed cannot in principle be known prior to execution. By design, it is a function that permits arbitrary code execution via string manipulation, which is also a security risk, even if it is not clear at the time how it could be exploited.

A decent StackOverflow thread: Why is using 'eval' a bad practice?

ok, will change

thanks for comments

effigies · 2018-08-06T19:03:03Z

@oesteban Any remaining concerns? Was your earlier concern addressed?

fixing detailed graph and workflow.run().nodes() representation

eadb14f

moving itername and _id from Node to EngineBase

431be35

djarecka added 2 commits August 2, 2018 09:34

cleaning self.name in EngineBase.__init__

7ed4ea2

fixing self.node in EngineBase

f860ad6

djarecka mentioned this pull request Aug 2, 2018

incorrect detailed graphs being generated #2668

Closed

djarecka added 2 commits August 2, 2018 14:33

adding tests for dot files (workflows with and without iterables)

052ee72

fixing tests for dot files - for py<3.6 the files can have lines in d…

879ea28

…ifferent order

effigies added this to the 1.1.2 milestone Aug 3, 2018

oesteban reviewed Aug 3, 2018

View reviewed changes

effigies reviewed Aug 6, 2018

View reviewed changes

small cleaning, Chris review

670c39e

effigies approved these changes Aug 6, 2018

View reviewed changes

effigies changed the title ~~fixing detailed graph and workflow.run().nodes() representation, closes 2668~~ FIX: Node __repr__ and detailed graph expansion Aug 7, 2018

effigies merged commit d2abc85 into nipy:master Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Node repr and detailed graph expansion #2669

FIX: Node repr and detailed graph expansion #2669

djarecka commented Aug 2, 2018 •

edited

Loading

djarecka commented Aug 2, 2018

codecov-io commented Aug 2, 2018 •

edited

Loading

djarecka commented Aug 2, 2018

effigies commented Aug 2, 2018

effigies commented Aug 3, 2018

djarecka commented Aug 3, 2018

oesteban Aug 3, 2018

djarecka Aug 3, 2018

effigies Aug 6, 2018

effigies Aug 3, 2018

effigies Aug 3, 2018

effigies Aug 6, 2018

effigies Aug 6, 2018

djarecka Aug 6, 2018

effigies Aug 6, 2018

djarecka Aug 6, 2018

djarecka Aug 6, 2018

effigies commented Aug 6, 2018

FIX: Node __repr__ and detailed graph expansion #2669

FIX: Node __repr__ and detailed graph expansion #2669

Conversation

djarecka commented Aug 2, 2018 • edited Loading

Summary

Acknowledgment

djarecka commented Aug 2, 2018

codecov-io commented Aug 2, 2018 • edited Loading

Codecov Report

djarecka commented Aug 2, 2018

effigies commented Aug 2, 2018

effigies commented Aug 3, 2018

djarecka commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 6, 2018

FIX: Node repr and detailed graph expansion #2669

FIX: Node repr and detailed graph expansion #2669

djarecka commented Aug 2, 2018 •

edited

Loading

codecov-io commented Aug 2, 2018 •

edited

Loading