Skip to content

[MAINT] Cleanup EngineBase #2376

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
Jan 18, 2018
Merged

[MAINT] Cleanup EngineBase #2376

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ca1489
[MAINT] Cleanup engine/base
oesteban Jan 17, 2018
47345da
update changes
oesteban Jan 17, 2018
11e42ea
refactoring tests
oesteban Jan 18, 2018
301fc4b
fix tests
oesteban Jan 18, 2018
b5f8537
sty
oesteban Jan 18, 2018
9a6e7cb
remove install_aliases
oesteban Jan 18, 2018
1d38127
fix error type
oesteban Jan 18, 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
64 changes: 26 additions & 38 deletions nipype/pipeline/engine/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,15 @@
absolute_import)
from builtins import object

from future import standard_library
standard_library.install_aliases()
Copy link
Member

Choose a reason for hiding this comment

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

I meant not to bother removing or reordering, and just tolerate the PEP8 violation. Unless it's causing problems, let's not change this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked locally with Python 2 and this aliases are not necessary. Let's get rid of them whenever possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@oesteban @effigies I'm not sure if I understand, does standard_library.install_aliases() at the end of the import block (e.g. in node.py or workflow.py) changes anything?

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe it does.


from copy import deepcopy
import re
import numpy as np
from ... import logging
from future import standard_library

from ...interfaces.base import DynamicTraitedSpec
from ...utils.filemanip import loadpkl, savepkl

logger = logging.getLogger('workflow')
standard_library.install_aliases()
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the point of having this before most imports to ensure that the imports work as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The aliases would modify the behavior of imports, so I guess that is the reason. Anyways, I moved it because flake8 suggested so. I can roll back at any moment

Copy link
Member

Choose a reason for hiding this comment

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

@mgxd @satra Are you familiar enough with future.standard_library.install_aliases to know whether there's a semantic change being made by reordering here?

You can also add a # noqa comment to pacify flake8, if you do return it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

some comments on the order are here: https://pypi.python.org/pypi/future

All examples here have it at the very beginning



class EngineBase(object):
Expand All @@ -47,35 +45,37 @@ 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.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 name and bool(re.match(r'^[\w_]+$', name)):
self._name = name
else:
raise ValueError('[Workflow|Node] name "%s" is not valid.' % name)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer:

if not name or not re.match(r'^\w+$', name):
    raise ValueError(...)
self._name = name

Note that I simplified the regex. \w matches underscores.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we were supporting also dashes - so I'll add them.


@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
Expand All @@ -86,13 +86,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):
Expand All @@ -103,17 +100,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:
Expand Down
12 changes: 10 additions & 2 deletions nipype/pipeline/engine/nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,14 @@ 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 = None # for compatibility with node expansion using iterables

self.name = name
self.iterables = iterables
self.synchronize = synchronize
self.itersource = itersource
Expand Down Expand Up @@ -228,7 +230,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
Expand All @@ -240,6 +241,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
Expand Down