nipype / pydra

Pydra Dataflow Engine
https://nipype.github.io/pydra/
Other
120 stars 59 forks source link

Task crashes when function returns None #256

Closed satra closed 4 years ago

satra commented 4 years ago

in both cases the named outputs should be set to None.

import pydra
import typing as ty
import os

@pydra.mark.task
@pydra.mark.annotate({"return": {"b": ty.Any}})
def test_multiout(val, val2):
    return None

if __name__ == "__main__":
    cache_dir = os.path.join(os.getcwd(), 'cache3')
    task = test_multiout(name="mo2",
                         val=[0, 1, 2],
                         val2=[4, 5, 6],
                         cache_dir=cache_dir)
    with pydra.Submitter(plugin="cf", n_procs=2) as sub:
        sub(runnable=task)
    results = task.result()
    print(results)

this should also be checked for multiple outputs.

@pydra.mark.task
@pydra.mark.annotate({"return": {"a": ty.Any, "b": ty.Any}})
def test_multiout(val, val2):
    return None
djarecka commented 4 years ago

not sure why the second example should work

satra commented 4 years ago

consider functions that you don't write yourself. in Python main functions will return None under certain operating conditions whether normally it would return multiple outputs.

djarecka commented 4 years ago

ok! but you want this work only if returns None. You don't want me to complete with Nones when the number of defined outputs doesn't match the one function returns

satra commented 4 years ago

i believe both. so:

@pydra.mark.task
@pydra.mark.annotate({"return": {"b": ty.Any}})
def test_multiout(val, val2):
    return None

@pydra.mark.task
@pydra.mark.annotate({"return": {"a": ty.Any, "b": ty.Any}})
def test_multiout(val, val2):
    return None

in the above two cases say the functions were task1 and task2

task1.result().output === {"b": None}
task2.result().output === {"b": None, "a": None}
effigies commented 4 years ago

I interpreted

You don't want me to complete with Nones when the number of defined outputs doesn't match the one function returns

as meaning that this should not happen:

@pydra.mark.task
@pydra.mark.annotate({"return": {"a": ty.Any, "b": ty.Any}})
def test_multiout(val, val2):
    return 1
task2.result().output === {"a": 1, "b": None}
satra commented 4 years ago

this situation should raise exception if only 1 value is returned, i think.

task2.result().output === {"a": 1, "b": None}

not completely set on all the possibilities, so worthwhile discussing

djarecka commented 4 years ago

yes, I was wondering about the situation described by @effigies

I'd vote to return an error, at least for now. it's easier for me to think about situations when not rising error might be more confusing

satra commented 4 years ago

i see the following possibilities for n_outputs > 1

  1. number of elements in returned tuple == n_outputs map appropriately 2a. number of elements in returned tuple < n_outputs map and set remainder to None (@effigies suggestion) 2b. number of elements in returned tuple < n_outputs raise Exception - current situation

i'm ok with going with 1 and 2a above. This should also handle None appropriately.

djarecka commented 4 years ago

@satra - i think i agree, but can you confirm if these are the behaviors you expect:

satra commented 4 years ago

multiple output matches with 2b, and we are saying we may want to move to 2a.

djarecka commented 4 years ago

I've got lost with this purely verbal communication - i didn't think that @effigies suggested this... I'm not convinced that 2a is better than 2b, but could change for now.

satra commented 4 years ago

@djarecka - you are probably correct. how about i leave it to you and @effigies to decide. i'm not strongly in favor of 2a vs 2b.

djarecka commented 4 years ago

i will merge for now my pr, we can always come back to it later, when we decide that 2a is a better approach

effigies commented 4 years ago

Sorry for not getting back to this. I agree with @djarecka (I'm pretty sure).

In the case where multiple values are expected and only one is returned: If that value is None, it is expanded to all expected fields. If that value is anything else, then it is a RuntimeError.