Skip to content
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

ENH: Add LibraryBaseInterface #2538

Merged
merged 11 commits into from
Apr 15, 2018
Merged

Conversation

effigies
Copy link
Member

Similar to CLI interfaces, interfaces built on Python libraries (such as dipy, nilearn, nipy, etc.) have some common features and requirements, and it may be useful to unify this into a base class.

Here I suggest a minimal LibraryBaseInterface class which:

  • Checks that the library is importable
  • Optionally checks for additional imports required by the interface (intended for requirements that go beyond the library's dependencies)
  • Defines version property by the library __version__, which should make it trivial to use min_ver and max_ver in input specs

As a quick demonstration, I've simplified DipyBaseInterface.

If this is something worth pursuing, I'll have a look at other library-based interface collections and see what useful things can be standardized and abstracted up into this class.

@satra @chrisfilo @oesteban Thoughts?

@oesteban
Copy link
Contributor

Looks like a great idea to me

@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #2538 into master will decrease coverage by <.01%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2538      +/-   ##
==========================================
- Coverage   66.88%   66.88%   -0.01%     
==========================================
  Files         327      330       +3     
  Lines       42491    42484       -7     
  Branches     5269     5269              
==========================================
- Hits        28422    28417       -5     
- Misses      13367    13369       +2     
+ Partials      702      698       -4
Flag Coverage Δ
#smoketests 50.67% <47.36%> (-0.19%) ⬇️
#unittests 64.23% <78.57%> (-0.02%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/__init__.py 100% <ø> (ø) ⬆️
nipype/interfaces/nitime/base.py 100% <100%> (ø)
nipype/algorithms/metrics.py 52.87% <100%> (-0.07%) ⬇️
nipype/interfaces/dipy/base.py 51.11% <100%> (-1.96%) ⬇️
nipype/interfaces/cmtk/nx.py 18.25% <100%> (-0.99%) ⬇️
nipype/interfaces/cmtk/base.py 100% <100%> (ø)
nipype/interfaces/cmtk/nbs.py 33.69% <100%> (-2.02%) ⬇️
nipype/interfaces/nilearn.py 96.66% <100%> (-0.16%) ⬇️
nipype/interfaces/nipy/model.py 26.63% <36.36%> (-3.76%) ⬇️
nipype/interfaces/cmtk/parcellation.py 13.05% <41.66%> (-1.14%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc09e00...3d5661c. Read the comment docs.

@satra
Copy link
Member

satra commented Apr 11, 2018

@effigies +1 - i think anytime we can abstract usefully and reduce boilerplate is a welcome change. i'm finishing up a new issue post on just this - it came up day before when i was with some folks in DC.

@effigies effigies force-pushed the enh/library_interfaces branch 2 times, most recently from cddaff1 to b8bd289 Compare April 12, 2018 13:23
@effigies
Copy link
Member Author

A few thoughts from playing with this:

  • There were a few different idioms for performing these checks, mostly involving test imports at various points in the file or the interfaces themselves. This seems to me to be a good argument for a more unified approach.
  • The file-scoped have_libXYZ variables have been centralized and are now imported into their original files, in case anybody's depending on them. I would suggest we eliminate these outside of test files, in the future.
  • From playing, the first importlib.import_module might take a little time, but every time after that is instantaneous, so we shouldn't be shy about keeping our namespaces clean for the sake of efficiency. And this will help us delay these test imports until actually needed, which should give us a speed boost on import nipype, as a lot of things are being loaded somewhere in the hierarchy now.
  • Style guide suggestion: No file-level imports that are not direct nipype dependencies. Imports inside _run_interface() are totally fine, efficiency-wise, and help make it clearer what the scope of each interface is.

Anyway, I think I've done what I'm likely to with this PR. If it seems valuable, cool. If there are other library-based interface sets that I've missed (I mostly discovered these by grepping for package_check) let me know. If it seems like it's not doing enough work to justify it, feel free to close.

except ImportError:
failed_imports.append(pkg)
if failed_imports:
iflogger.warn('Unable to import %s; %s interface may fail to '
Copy link
Member

Choose a reason for hiding this comment

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

is this going to generate a lot of warnings when we create docs? or make specs? i.e. things that simply create an instance of an interface:

InterfaceX()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, potentially. I guess we could set a class-level flag to only display once per class? Or do you have another preferred approach?

Copy link
Member

Choose a reason for hiding this comment

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

one option would be to remove this if. let's merge it and see how annoying it gets.

@satra
Copy link
Member

satra commented Apr 14, 2018

@effigies - this looks good to me.

for future work (in 2.0) we could consider simplifying many of these interfaces through the py3 annotation framework

@satra satra merged commit a7a9c85 into nipy:master Apr 15, 2018
@effigies effigies added this to the 1.0.3 milestone Apr 15, 2018
@effigies effigies deleted the enh/library_interfaces branch April 15, 2018 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants