WIP: Allow dicts as Interface input and output specs #2083
Conversation
|
Note how much boilerplate remains in my "lightweight" custom interface, though! |
|
Regarding the remaining boilerplate, we use the following SimpleInterface as the base for a lot of our interfaces: class SimpleInterface(BaseInterface):
""" An interface pattern that allows outputs to be set in a dictionary """
def __init__(self, **inputs):
super(SimpleInterface, self).__init__(**inputs)
self._results = {}
def _list_outputs(self):
return self._resultsWith your additions, the class you described could be shortened to: class MeanImage(SimpleInterface):
input_spec = dict(in_file=traits.File())
output_spec = dict(mean_val=traits.Float())
def _run_interface(self, runtime):
data = nib.load(self.inputs.in_file).get_data()
self._results['mean_val'] = data.mean()
return runtimeThis isn't necessarily suggesting you add anything to the PR, but it seems relevant to the overall discussion of how downstream projects work with/around nipype boilerplate. |
|
That's nice. So why is it normally necessary to initialize the outputs directory with |
|
Probably automatic population/derivation of output values from corresponding inputs, such as the common idiom that takes |
|
@mwaskom - the _outputs().get() populates a dictionary with the expected outputs. when we first started nipype, https://github.com/nipy/nipype/blob/master/nipype/interfaces/base.py#L1155 while this still holds for many interfaces, for several this pattern breaks down. |
|
That makes sense, but it's very unclear from the name! (And not obvious to me which method call is doing the work). It's tangental to this pull request, but worth thinking about transitioning that functionality to a method with a more clear name. |
|
So CI failures seem mostly related to my removal of the
which appears functionally inaccurate in practice. |
mwaskom
force-pushed
the
interface_spec_dicts
branch
from
07b3a2b to
1439222
Compare
Codecov Report
@@ Coverage Diff @@
## master #2083 +/- ##
==========================================
+ Coverage 72.16% 72.16% +<.01%
==========================================
Files 1144 1144
Lines 57510 57514 +4
Branches 8244 8246 +2
==========================================
+ Hits 41501 41505 +4
- Misses 14708 14709 +1
+ Partials 1301 1300 -1
Continue to review full report at Codecov.
|
|
This turns out not to work as well as I would have liked in practice; my workflow dies with a pickling issue: Any thoughts? |
|
@mwaskom - based on your workflow, can you create a test case that fails? it may help us debug this. |
|
I can but it will take some time, so if there's something obvious I need to know about pickling and traits it would be helpful. |
|
OK not a workflow example, but this demonstrates the behavior: from nipype.interfaces import base
from nipype.interfaces.fsl import BET
from nipype.utils.filemanip import savepkl
class SimpleInterface(base.BaseInterface):
def __init__(self, **inputs):
if isinstance(self.input_spec, dict):
self.input_spec = type("AnonymousInputSpec",
(base.BaseInterfaceInputSpec,),
self.input_spec)
if isinstance(self.output_spec, dict):
self.output_spec = type("AnonymousOutputSpec",
(base.TraitedSpec,),
self.output_spec)
self.inputs = self.input_spec(**inputs)
self.estimated_memory_gb = 1
self.num_threads = 1
self._results = {}
def _list_outputs(self):
return self._results
class Test(SimpleInterface):
input_spec = dict(in_file=base.traits.File())
output_spec = dict(out_file=base.traits.File())
if __name__ == "__main__":
# works
savepkl("bet_input.pkl", BET().input_spec)
# fails
savepkl("test_input.pkl", Test().input_spec) |
|
If this is causing problems, what about this style of input/output spec? class MeanImage(SimpleInterface):
class input_spec(TraitedSpec):
in_file = traits.File()
class output_spec(TraitedSpec):
mean_val = traits.Float()
def _run_interface(self, runtime):
data = nib.load(self.inputs.in_file).get_data()
self._results['mean_val'] = data.mean()
return runtimeI haven't tried it, so I don't know if it has pickling troubles, but if it works and is an acceptable level of boilerplate, this PR might not be necessary. |
|
That avoids the pickling error but it still smells like bad code to me. Inputs and outputs shouldn't need to be anything more than key, value pairs from the user perspective. |
|
Simpler test case: from nipype.interfaces import base
from nipype.utils.filemanip import savepkl
if __name__ == "__main__":
spec = type("AnonymousInputSpec",
(base.TraitedSpec,),
dict(a="b"))
savepkl("test_input.pkl", spec) |
|
Not sure if informative, but: In [18]: dill.detect.badobjects(spec)
Out[18]: traits.has_traits.AnonymousInputSpec
In [19]: dill.detect.badtypes(spec)
Out[19]: traits.has_traits.MetaHasTraits |
|
likely some interaction with traits that's not giving it appropriate properties. In [26]: spec().copyable_trait_names()
Out[26]: []
In [27]: spec().a
Out[27]: 'b' |
|
Seems this is not a trait issue so much as a dynamic class issue: |
|
Interestingly, |
|
OK maybe here's an alternate approach that avoids dynamic metaclass stuff: import pickle
from nipype.interfaces import base
class SimpleInterface(base.BaseInterface):
def __init__(self, **inputs):
if isinstance(self.input_spec, dict):
input_spec = base.BaseInterfaceInputSpec()
for key, val in self.input_spec.items():
input_spec.add_trait(key, val)
self.input_spec = input_spec
class Test(SimpleInterface):
input_spec = dict(in_file=base.traits.File())
if __name__ == "__main__":
pickle.dumps(Test().input_spec) |
|
Wait that doesn't work as the InputSpec shouldn't be called in the constructor before setting the inputs (I guess?) |
|
here is a relevant so post: https://stackoverflow.com/questions/4647566/pickle-a-dynamically-parameterized-sub-class/11526524#11526524 |
|
Closing as this ended up being more complicated and less useful than expected. |
See #2081 for motivation. Here is a basic proof of principle:
I did run into some issues in type() with
__future__.unicode_literalsand then what I am guessing is whatever cross 2/3 magic nipype does tostr()that will need to be sorted out...