[MAINT] Reorganize nipype.interfaces.base #2303

oesteban · 2017-11-26T01:31:57Z

This PR intends to 1) split nipype/nipype/interfaces/base.py in a more digestible python submodule; 2) remove miscellaneous/utility functions and place them in modules where they are more visible or they fit better. After all, nipype code should be more readable after these changes;
3) completely isolate the traits_extension module, so users are now expected to use base.traits for all traits in nipype. That would also help the transition towards traits or alternatives.

This PR does not intend to change any functionality. The idea is making it easier to transition code of Nipype Interfaces from 1.0 to 2.0. Also, it will be very appreciated in a "detachable" nipype CommandLine I'm working on (the run member will still work exactly the same, but you can just start an interface and let it run in the background while you keep using your ipython shell, for instance).

New module structure

nipype.interfaces.base/
                  |- __init__          - Makes classes available the way they used to be available
                  |- core              - The interfaces proper, cleaned up of other code
                  |- specs             - input/output specs to be used by interfaces
                  |- support           - some miscellaneous utilities needed by interfaces
                  \- traits_extension  - same old module, with few additions

This should not be merged before releasing 0.14.0.

List of changes:

…ref/interface-base

oesteban · 2017-12-01T03:55:32Z

It'd be great to consider this one ASAP before it gets stale

@satra - I asked Dorota and Mathias for reviews, but your big-picture opinion would be greatly appreciated :)

djarecka · 2017-12-01T04:20:22Z

@oesteban - completely missed the request... Will check in the morning and see if I have any comments.

oesteban · 2017-12-01T04:21:24Z

@djarecka sure, no rush!

djarecka

I tried to go through my old traitlets pr and see which concerns were valid also for traits. I made some comments to your changes, but some of them are related to part you're not changing:

is anyone using MpiCommandLine? couldn't find any examples in nipypehttps://github.com/oesteban/nipype/blob/ref/interface-base/nipype/interfaces/base/core.py#L1145
_check_version_requirements does not check the max_version, this block has wrong indentation: https://github.com/oesteban/nipype/blob/ref/interface-base/nipype/interfaces/base/core.py#L403
I was a bit confused about BaseFile and File, etc., do we really need both? Also I believe that we're assuming that BaseUnicode uses the fast validator (https://github.com/oesteban/nipype/blob/ref/interface-base/nipype/interfaces/base/traits_extension.py#L98), but from traits documentation I understood that Unicode (not BaseUnicode) uses the C-level fast validator (http://docs.enthought.com/traits/traits_api_reference/trait_types.html?highlight=baseunicode#traits.trait_types.BaseUnicode. Am I missing something?

djarecka · 2017-12-01T13:51:25Z

nipype/interfaces/base/core.py

-    """
-    _ = traits.Disallow
-
-
 class Interface(object):
    """This is an abstract definition for Interface objects.


this is a general question that I had at some point, since we try to build an abstract class here, have you ever had discussion about abc class? Never used it, but it seems to be the package to write an abstract class in python

Back in a day, I proposed something similar: https://github.com/oesteban/nipype/blob/fe4578b4461ee8aee63c86eaf47de5ac4985fae6/nipype/interfaces/base/interfaces.py#L62-L101

In this case, using the abstract interfaces that you build using traits.api.Interface and those @provides decorators.

djarecka · 2017-12-01T15:29:36Z

nipype/interfaces/base/specs.py

+                                   **{'%s' % name: Undefined,
+                                      '%s' % trait_spec.new_name: new})
+
+    def _hash_infile(self, adict, key):


I believe we're not using it anywhere

djarecka · 2017-12-01T15:34:15Z

nipype/interfaces/base/specs.py

+        """
+
+        dict_withhash = []
+        dict_nofilename = []


dict_withhash and dict_nofilename are lists, should we slightly change the names?

djarecka · 2017-12-01T15:53:37Z

nipype/interfaces/base/support.py

+        sorted_dict = to_str(sorted(dict_nofilename.items()))
+        return dict_withhash, md5(sorted_dict.encode()).hexdigest()
+
+    def __pretty__(self, p, cycle):


is this method still working? is it for this package: http://ipython.readthedocs.io/en/stable/api/generated/IPython.lib.pretty.html#extending ?
they suggest _repr_pretty_

djarecka · 2017-12-01T15:58:20Z

nipype/interfaces/base/specs.py

+                           'which is already set') % (name, trait_name)
+                    raise IOError(msg)
+
+    def _requires_warn(self, obj, name, old, new):


couldn't find any place where we're using it

djarecka · 2017-12-01T16:08:38Z

nipype/interfaces/base/traits_extension.py

@@ -334,3 +352,117 @@ def has_metadata(trait, metadata, value=None, recursive=True):
                count += has_metadata(handler, metadata, recursive)

    return count > 0
+
+
+class MultiPath(traits.List):


after @satra suggestions i've started calling it MultiObject

djarecka · 2017-12-01T16:13:36Z

nipype/interfaces/base/traits_extension.py

+        self.set_value(object, name, value)
+
+
+class InputMultiPath(MultiPath):


this class don't have any new method so i was wondering if we can decrease number of classes, e.g. leave InputMultiPath and OutputMultiPath

satra · 2017-12-02T15:59:46Z

nipype/interfaces/base/support.py

+        return '{}'.format(self.value)
+
+
+class Bunch(object):


we should try to refine and simplify this class' usage. perhaps replace it with hastraits in a refactor. this was one of the first classes we created in nipype to simplify interactive usage even before traits was added!

Yep, this would be great to add in @djarecka's issue.

satra · 2017-12-02T16:04:00Z

nipype/pipeline/plugins/tests/test_callback.py

    so = Status()
-    wf = pe.Workflow(name='test', base_dir=tmpdir.strpath)
+    wf = pe.Workflow(name='test')


perhaps base_dir should be set to local wd. in this case the working directory will be in some other tmp directory

satra · 2017-12-02T16:07:26Z

setup.py

@@ -106,7 +106,7 @@ def main():
        pjoin('workflows', 'data', '*'),
        pjoin('pipeline', 'engine', 'report_template.html'),
        pjoin('external', 'd3.js'),
-        pjoin('interfaces', 'script_templates', '*'),
+        pjoin('script_templates', '*'),


since the script_templates are all fsl model related, perhaps this can be moved into interfaces/fsl/model/script_templates

satra · 2017-12-02T16:09:29Z

@oesteban - overall looks great!

oesteban · 2017-12-03T17:03:53Z

Thanks! @mgxd may you do the honors?

mgxd

lgtm - minor comments

mgxd · 2017-12-03T20:10:09Z

nipype/interfaces/base/support.py

+        as the `inputs` to another node when interfaces are used in
+        the pipeline.
+    runtime : Bunch
+


extra line?

removed 👍

mgxd · 2017-12-03T20:10:41Z

nipype/interfaces/niftyreg/tests/test_reg.py

+from .. import (get_custom_path, RegAladin, RegF3D)
+
+
+def no_nifty_tool(cmd=None):


maybe define in tests/utils.py and import in each test?

mgxd · 2017-12-03T20:10:53Z

nipype/utils/filemanip.py

+    """
+
+    if pathext is None:
+        pathext = os.environ.get("PATHEXT", "").split(os.pathsep)


os.getenv cleaner?

satra · 2017-12-03T21:11:59Z

@oesteban - shouldn't many of the base tests move into the base directory as well?

oesteban · 2017-12-04T07:29:42Z

Working on it

…ref/interface-base

oesteban · 2017-12-04T20:39:22Z

this PR will require a huge make specs before merging, just let me know when you are ready to make that happen.

satra · 2017-12-04T21:02:49Z

@oesteban - changes look good to me. thanks for the effort.

satra · 2017-12-04T21:19:09Z

when updating the autotests make sure to add init.py to the new test folders.

oesteban added 14 commits November 22, 2017 15:08

move _exists_in_path to filemanip, and update

7b2b350

use nipype's which in nifty package

f6f97b6

ensure which call in ants/segmentation is correct)

a984f9f

ensure which calls in interfaces/base are correct

8b56e12

cleaning up imports

6676d22

move from base with deprecation the load_template method

d6a4b90

mv Multi- traits to traits_extension

833d716

deep refactor of nipype.interfaces.base

82411ae

fix final errors and wrong imports

d9c7f3d

test passing locally

d663e2f

remove some unicode prefixes

8629376

simplify fname_presuffix adding test

6949323

run test in temp directory

4e37641

add some docstrings

672d1a0

oesteban added in-progress maintenance labels Nov 26, 2017

oesteban added this to the 0.14.1 milestone Nov 26, 2017

oesteban added 4 commits November 25, 2017 17:53

fixup DictStrStr

92a8ce5

member could be function, and use filemanip.hash_infile

3b83940

amend to previous commit

59ae74c

minor fixes

290eb77

oesteban requested review from djarecka and mgxd November 27, 2017 16:32

oesteban added 2 commits November 29, 2017 14:07

Merge remote-tracking branch 'upstream/master' into ref/interface-base

67491af

Merge branch 'ref/interface-base' of github.com:oesteban/nipype into …

e5bb856

…ref/interface-base

oesteban changed the title ~~[WIP,MAINT] Reorganize nipype.interfaces.base~~ [MAINT] Reorganize nipype.interfaces.base Dec 1, 2017

djarecka reviewed Dec 1, 2017

View reviewed changes

djarecka mentioned this pull request Dec 1, 2017

suggesting changes to interfaces.base #2320

Closed

12 tasks

oesteban added 2 commits December 2, 2017 07:46

Merge remote-tracking branch 'upstream/master' into ref/interface-base

281c0e7

change dict_ when they are list_

5758373

satra reviewed Dec 2, 2017

View reviewed changes

oesteban added 2 commits December 2, 2017 18:54

set base_dir

b454961

move fsl.model templates

98f5c31

mgxd approved these changes Dec 3, 2017

View reviewed changes

final fixes

79c22ee

oesteban added 4 commits December 4, 2017 12:07

move tests, update specs

9147408

split huge test_base into pieces

86792b9

fix imports in new tests

15cd86f

Merge branch 'ref/interface-base' of github.com:oesteban/nipype into …

609c505

…ref/interface-base

oesteban added 2 commits December 4, 2017 14:36

fix tests

50580aa

update specs [skip ci]

5512779

oesteban removed the in-progress label Dec 5, 2017

oesteban merged commit d42ae1a into nipy:master Dec 5, 2017

oesteban deleted the ref/interface-base branch December 5, 2017 00:12

effigies modified the milestones: 0.14.1, 1.0 Jan 20, 2018

This was referenced Jan 22, 2018

changes in Interfaces base (closes #2320) #2387

Merged

refining and simplifying Bunch #2394

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT] Reorganize nipype.interfaces.base #2303

[MAINT] Reorganize nipype.interfaces.base #2303

oesteban commented Nov 26, 2017

oesteban commented Dec 1, 2017

djarecka commented Dec 1, 2017

oesteban commented Dec 1, 2017

djarecka left a comment

djarecka Dec 1, 2017

oesteban Dec 1, 2017

djarecka Dec 1, 2017

djarecka Dec 1, 2017

djarecka Dec 1, 2017

djarecka Dec 1, 2017

djarecka Dec 1, 2017

djarecka Dec 1, 2017

satra Dec 2, 2017 •

edited

Loading

oesteban Dec 3, 2017

satra Dec 2, 2017

satra Dec 2, 2017

satra commented Dec 2, 2017

oesteban commented Dec 3, 2017

mgxd left a comment

mgxd Dec 3, 2017

oesteban Dec 3, 2017

mgxd Dec 3, 2017

mgxd Dec 3, 2017

satra commented Dec 3, 2017

oesteban commented Dec 4, 2017

oesteban commented Dec 4, 2017

satra commented Dec 4, 2017

satra commented Dec 4, 2017

		self.set_value(object, name, value)


		class InputMultiPath(MultiPath):

		from .. import (get_custom_path, RegAladin, RegF3D)


		def no_nifty_tool(cmd=None):

[MAINT] Reorganize nipype.interfaces.base #2303

[MAINT] Reorganize nipype.interfaces.base #2303

Conversation

oesteban commented Nov 26, 2017

New module structure

oesteban commented Dec 1, 2017

djarecka commented Dec 1, 2017

oesteban commented Dec 1, 2017

djarecka left a 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

satra Dec 2, 2017 • edited Loading

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

satra commented Dec 2, 2017

oesteban commented Dec 3, 2017

mgxd left a 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

satra commented Dec 3, 2017

oesteban commented Dec 4, 2017

oesteban commented Dec 4, 2017

satra commented Dec 4, 2017

satra commented Dec 4, 2017

satra Dec 2, 2017 •

edited

Loading