Skip to content

FIX: Node __repr__ and detailed graph expansion #2669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 7, 2018
Merged

FIX: Node __repr__ and detailed graph expansion #2669

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eadb14f
fixing detailed graph and workflow.run().nodes() representation
djarecka Aug 2, 2018
431be35
moving itername and _id from Node to EngineBase
djarecka Aug 2, 2018
7ed4ea2
cleaning self.name in EngineBase.__init__
djarecka Aug 2, 2018
f860ad6
fixing self.node in EngineBase
djarecka Aug 2, 2018
052ee72
adding tests for dot files (workflows with and without iterables)
djarecka Aug 2, 2018
879ea28
fixing tests for dot files - for py<3.6 the files can have lines in d…
djarecka Aug 2, 2018
670c39e
small cleaning, Chris review
djarecka Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions nipype/pipeline/engine/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ def __init__(self, name=None, base_dir=None):

"""
self._hierarchy = None
self._name = None
self.name = name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

self._id = self.name # for compatibility with node expansion using iterables

self.base_dir = base_dir
self.config = deepcopy(config._sections)
self.name = name

@property
def name(self):
Expand All @@ -66,6 +66,14 @@ def inputs(self):
def outputs(self):
raise NotImplementedError

@property
def itername(self):
"""Name for expanded iterable"""
itername = self._id
if self._hierarchy:
itername = '%s.%s' % (self._hierarchy, self._id)
return itername

def clone(self, name):
"""Clone an EngineBase object

Expand Down Expand Up @@ -95,6 +103,9 @@ def _check_inputs(self, parameter):
def __str__(self):
return self.fullname

def __repr__(self):
return self.itername

def save(self, filename=None):
if filename is None:
filename = 'temp.pklz'
Expand Down
9 changes: 0 additions & 9 deletions nipype/pipeline/engine/nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ def __init__(self,
self._got_inputs = False
self._originputs = None
self._output_dir = None
self._id = self.name # for compatibility with node expansion using iterables

self.iterables = iterables
self.synchronize = synchronize
Expand Down Expand Up @@ -249,14 +248,6 @@ def n_procs(self, value):
if hasattr(self._interface.inputs, 'num_threads'):
self._interface.inputs.num_threads = self._n_procs

@property
def itername(self):
"""Name for expanded iterable"""
itername = self._id
if self._hierarchy:
itername = '%s.%s' % (self._hierarchy, self._id)
return itername

def output_dir(self):
"""Return the location of the output directory for the node"""
# Output dir is cached
Expand Down
151 changes: 150 additions & 1 deletion nipype/pipeline/engine/tests/test_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from builtins import open
from copy import deepcopy
from glob import glob
import os
import os, pdb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to use pdb.



import pytest
Expand Down Expand Up @@ -441,6 +441,7 @@ def test_write_graph_runs(tmpdir):

assert os.path.exists('graph.dot') or os.path.exists(
'graph_detailed.dot')

try:
os.remove('graph.dot')
except OSError:
Expand Down Expand Up @@ -484,6 +485,154 @@ def test_deep_nested_write_graph_runs(tmpdir):
pass


# examples of dot files used in the following test
dotfile_orig = ['strict digraph {\n',
'"mod1 (engine)";\n',
'"mod2 (engine)";\n',
'"mod1 (engine)" -> "mod2 (engine)";\n',
'}\n']

dotfile_detailed_orig = ['digraph structs {\n',
'node [shape=record];\n',
'pipemod1 [label="{IN}|{ mod1 | engine | }|{OUT|<outoutput1> output1}"];\n',
'pipemod2 [label="{IN|<ininput1> input1}|{ mod2 | engine | }|{OUT}"];\n',
'pipemod1:outoutput1:e -> pipemod2:ininput1:w;\n',
'}']

dotfile_flat = dotfile_orig
dotfile_detailed_flat = dotfile_detailed_orig

dotfile_exec = dotfile_orig
dotfile_detailed_exec = dotfile_detailed_orig


dotfile_hierarchical = ['digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)"];\n',
' pipe_mod2[label="mod2 (engine)"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfile_colored = ['digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod2[label="mod2 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

@pytest.mark.parametrize("simple", [True, False])
@pytest.mark.parametrize("graph_type", ['orig', 'flat', 'exec', 'hierarchical', 'colored'])
def test_write_graph_dotfile(tmpdir, graph_type, simple):
""" checking dot files for a workflow without iterables"""
tmpdir.chdir()

pipe = pe.Workflow(name='pipe')
mod1 = pe.Node(interface=EngineTestInterface(), name='mod1')
mod2 = pe.Node(interface=EngineTestInterface(), name='mod2')
pipe.connect([(mod1, mod2, [('output1', 'input1')])])
pipe.write_graph(
graph2use=graph_type, simple_form=simple, format='dot')

with open("graph.dot") as f:
graph_str = f.read()

if simple:
for str in eval("dotfile_{}".format(graph_type)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for comments

assert str in graph_str
else:
# if simple=False graph.dot uses longer names
for str in eval("dotfile_{}".format(graph_type)):
if graph_type in ["hierarchical", "colored"]:
assert str.replace("mod1 (engine)", "mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "mod2.EngineTestInterface.engine") in graph_str
else:
assert str.replace(
"mod1 (engine)", "pipe.mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "pipe.mod2.EngineTestInterface.engine") in graph_str

# graph_detailed is not created for hierachical or colored
if graph_type not in ["hierarchical", "colored"]:
with open("graph_detailed.dot") as f:
graph_str = f.read()
for str in eval("dotfile_detailed_{}".format(graph_type)):
assert str in graph_str


# examples of dot files used in the following test
dotfile_iter_orig = dotfile_orig
dotfile_detailed_iter_orig = dotfile_detailed_orig

dotfile_iter_flat = dotfile_orig
dotfile_detailed_iter_flat = dotfile_detailed_orig

dotfile_iter_exec = dotfile_orig
dotfile_detailed_iter_exec = [
'digraph structs {\n',
'node [shape=record];\n',
'pipemod1aIa1 [label="{IN}|{ a1 | engine | mod1.aI }|{OUT|<outoutput1> output1}"];\n',
'pipemod2a1 [label="{IN|<ininput1> input1}|{ a1 | engine | mod2 }|{OUT}"];\n',
'pipemod1aIa0 [label="{IN}|{ a0 | engine | mod1.aI }|{OUT|<outoutput1> output1}"];\n',
'pipemod2a0 [label="{IN|<ininput1> input1}|{ a0 | engine | mod2 }|{OUT}"];\n',
'pipemod1aIa0:outoutput1:e -> pipemod2a0:ininput1:w;\n',
'pipemod1aIa1:outoutput1:e -> pipemod2a1:ininput1:w;\n',
'}']

dotfile_iter_hierarchical = [
'digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", shape=box3d,style=filled, color=black, colorscheme=greys7 fillcolor=2];\n',
' pipe_mod2[label="mod2 (engine)"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfile_iter_colored = [
'digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", shape=box3d,style=filled, color=black, colorscheme=greys7 fillcolor=2];\n',
' pipe_mod2[label="mod2 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

@pytest.mark.parametrize("simple", [True, False])
@pytest.mark.parametrize("graph_type", ['orig', 'flat', 'exec', 'hierarchical', 'colored'])
def test_write_graph_dotfile_iterables(tmpdir, graph_type, simple):
""" checking dot files for a workflow with iterables"""
tmpdir.chdir()

pipe = pe.Workflow(name='pipe')
mod1 = pe.Node(interface=EngineTestInterface(), name='mod1')
mod1.iterables = ('input1', [1, 2])
mod2 = pe.Node(interface=EngineTestInterface(), name='mod2')
pipe.connect([(mod1, mod2, [('output1', 'input1')])])
pipe.write_graph(
graph2use=graph_type, simple_form=simple, format='dot')

with open("graph.dot") as f:
graph_str = f.read()

if simple:
for str in eval("dotfile_iter_{}".format(graph_type)):
assert str in graph_str
else:
# if simple=False graph.dot uses longer names
for str in eval("dotfile_iter_{}".format(graph_type)):
if graph_type in ["hierarchical", "colored"]:
assert str.replace("mod1 (engine)", "mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "mod2.EngineTestInterface.engine") in graph_str
else:
assert str.replace(
"mod1 (engine)", "pipe.mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "pipe.mod2.EngineTestInterface.engine") in graph_str

# graph_detailed is not created for hierachical or colored
if graph_type not in ["hierarchical", "colored"]:
with open("graph_detailed.dot") as f:
graph_str = f.read()
for str in eval("dotfile_detailed_iter_{}".format(graph_type)):
assert str in graph_str



def test_io_subclass():
"""Ensure any io subclass allows dynamic traits"""
from nipype.interfaces.io import IOBase
Expand Down
6 changes: 3 additions & 3 deletions nipype/pipeline/engine/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

inports = []
for u, v, d in graph.in_edges(nbunch=n, data=True):
for cd in d['connect']:
Expand All @@ -519,8 +519,8 @@ def _write_detailed_dot(graph, dotfilename):
ipstrip = 'in%s' % _replacefunk(inport)
opstrip = 'out%s' % _replacefunk(outport)
edges.append(
'%s:%s:e -> %s:%s:w;' % (str(u).replace('.', ''), opstrip,
str(v).replace('.', ''), ipstrip))
'%s:%s:e -> %s:%s:w;' % (u.itername.replace('.', ''), opstrip,
v.itername.replace('.', ''), ipstrip))
if inport not in inports:
inports.append(inport)
inputstr = ['{IN'] + [
Expand Down