Merge pull request #2376 from oesteban/maint/engine-base

[MAINT] Cleanup EngineBase

Author: effigies

CHANGES

@@ -1,9 +1,10 @@
 Upcoming release (0.14.1)
 =========================
 
+* MAINT: Cleanup EngineBase (https://github.com/nipy/nipype/pull/2376)
 * FIX: Robustly handled outputs of 3dFWHMx across different versions of AFNI (https://github.com/nipy/nipype/pull/2373)
 * FIX: Cluster threshold in randomise + change default prefix (https://github.com/nipy/nipype/pull/2369)
-* MAINT: Cleaning / simplify ``Node`` (https://github.com/nipy/nipype/pull/#2325)
+* MAINT: Cleaning / simplify ``Node`` (https://github.com/nipy/nipype/pull/2325)
 * STY: Cleanup of PEP8 violations (https://github.com/nipy/nipype/pull/2358)
 * STY: Cleanup of trailing spaces and adding of missing newlines at end of files (https://github.com/nipy/nipype/pull/2355)
 

nipype/interfaces/tests/test_io.py

@@ -147,7 +147,8 @@ def test_s3datagrabber():
         "node_output": ["model"]
     }),
 ])
-def test_selectfiles(SF_args, inputs_att, expected):
+def test_selectfiles(tmpdir, SF_args, inputs_att, expected):
+    tmpdir.chdir()
     base_dir = op.dirname(nipype.__file__)
     dg = nio.SelectFiles(base_directory=base_dir, **SF_args)
     for key, val in inputs_att.items():

nipype/pipeline/engine/base.py

@@ -18,18 +18,14 @@
                         absolute_import)
 from builtins import object
 
-from future import standard_library
-standard_library.install_aliases()
-
 from copy import deepcopy
 import re
 import numpy as np
-from ... import logging
+
+from ... import config
 from ...interfaces.base import DynamicTraitedSpec
 from ...utils.filemanip import loadpkl, savepkl
 
-logger = logging.getLogger('workflow')
-
 
 class EngineBase(object):
     """Defines common attributes and functions for workflows and nodes."""
@@ -47,35 +43,36 @@ def __init__(self, name=None, base_dir=None):
             default=None, which results in the use of mkdtemp
 
         """
+        self._hierarchy = None
+        self._name = None
+
         self.base_dir = base_dir
-        self.config = None
-        self._verify_name(name)
+        self.config = deepcopy(config._sections)
         self.name = name
-        # for compatibility with node expansion using iterables
-        self._id = self.name
-        self._hierarchy = None
 
     @property
-    def inputs(self):
-        raise NotImplementedError
+    def name(self):
+        return self._name
 
-    @property
-    def outputs(self):
-        raise NotImplementedError
+    @name.setter
+    def name(self, name):
+        if not name or not re.match(r'^[\w-]+$', name):
+            raise ValueError('[Workflow|Node] name "%s" is not valid.' % name)
+        self._name = name
 
     @property
     def fullname(self):
-        fullname = self.name
         if self._hierarchy:
-            fullname = self._hierarchy + '.' + self.name
-        return fullname
+            return '%s.%s' % (self._hierarchy, self.name)
+        return self.name
 
     @property
-    def itername(self):
-        itername = self._id
-        if self._hierarchy:
-            itername = self._hierarchy + '.' + self._id
-        return itername
+    def inputs(self):
+        raise NotImplementedError
+
+    @property
+    def outputs(self):
+        raise NotImplementedError
 
     def clone(self, name):
         """Clone an EngineBase object
@@ -86,13 +83,10 @@ def clone(self, name):
         name : string (mandatory)
             A clone of node or workflow must have a new name
         """
-        if (name is None) or (name == self.name):
-            raise Exception('Cloning requires a new name')
-        self._verify_name(name)
+        if name == self.name:
+            raise ValueError('Cloning requires a new name, "%s" is in use.' % name)
         clone = deepcopy(self)
         clone.name = name
-        clone._id = name
-        clone._hierarchy = None
         return clone
 
     def _check_outputs(self, parameter):
@@ -103,17 +97,8 @@ def _check_inputs(self, parameter):
             return True
         return hasattr(self.inputs, parameter)
 
-    def _verify_name(self, name):
-        valid_name = bool(re.match('^[\w-]+$', name))
-        if not valid_name:
-            raise ValueError('[Workflow|Node] name \'%s\' contains'
-                             ' special characters' % name)
-
-    def __repr__(self):
-        if self._hierarchy:
-            return '.'.join((self._hierarchy, self._id))
-        else:
-            return '{}'.format(self._id)
+    def __str__(self):
+        return self.fullname
 
     def save(self, filename=None):
         if filename is None:

nipype/pipeline/engine/nodes.py

@@ -161,12 +161,13 @@ def __init__(self,
 
         super(Node, self).__init__(name, kwargs.get('base_dir'))
 
-        self.name = name
         self._interface = interface
         self._hierarchy = None
         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
         self.itersource = itersource
@@ -228,7 +229,6 @@ def n_procs(self):
         if hasattr(self._interface.inputs, 'num_threads') and isdefined(
                 self._interface.inputs.num_threads):
             return self._interface.inputs.num_threads
-
         return 1
 
     @n_procs.setter
@@ -240,6 +240,13 @@ def n_procs(self, value):
         if hasattr(self._interface.inputs, 'num_threads'):
             self._interface.inputs.num_threads = self._n_procs
 
+    @property
+    def itername(self):
+        itername = self._id
+        if self._hierarchy:
+            itername = self._hierarchy + '.' + self._id
+        return itername
+
     def output_dir(self):
         """Return the location of the output directory for the node"""
         # Output dir is cached

nipype/pipeline/engine/tests/test_base.py

@@ -0,0 +1,66 @@
+# -*- coding: utf-8 -*-
+# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
+# vi: set ft=python sts=4 ts=4 sw=4 et:
+from __future__ import print_function, unicode_literals
+
+import pytest
+from ..base import EngineBase
+from ....interfaces import base as nib
+
+
+class InputSpec(nib.TraitedSpec):
+    input1 = nib.traits.Int(desc='a random int')
+    input2 = nib.traits.Int(desc='a random int')
+    input_file = nib.traits.File(desc='Random File')
+
+
+class OutputSpec(nib.TraitedSpec):
+    output1 = nib.traits.List(nib.traits.Int, desc='outputs')
+
+
+class EngineTestInterface(nib.BaseInterface):
+    input_spec = InputSpec
+    output_spec = OutputSpec
+
+    def _run_interface(self, runtime):
+        runtime.returncode = 0
+        return runtime
+
+    def _list_outputs(self):
+        outputs = self._outputs().get()
+        outputs['output1'] = [1, self.inputs.input1]
+        return outputs
+
+
+@pytest.mark.parametrize(
+    'name', ['valid1', 'valid_node', 'valid-node', 'ValidNode0'])
+def test_create(name):
+    base = EngineBase(name=name)
+    assert base.name == name
+
+
+@pytest.mark.parametrize(
+    'name', ['invalid*1', 'invalid.1', 'invalid@', 'in/valid', None])
+def test_create_invalid(name):
+    with pytest.raises(ValueError):
+        EngineBase(name=name)
+
+
+def test_hierarchy():
+    base = EngineBase(name='nodename')
+    base._hierarchy = 'some.history.behind'
+
+    assert base.name == 'nodename'
+    assert base.fullname == 'some.history.behind.nodename'
+
+
+def test_clone():
+    base = EngineBase(name='nodename')
+    base2 = base.clone('newnodename')
+
+    assert (base.base_dir == base2.base_dir and
+            base.config == base2.config and
+            base2.name == 'newnodename')
+
+    with pytest.raises(ValueError):
+        base.clone('nodename')