lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
401 stars 50 forks source link

Breaks with `from __future__ import annotations` #69

Closed rggjan closed 1 year ago

rggjan commented 3 years ago

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...

lebrice commented 3 years ago

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.

orsinium commented 3 years ago

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.

Conchylicultor commented 3 years ago

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

lebrice commented 2 years ago

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
  3. 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 commented 1 year ago

Solved by #111