[WIP] Lightweight implementation of `InterfaceResult` #2805

oesteban · 2018-11-27T00:34:28Z

Summary

(Ref #2776)
This PR replaces the good old Bunch. Bunch has been moved to the module nipype.utils.misc, although it is still loadable from nipype.interfaces.base.

The memory fingerprint of runtime objects improve by some 20%, not that we will see a huge difference but it justifies itself as an optimization:

>>> from nipype.interfaces.base.support import InterfaceRuntime 
>>> from pympler import asizeof 
>>> rt = InterfaceRuntime(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters') 
>>> asizeof.asizeof(rt)
472

>>> from nipype.interfaces.base import Bunch
>>> from pympler import asizeof
>>> rt = Bunch(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters')
>>> asizeof.asizeof(rt)
584

** pympler measures the whole size of the data structure, including the full tree of references.

List of changes proposed in this PR (pull-request)

A new nipype.interfaces.base.support.InterfaceRuntime object with a reduce memory footprint that the previous Bunch.
Bunch has been moved to nipype.utils.misc and some unused code has been removed.

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

This PR replaces the good old ``Bunch``. ``Bunch`` has been moved to the module ``nipype.utils.misc``, although it is still loadable from ``nipype.interfaces.base``.

oesteban · 2018-11-27T01:22:54Z

One caveat is that this is not better on Python 2.7:

>>> from nipype.interfaces.base.support import InterfaceRuntime
>>> from nipype.interfaces.base import Bunch
>>> from pympler import asizeof 
>>> rt = InterfaceRuntime(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters') 
>>> asizeof.asizeof(rt)
736

>>> rt2 = Bunch(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters')
>>> asizeof.asizeof(rt2)
688

The difference is about 10% (half as much as before), and we can always patch the InterfaceRuntime object to be a Bunch for Python 2.

EDIT: the difference goes away with bigger objects:

>>> from nipype.interfaces.base.support import InterfaceRuntime
>>> from nipype.interfaces.base import Bunch
>>> from pympler import asizeof 
>>> rt = InterfaceRuntime(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore')
>>> asizeof.asizeof(rt)
784

>>> rt2 = Bunch(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore')
>>> asizeof.asizeof(rt2)
784

and

>>> from nipype.interfaces.base.support import InterfaceRuntime
>>> from nipype.interfaces.base import Bunch
>>> from pympler import asizeof 
>>> rt = InterfaceRuntime(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore', returncode=0)
>>> asizeof.asizeof(rt)
808

>>> rt2 = Bunch(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore', returncode=0)
>>> asizeof.asizeof(rt2)
856

And, in python 3.7, things start to be very different:

>>> from nipype.interfaces.base.support import InterfaceRuntime
>>> from nipype.interfaces.base import Bunch
>>> from pympler import asizeof 
>>> rt = InterfaceRuntime(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore', returncode=0)
>>> asizeof.asizeof(rt)
552

>>> rt2 = Bunch(cmdline='/bin/echo', cwd='/home/oesteban', stdout='some text that is actually somewhat longer than other parameters', stderr='onemore', returncode=0)
>>> asizeof.asizeof(rt2)
784

codecov-io · 2018-11-27T02:23:58Z

Codecov Report

Merging #2805 into master will decrease coverage by 3.38%.
The diff coverage is 79.03%.

@@            Coverage Diff             @@
##           master    #2805      +/-   ##
==========================================
- Coverage   67.52%   64.14%   -3.39%     
==========================================
  Files         341      340       -1     
  Lines       43312    43274      -38     
  Branches     5371     5369       -2     
==========================================
- Hits        29248    27758    -1490     
- Misses      13363    14444    +1081     
- Partials      701     1072     +371

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.14% <79.03%> (-0.81%)`	⬇️

Impacted Files	Coverage Δ
nipype/utils/draw_gantt_chart.py	`13.07% <0%> (+0.25%)`	⬆️
nipype/workflows/smri/freesurfer/autorecon3.py	`68.4% <0%> (-6.95%)`	⬇️
nipype/algorithms/modelgen.py	`58.93% <100%> (-8.51%)`	⬇️
nipype/interfaces/spm/model.py	`43.09% <100%> (+0.13%)`	⬆️
nipype/interfaces/base/__init__.py	`100% <100%> (ø)`	⬆️
nipype/interfaces/base/core.py	`85% <100%> (-2.09%)`	⬇️
nipype/pipeline/engine/utils.py	`71.46% <20%> (-10.69%)`	⬇️
nipype/pipeline/engine/nodes.py	`77.9% <50%> (-6.62%)`	⬇️
nipype/utils/bunch.py	`73.46% <73.46%> (ø)`
nipype/interfaces/base/support.py	`96.82% <96.07%> (+25.82%)`	⬆️
... and 52 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 0fde422...95444f2. Read the comment docs.

nipype/interfaces/base/support.py

effigies · 2018-11-28T02:43:01Z

nipype/interfaces/base/support.py

+    >>> pickleds = pickle.dumps(rt)
+    >>> newrt = pickle.loads(pickleds)
+    >>> newrt
+    Bunch(cmdline='/bin/echo', cwd='/scratch/workflow', returncode=0)


Why Bunch and not InterfaceRuntime?

That is for backwards compatibility. If I change Bunch I'm afraid I'll need to update all the tests. But I can make that extra mile.

I would much prefer repr to accurately reflect the class name. Sorry that it's a lot of work...

effigies · 2018-11-28T02:49:50Z

nipype/interfaces/base/support.py

    """
+    __slots__ = ['interface', 'runtime', 'inputs', 'outputs', 'provenance']
+    version = __version__


I think the goal of the version attribute was to invalidate results loaded from an incompatible format, not record the nipype version.

MMM so this is why we had 2.0 here? was that the pickle protocol version?

@satra Perhaps you can clarify? I interpreted as the InterfaceResult API version.

effigies · 2018-11-28T02:50:44Z

nipype/interfaces/base/support.py

+    def __setstate__(self, state):
+        """Necessary for un-pickling"""
+        for key in self.__class__.__slots__:
+            setattr(self, key, state.get(key, None))


Should you check that state.get('version') == self.version?

Throw a warning at least would be appropriate. Since I was trying to do a minimal proof of concept, I passed. But yes, I'll include the warning.

Warning added 👍

nipype/pipeline/engine/utils.py

nipype/utils/draw_gantt_chart.py

…e-object

oesteban · 2018-11-29T20:29:36Z

Do not merge yet, still under testing.

effigies · 2019-01-16T17:42:39Z

Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release.

oesteban · 2019-01-16T18:35:32Z

We can push this one to 1.1.9

effigies · 2019-02-14T15:41:54Z

@oesteban Just a bump to note that the merge window closes next Friday, Feb 22.

oesteban · 2019-02-14T17:14:44Z

I'm going to close this one for now. I am confident this is a good direction, but I need to mull over the details of the implementation. I'll reopen when ready.

effigies · 2019-02-14T17:16:41Z

I'm going to mark it orphaned. Let's start a new PR when you're ready.

effigies · 2019-02-14T17:17:25Z

This pull request is "orphaned," which means it has been deemed to be abandoned by its original author. Orphaned pull requests have not been rejected, and we hope that if a user sees one that will meet their needs with a little work, that they will fork it and open a new pull request (or, in the case of the original author, reopen the original PR).

We ask that all adopted PRs be updated to merge or rebase the current master. If you would like to adopt a PR and need help getting started, any of a number of contributors will be happy to help.

[ENH] Lean implementation of InterfaceResult

bb6caec

This PR replaces the good old ``Bunch``. ``Bunch`` has been moved to the module ``nipype.utils.misc``, although it is still loadable from ``nipype.interfaces.base``.

fix tests

7b08507

make InterfaceResult also pickleable having __slots__

0a4ba14

oesteban added this to the 1.1.7 milestone Nov 27, 2018

aim at 100% coverage on support.py

771482e

oesteban changed the title ~~[ENH] Lean implementation of InterfaceResult~~ [ENH] Lightweight implementation of InterfaceResult Nov 27, 2018

effigies reviewed Nov 28, 2018

View reviewed changes

oesteban mentioned this pull request Nov 28, 2018

Idea to minimize memory fingerprint #2776

Open

oesteban added 4 commits November 28, 2018 08:53

address @effigies' comments

92295ff

fix import

a73d064

Merge remote-tracking branch 'upstream/master' into enh/leaner-runtim…

21555f0

…e-object

stop using Bunch in repr of InterfaceRuntime

dd6351a

oesteban force-pushed the enh/leaner-runtime-object branch from 78fed19 to dd6351a Compare November 28, 2018 23:47

oesteban added 3 commits November 28, 2018 16:27

place Bunch isolated to avoid importing full misc

ee3a8a9

update examples

ad0c99b

include changes reverted by reset --hard

95444f2

oesteban changed the title ~~[ENH] Lightweight implementation of InterfaceResult~~ [WIP] Lightweight implementation of InterfaceResult Nov 29, 2018

effigies modified the milestones: 1.1.7, 1.1.8 Dec 14, 2018

effigies modified the milestones: 1.1.8, 1.1.9 Jan 16, 2019

oesteban closed this Feb 14, 2019

effigies added the orphaned label Feb 14, 2019

effigies removed this from the 1.1.9 milestone Feb 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Lightweight implementation of `InterfaceResult` #2805

[WIP] Lightweight implementation of `InterfaceResult` #2805

oesteban commented Nov 27, 2018

oesteban commented Nov 27, 2018 •

edited

Loading

codecov-io commented Nov 27, 2018 •

edited

Loading

effigies Nov 28, 2018

oesteban Nov 28, 2018

effigies Nov 28, 2018

effigies Nov 28, 2018

oesteban Nov 28, 2018

effigies Nov 28, 2018

effigies Nov 28, 2018

oesteban Nov 28, 2018

oesteban Nov 28, 2018

oesteban commented Nov 29, 2018

effigies commented Jan 16, 2019

oesteban commented Jan 16, 2019

effigies commented Feb 14, 2019

oesteban commented Feb 14, 2019

effigies commented Feb 14, 2019

effigies commented Feb 14, 2019

[WIP] Lightweight implementation of InterfaceResult #2805

[WIP] Lightweight implementation of InterfaceResult #2805

Conversation

oesteban commented Nov 27, 2018

Summary

List of changes proposed in this PR (pull-request)

Acknowledgment

oesteban commented Nov 27, 2018 • edited Loading

codecov-io commented Nov 27, 2018 • edited Loading

Codecov Report

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

oesteban commented Nov 29, 2018

effigies commented Jan 16, 2019

oesteban commented Jan 16, 2019

effigies commented Feb 14, 2019

oesteban commented Feb 14, 2019

effigies commented Feb 14, 2019

effigies commented Feb 14, 2019

[WIP] Lightweight implementation of `InterfaceResult` #2805

[WIP] Lightweight implementation of `InterfaceResult` #2805

oesteban commented Nov 27, 2018 •

edited

Loading

codecov-io commented Nov 27, 2018 •

edited

Loading