SimpleParsing icon indicating copy to clipboard operation
SimpleParsing copied to clipboard

Breaks with `from __future__ import annotations`

Open rggjan opened this issue 3 years ago • 4 comments

When using from __future__ import annotations at the top of any of the examples, SimpleParsing crashes with:

...
  File "/Users/jrueegg/miniconda/envs/shrek/lib/python3.7/site-packages/simple_parsing/parsing.py", line 221, in _preprocessing
    wrapper.add_arguments(parser=self)
  File "/Users/jrueegg/miniconda/envs/shrek/lib/python3.7/site-packages/simple_parsing/wrappers/dataclass_wrapper.py", line 103, in add_arguments
    group.add_argument(*wrapped_field.option_strings, **wrapped_field.arg_options)
  File "/Users/jrueegg/miniconda/envs/shrek/lib/python3.7/argparse.py", line 1364, in add_argument
    raise ValueError('%r is not callable' % (type_func,))
ValueError: 'str' is not callable

I guess you're missing a typing.get_type_hints() somewhere in the code...

rggjan avatar Jun 25 '21 06:06 rggjan

Good point! I think the codebase currently heavily depends on the annotations being evaluated (e.g. on the type attribute of the dataclasess.Field objects to be a 'live' type (<class str> and not just "str")) so using this postponed evaluation option (which I'll admit I'm not yet familiar with) might break a few things.. I'll try and give this a go and I'll get back to you.

lebrice avatar Jun 27 '21 03:06 lebrice

You most probably will find all answers in this discussion: https://github.com/samuelcolvin/pydantic/issues/2678

If I understand correctly, the lazy evaluation breaks too many things depending on reflection, so it was postponed. However, some simple cases can be evaluated by get_type_hints, it just won't work for local variables.

orsinium avatar Aug 30 '21 08:08 orsinium

Any update on this ? Independently of samuelcolvin/pydantic#2678 issue, I think using typing.get_type_hints() should works for most common cases.

Conchylicultor avatar Sep 09 '21 10:09 Conchylicultor

Hey @Conchylicultor , No updates yet, I'm quite busy with the ICLR deadline at the moment. I do plan to get to this at some point, but I could also always use some help!

In the meantime, here's a more detailed illustration of the problem, as well as my idea of how to solve it, in case anyone is kind enough to help me out with this:

from __future__ import annotations
from dataclasses import dataclass, fields


@dataclass
class Foo:
    a: int = 123
    
field_types = {f.name: f.type for f in fields(Foo)}
print(field_types) # would like to get {"a": int}, but instead get {"a": "int"}

Now, the bug with simple-parsing is that we rely on the field.type to be a "live" type, while it could instead be a string. Probably the best way of solving this, in my opinion, is to:

  1. Replace all references to self.field.type in the FieldWrapper, with a new attribute, perhaps something like self.field_type.
  2. Create a get_field_type function somewhere: (and test it)
def get_field_type(some_dataclass: Type[Dataclass], field: Field) -> Type:
    """ Retrieves the evaluated type annotation for the given field of this dataclass. """
    if isinstance(field.type, str):
         # todo: check what happens with forward refs here too.
         return get_type_hints(some_dataclass)[field.name]  # not 100% sure this is the right way to use this, but you get the point.
    return field.type
  1. Pass the value of this attribute to the constructor of the FieldWrapper when creating them inside the __post_init__ of the DataclassWrapper class. I think it's necessary to pass the value down, since you need the class to get the type annotations of the field, you couldn't get the type annotation from the field alone.

Hope this helps whoever reads this! :)

lebrice avatar Oct 03 '21 20:10 lebrice

Solved by #111

lebrice avatar Oct 20 '22 22:10 lebrice