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

Copy link

Collaborator

@djarecka djarecka commented Jan 21, 2018 •

edited

Loading

changes in interface base listed in #2320:

_check_version_requirements does not check the max_version, this block has wrong indentation
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?
couldn't find a place where we're using _requires_warn
the same about _hash_infile
is __pretty__ still working?
is it for this package: http://ipython.readthedocs.io/en/stable/api/generated/IPython.lib.pretty.html#extending ?
they suggest _repr_pretty_
Check on this synchronize
Check the logic of this

djarecka added 6 commits

January 20, 2018 22:22


 fixing test_input_version (showing that maximum version check doesnt ...

6d65cbb

...work)


 fixing _check_version_requirements, so it also checks maximal version

955b3ad


 fixing and splitting test_input_version

40a0804


 removing _requires_warn (not used)


 removing _hash_infile (not used)


 changing __pretty__ to _repr_pretty_

cc4c418

Copy link

Member

effigies commented Jan 21, 2018

Is this intended for 1.0, 1.0 maintenance, or 2.0?

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

@effigies this are for 1.0, but don't know if we have to wait to include everything before release. Was planning to do it last week, but didn't happen..

Notice, that these are independent points that were suggested by me or @satra during #2303 revision. If you or @satra find time to review it we can merge before finishing all points (I'm trying to keep them in separate commits) and I can open a new PR.
that would eliminate a bug in _check_version_requirements.

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

do you know anyone using MpiCommandLine? should I remove it?


 changing name MultiPath to MultiObject

36ffda1

Copy link

Member

effigies commented Jan 22, 2018

Okay, I can have a look.

As for MpiCommandLine, I would generally prefer not to remove functionality that someone may be using without a deprecation cycle. i.e. We should have had a few releases where we raised warnings that it was going to go away, which would give people time to complain or find another approach before we actually pull the rug out from under them. So unless there's a really compelling reason to remove it, I would leave it. If people think it's not worth supporting, we can deprecate it so it can be removed later.


 File(Directory) class didnt dffer from BaseFile(Directory), i.e. didn...

ab8ae1a

...t use a afast validator, so i renamed BaseFile(Directory) to File(Directory) and removed extra classes

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

@effigies ok, I removed MpiCommandLine from the list.

Note that my last commits renamed MultiPath (as suggested by @satra some time ago), and removed BaseFile/BaseDirectory (because they were not used and didn't differ from File/Directory). Hope that's ok, but if you think that would cause problems, I can revert

effigies

effigies reviewed

nipype/nipype/interfaces/base.py

View reviewed changes

Copy link

Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Commenting on the first 5 points (perhaps not in order).

I would be inclined to leave *MultiPath as it is for 1.0. External users depend on it. We can change it in 2.0. If you think it would be good to have *MultiObject for 1.0, I would simply do:

InputMultiObject = InputMultiPath
OutputMultiObject = OutputMultiPath

This would allow users to begin switching to the new syntax before 2.0, without modifying the class hierarchy.

I'll get to the rest shortly, but wanted to get you some feedback right now.

nipype/interfaces/base/core.py

check = dict(max_ver=lambda t: t is not None)

names = trait_object.trait_names(**check)

if names and self.version:

version = LooseVersion(str(self.version))

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

I'm not sure I see a semantic difference in 955b3ad. I see:

if names and self.version:
 ... # Check min_ver
 check = dict(...)
 names = trait_object.trait_names(**check)
 ... # Check max_ver

Which becomes:

if names and self.version:
 ... # Check min_ver
check = dict(...)
names = trait_object.trait_names(**check)
if names and self.version:
 ... # Check max_ver

Am I missing something?

Copy link

Collaborator Author

@djarecka djarecka Jan 22, 2018

Choose a reason for hiding this comment

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

answered lower in the discussion

nipype/interfaces/base/specs.py

'which is already set') % (name, trait_name)

raise IOError(msg)

def _requires_warn(self, obj, name, old, new):

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

This used to be called from _generate_handlers.

Lines 389 to 390 in 5495816

for elem in requires:

self.on_trait_change(self._requires_warn, elem)

I was unable to quickly find the PR it was removed in, so it's hard to tell whether its removal was intentional or a mistake. As an unused private methods, I'm okay with getting rid of it, but @satra, if this should still be in _generate_handlers, let us know.

nipype/interfaces/base/traits_extension.py

class BaseFile(BaseUnicode):

class File(BaseUnicode):

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

I'm unclear on the original justification for this separation, but dropping BaseFile seems fine to me.

nipype/interfaces/base/specs.py

'%s' % trait_spec.new_name: new

})

def _hash_infile(self, adict, key):

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

Unused private method. Good to remove, as far as I'm concerned. Again, @satra, feel free to override if this looks like a mistake.

nipype/interfaces/base/support.py

"""Support for the pretty module

pretty is included in ipython.externals for ipython > 0.10"""

def _repr_pretty_(self, p, cycle):

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

I'm okay with this, but we should verify that we can pretty print Bunches in IPython with this change. I don't know if the API changed beyond what method it looks for in your objects.

if it doesn't work quickly, I'd say remove the function altogether and create an issue to fix and re-add, rather than waste time on this today.

Copy link

Collaborator Author

@djarecka djarecka Jan 22, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

I've just checked that pretty from IPython.lib.pretty uses _repr_pretty_ method (and completely ignored when it was named __pretty__).
In my opinion it looks as "pretty" as simply printing Bunch, so I can either remove it or keep my name.

Let me know if this was design to work with different library.

Copy link

Member

@effigies effigies Jan 22, 2018

Choose a reason for hiding this comment

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

It looks like what it does is prevents an infinite loop during formating. Could you test with a bunch that contains itself?

Copy link

Collaborator Author

@djarecka djarecka Jan 22, 2018

Choose a reason for hiding this comment

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

yes, you're right, for a bunch that contains itself it makes a difference! But only with the new name _repr_pretty_.
And you don't actually have to import pretty in ipython.

@effigies effigies mentioned this pull request

[MAINT] Cleaning / simplify Node #2325

Merged

6 tasks

Copy link

Member

effigies commented Jan 22, 2018

I would not refactor Bunch this close to release. That feels like a rabbit hole that should be entered on a leisurely time frame.

Copy link

Member

effigies commented Jan 22, 2018

@djarecka Review complete. I'll look at the max_ver issue locally. I don't see how your change can be fixing it, but if the test fails before and works after, I'll allow it.

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

@effigies ok, I'm just working actually on Bunch, but I can definitely set this as a new PR for future versions

@satra

Copy link

Member

satra commented Jan 22, 2018

@djarecka - basefile and basedirectory provide different validation error messages than from traits. the code is not the same. please don't remove when the code does not match the traits code.

_requires_warn can be removed since we disabled that.
_hash_infile can be removed

there are many folks who rely on InputMultiPath for their external interfaces. MultiObject/Bunch should be left for the nipype 2.0 refactor.

Copy link

Collaborator Author

djarecka commented Jan 22, 2018 •

edited

Loading

@effigies re max_ver:
notice how names are defined. They should be redefined for max version check.

The best way to see it doesn't work is to see what is the Exception that is really being raised here. After fixing it the test will fail

Copy link

Member

effigies commented Jan 22, 2018

Ah, gotcha. Yeah, 👍 on max_ver fix.

Copy link

Collaborator Author

djarecka commented Jan 22, 2018 •

edited

Loading

@satra @effigies:

I'll revert MultiObject and together with Bunch refactoring will wait till 2.0
I can revert my changes of basefile and basedirectory, but I don't understand your point @satra. I did NOT remove these classes, I removed File and Directory classes because IMO were the same as BaseFile and BaseDirectory. Since I didn't want to change the API, I simply renamed BaseFile to File and BaseDirectory to Directory.

Answering your question @effigies about the original justification: I'm not sure (@satra can correct me), but 2 classes File and BaseFile were created probably to provide an option to use either fast validator or not (see the difference between BaseUnicode and Unicode in traits doc. However the code that was changing validator in File and Directory classes were commented 2 years ago

@satra

Copy link

Member

satra commented Jan 22, 2018

@djarecka - the reason for the classes have nothing to do with the validator, only the error message. trait's own error message is very cryptic for missing files/directories.

i'm fine with File/Directory being mapped directly to BaseFile/BaseDirectory

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

yes, I only meant that the reason to have two pairs File/Directory and BaseFile/BaseDirectory was probably related to the validator options offered by traits.


 Revert "changing name MultiPath to MultiObject"

252a745

It will be included in 2.0
This reverts commit 36ffda1.

@effigies effigies added this to the 1.0 milestone

djarecka added 2 commits

January 22, 2018 14:47


 removing extra line from misc.py

11212a4


  a small changes of comments about synchronize

c1ab001

Copy link

Collaborator Author

djarecka commented Jan 22, 2018

@satra @effigies if I understood all your comments correctly this should be ready for final review and merging if all tests pass

effigies

effigies approved these changes

View reviewed changes

@effigies effigies merged commit 3f5e185 into nipy:master