nipype / pydra

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

TypeError: Cannot coerce {'mandatory': True} into <class 'str'> #740

Open ghisvail opened 6 months ago

ghisvail commented 6 months ago

This snippet used to work with version 0.22:

from pydra.engine import Worflow
from pydra.engine.specs import BaseSpec, SpecInfo

input_spec = SpecInfo(name="Input", fields=[("T1w", str, {"mandatory": True})], bases=(BaseSpec,))
workflow = Workflow(name="foo", input_spec=input_spec)

But fails with version 0.23 with a coercion error:

TypeError: Cannot coerce {'mandatory': True} into <class 'str'> in 'T1w' field of Input as the coercion doesn't match any of the explicit inclusion criteria: Sequence -> Sequence, Mapping -> Mapping, Path -> PathLike, str -> PathLike, PathLike -> Path, PathLike -> str, Any -> MultiInputObj, int -> float, Integer -> float, int -> Decimal, Boolean -> bool, Decimal -> float, Integer -> int, Text -> str, bool -> Boolean, float -> Decimal, int -> Integer, str -> Text, integer -> int, floating -> float, bool_ -> bool, integer -> float, character -> str, complexfloating -> complex, bytes_ -> bytes, ndarray -> Sequence, Sequence -> ndarray

Suggesting that fields with metadata are incorrectly fed to the type coercion functionality.

ghisvail commented 6 months ago

Okay, here the metadata dictionary is wrongly passed as the default value.

ghisvail commented 6 months ago

Looks like the make_klass helper uses the presence of the help_string key to distinguish between a field defined with a triple (name, type, default) and (name, type, metadata), hence the bug.

ghisvail commented 6 months ago

It does not look like the logic in make_klass has changed much with this commit, so it was probably discarded silently somewhere further in the code before 🤔

Not sure what to make of this bug. Will adjust the offending test in Clinica meanwhile.

tclose commented 6 months ago

I think this is just a case of type-checking uncovering an existing bug.

I have run into the missing help_string -> 3rd arg == default issue before and it is pretty confusing. I reckon we should have a look at changing that behaviour if we end up making any changes to the API

ghisvail commented 6 months ago

Supporting multiple triples whose content of the last attribute decides what it means, is a recipe for disaster imo. Since metadata is pretty much required to provide help_string, then the only sensible choices may be:

Maybe?