Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[MAINT] Reorganize nipype.interfaces.base #2303

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.

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
oesteban merged 31 commits into nipy:master from oesteban:ref/interface-base
Dec 5, 2017

Conversation

@oesteban
Copy link
Contributor

@oesteban oesteban commented Nov 26, 2017

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:

  • Moved nipype.interfaces.base._exists_in_path -> nipype.utils.filemanip.which. Additionally, this function has been modified to rely on shutil.which on python >= 3.3. This function does not return bool, str anymore, to make it more similar to shutil.which. If a command is not found in PATH, it returns None (vs. False, None in the former version). Uses of the old _exists_in_path have been revised for this change.
  • Moved nipype.interfaces.base._canonicalize_env -> nipype.utils.filemanip.canonicalize_env.
  • Moved nipype.interfaces.base.get_depencencies -> nipype.utils.filemanip.get_dependencies.
  • Moved some nipype traits (OutputMultiPath, InputMultiPath, etc) and some monkeypatches to tratis into the new traits_extension submodule.
  • Moved interface specs (BaseTraitedSpec, TraitedSpec, DynamicTraitedSpec, etc) into a nipype.interfaces.base.specs module.
  • Moved nipype.interfaces.base.load_template to nipype.interfaces.fsl.model which was the only module that relied on it. A stub for backwards compatibility with a deprecation warning has been left in place. I also moved the script_templates folder to a (IMHO) better location under the top folder. Also, the templates are now accessed using the package_resources module.
  • Created a base submodule called support with utilities to the base interfaces (the Bunch, InterfaceResult, etc.).
  • Revised nifty- interfaces to use the version_from_command property of nipype interfaces (instead of re-implementing the same thing).
  • Minor editions of small issues found in files that I had to touch along the way (mostly unicode prefixes in tests, unused imports, etc).
  • Added some documentation to the new files.

Copy link
Contributor Author

oesteban commented Dec 1, 2017

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

@oesteban oesteban changed the title (削除) [WIP,MAINT] Reorganize nipype.interfaces.base (削除ここまで) (追記) [MAINT] Reorganize nipype.interfaces.base (追記ここまで) Dec 1, 2017
Copy link
Collaborator

djarecka commented Dec 1, 2017

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

Copy link
Contributor Author

oesteban commented Dec 1, 2017

@djarecka sure, no rush!

Copy link
Collaborator

@djarecka djarecka left a comment

Choose a reason for hiding this comment

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

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:



class Interface(object):
"""This is an abstract definition for Interface objects.
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@oesteban oesteban Dec 1, 2017

Choose a reason for hiding this comment

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

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.

**{'%s' % name: Undefined,
'%s' % trait_spec.new_name: new})

def _hash_infile(self, adict, key):
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

I believe we're not using it anywhere

"""

dict_withhash = []
dict_nofilename = []
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

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

oesteban reacted with thumbs up emoji
sorted_dict = to_str(sorted(dict_nofilename.items()))
return dict_withhash, md5(sorted_dict.encode()).hexdigest()

def __pretty__(self, p, cycle):
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

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_

'which is already set') % (name, trait_name)
raise IOError(msg)

def _requires_warn(self, obj, name, old, new):
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

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

return count > 0


class MultiPath(traits.List):
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

after @satra suggestions i've started calling it MultiObject

self.set_value(object, name, value)


class InputMultiPath(MultiPath):
Copy link
Collaborator

@djarecka djarecka Dec 1, 2017

Choose a reason for hiding this comment

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

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

return '{}'.format(self.value)


class Bunch(object):
Copy link
Member

@satra satra Dec 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@oesteban oesteban Dec 3, 2017

Choose a reason for hiding this comment

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

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


so = Status()
wf = pe.Workflow(name='test', base_dir=tmpdir.strpath)
wf = pe.Workflow(name='test')
Copy link
Member

Choose a reason for hiding this comment

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

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

setup.py Outdated
pjoin('pipeline', 'engine', 'report_template.html'),
pjoin('external', 'd3.js'),
pjoin('interfaces', 'script_templates', '*'),
pjoin('script_templates', '*'),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

satra commented Dec 2, 2017

@oesteban - overall looks great!

Copy link
Contributor Author

oesteban commented Dec 3, 2017

Thanks! @mgxd may you do the honors?

Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

lgtm - minor comments

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

Choose a reason for hiding this comment

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

extra line?

Copy link
Contributor Author

@oesteban oesteban Dec 3, 2017

Choose a reason for hiding this comment

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

removed 👍

from .. import (get_custom_path, RegAladin, RegF3D)


def no_nifty_tool(cmd=None):
Copy link
Member

Choose a reason for hiding this comment

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

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

"""

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

Choose a reason for hiding this comment

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

os.getenv cleaner?

Copy link
Member

satra commented Dec 3, 2017

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

Copy link
Contributor Author

oesteban commented Dec 4, 2017

Working on it

Copy link
Contributor Author

oesteban commented Dec 4, 2017

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

Copy link
Member

satra commented Dec 4, 2017

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

Copy link
Member

satra commented Dec 4, 2017

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

@oesteban oesteban merged commit d42ae1a into nipy:master Dec 5, 2017
@oesteban oesteban deleted the ref/interface-base branch December 5, 2017 00:12
@effigies effigies modified the milestones: 0.14.1, 1.0 Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satra satra satra left review comments

@djarecka djarecka djarecka left review comments

@mgxd mgxd mgxd approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

1.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /