Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypingHintArgSpecGuesser breaks on type alias List #216

Closed
thorwhalen opened this issue Jan 15, 2024 · 11 comments
Closed

TypingHintArgSpecGuesser breaks on type alias List #216

thorwhalen opened this issue Jan 15, 2024 · 11 comments
Assignees
Labels
Milestone

Comments

@thorwhalen
Copy link

Sorry not to give further details but I had the choice between sending a quick issue or not mentioning it at all.

My script, that uses argh, stopped working when I updated to argh==0.31.0 from argh==0.26.2.

The error

    argh.dispatch_command(populate_pkg_dir)
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/dispatching.py", line 470, in dispatch_command
    set_default_command(parser, function, name_mapping_policy=name_mapping_policy)
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 420, in set_default_command
    inferred_args: List[ParserAddArgumentSpec] = list(
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 183, in infer_argspecs_from_function
    TypingHintArgSpecGuesser.typing_hint_to_arg_spec_params(
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 778, in typing_hint_to_arg_spec_params
    item_type = cls._extract_item_type_from_list_type(first_subtype)
  File "~.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 799, in _extract_item_type_from_list_type
    if args[0] in cls.BASIC_TYPES:
IndexError: tuple index out of range

My guess is that the len(args) == 0 case is not handled properly?

Without looking further, I'd suggest:

 if args and (args[0] in cls.BASIC_TYPES):

instead?

If I have time, I'll look at it further this week and send a PR.

@thorwhalen thorwhalen added the bug label Jan 15, 2024
thorwhalen added a commit to thorwhalen/argh that referenced this issue Jan 15, 2024
Replaced 

```python
 if args and (args[0] in cls.BASIC_TYPES):
```

with

```python
 if args and (args[0] in cls.BASIC_TYPES):
```
@thorwhalen
Copy link
Author

I went ahead and made a PR for this: #217

@neithere neithere added this to the 0.31.1 milestone Jan 15, 2024
@neithere neithere changed the title argh==0.31.0 breaks (my) argh.dispatch_command call TypingHintArgSpecGuesser breaks on type alias List Jan 16, 2024
@neithere
Copy link
Owner

Minimal example as discussed in the related PR:

  • def func(xs: list) — works
  • def func(xs: List) — breaks

@thorwhalen
Copy link
Author

@neithere : This still breaks for me with argh==0.31.1:

from typing import Optional, List

def func(x: Optional[List] = None):
        return 'hello'

if __name__ == '__main__':
    import argh

    argh.dispatch_command(func)

Note that replacing Optional[List] with Optional[list] or just List works though.

  File "~/tmp/scrap.py", line 9, in <module>
    argh.dispatch_command(func)
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/dispatching.py", line 470, in dispatch_command
    set_default_command(parser, function, name_mapping_policy=name_mapping_policy)
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 420, in set_default_command
    inferred_args: List[ParserAddArgumentSpec] = list(
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 183, in infer_argspecs_from_function
    TypingHintArgSpecGuesser.typing_hint_to_arg_spec_params(
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 778, in typing_hint_to_arg_spec_params
    item_type = cls._extract_item_type_from_list_type(first_subtype)
  File "~/.pyenv/versions/p10/lib/python3.10/site-packages/argh/assembling.py", line 799, in _extract_item_type_from_list_type
    if args[0] in cls.BASIC_TYPES:
IndexError: tuple index out of range

@neithere
Copy link
Owner

Ahh, you're right. I fixed a more general case but it was not enough.

Will ship the fix with 0.31.2 later today.

BTW, we'll get rid of those aliases (List etc.) when we drop support for Python 3.8 (pretty soon), they are deprecated since Python 3.9, so I'd really try to use Optional[list] or even list | None whenever possible.

@neithere neithere reopened this Jan 23, 2024
@neithere neithere modified the milestones: 0.31.1, 0.31.2 Jan 23, 2024
@thorwhalen
Copy link
Author

@neithere: I see you're active, so let me help you out with this.

You might want to use this to make a more significant test:

def issue_216_happens_annotations(func, annotation):
    """
    Util to test what annotations make the 
        https://github.com/neithere/argh/issues/216 
    issue happen
    """
    import argh
    func.__annotations__['x'] = annotation
    try:
        argh.dispatch_command(func)
    except IndexError as e:
        if e.args[0] == 'tuple index out of range':
             return True
    except BaseException:
         pass
    return False

# Example:

from typing import Optional, List
from functools import partial

def func(x = None):
        return 'hello'

there_is_an_issue = partial(issue_216_happens_annotations, func)

test_types = [List, Optional[list], Optional[List], Optional[List[int]]]

list(map(there_is_an_issue, test_types))
# [False, False, True, False]

@thorwhalen
Copy link
Author

Combine this with this and you'll get some monster coverage:

def type_combos(generic_types, type_variables):
    """
    Generate "generic" using combinations of types such as 
        `Optional[List], Dict[Tuple, List], Callable[[List], Dict]`
    from a list of generic types such as `Optional`, `List`, `Dict`, `Callable`
    and a list of type variables that are used to parametrize these generic types.
    
    :param generic_types: A list of generic types
    :param type_variables: A list of type variables
    :return: A generator that yields generic types

    >>> from typing import Optional, Dict, Tuple, List, Callable
    >>> list(type_combos([Optional, Tuple], [list, dict]))  # doctest: +NORMALIZE_WHITESPACE
    [typing.Optional[list], typing.Optional[dict], 
    typing.Tuple[list, dict], typing.Tuple[dict, list]]

    More significant example:

    >>> generic_types = [Optional, Callable, Dict, Tuple]
    >>> type_variables = [tuple, dict, List]
    >>>
    >>> for combo in type_combos(generic_types, type_variables):
    ...     print(combo)
    ...
    typing.Optional[tuple]
    typing.Optional[dict]
    typing.Optional[typing.List]
    typing.Callable[[typing.Tuple[dict, ...]], tuple]
    typing.Callable[[typing.Tuple[typing.List, ...]], tuple]
    typing.Callable[[typing.Tuple[dict, typing.List]], tuple]
    typing.Callable[[typing.Tuple[typing.List, dict]], tuple]
    typing.Callable[[typing.Tuple[tuple, ...]], dict]
    typing.Callable[[typing.Tuple[typing.List, ...]], dict]
    typing.Callable[[typing.Tuple[tuple, typing.List]], dict]
    typing.Callable[[typing.Tuple[typing.List, tuple]], dict]
    typing.Callable[[typing.Tuple[tuple, ...]], typing.List]
    typing.Callable[[typing.Tuple[dict, ...]], typing.List]
    typing.Callable[[typing.Tuple[tuple, dict]], typing.List]
    typing.Callable[[typing.Tuple[dict, tuple]], typing.List]
    typing.Dict[tuple, dict]
    typing.Dict[tuple, typing.List]
    typing.Dict[dict, tuple]
    typing.Dict[dict, typing.List]
    typing.Dict[typing.List, tuple]
    typing.Dict[typing.List, dict]
    typing.Tuple[tuple, dict, typing.List]
    typing.Tuple[tuple, typing.List, dict]
    typing.Tuple[dict, tuple, typing.List]
    typing.Tuple[dict, typing.List, tuple]
    typing.Tuple[typing.List, tuple, dict]
    typing.Tuple[typing.List, dict, tuple]
    """
    def generate_combos(generic_type, remaining_vars):
        from itertools import permutations
        from typing import Callable, Dict, Tuple
        
        if generic_type is Callable:
            # Separate one variable for the output type
            for output_type in remaining_vars:
                input_vars = [var for var in remaining_vars if var != output_type]
                # Generate combinations of input types
                for n in range(1, len(input_vars) + 1):
                    for input_combo in permutations(input_vars, n):
                        # Format single-element tuples correctly
                        if len(input_combo) == 1:
                            input_type = Tuple[input_combo[0], ...]
                        else:
                            input_type = Tuple[input_combo]
                        yield Callable[[input_type], output_type]
        elif generic_type in [Dict, Tuple]:
            required_params = 2 if generic_type is Dict else len(remaining_vars)
            for combo in permutations(remaining_vars, required_params):
                yield generic_type[combo]
        else:
            for type_var in remaining_vars:
                yield generic_type[type_var]
    for generic_type in generic_types:
        yield from generate_combos(generic_type, type_variables)

@thorwhalen
Copy link
Author

The good news is that, trying it out on my side, I find that, out of 109776 combinations tested, the only one that failed was our good old `Optional[List]'!

I'll do a PR to add these tests.

@thorwhalen
Copy link
Author

@neithere -- made a PR containing my code to increase the test coverage for this problem.
Uses (further edited) versions of my code I pasted above, plus a test function to apply it to argh.dispatch over a large (100K+) number of type combinations.

#219

@neithere
Copy link
Owner

@thorwhalen oh wow, thanks, that's a lot going on there :) I'll need some time to read the code itself.

I've enabled CI for that PR and it's failing with some strange error messages (and lots of them!). I don't have time to dig deep into that at the moment, so I think that for now I'll just rely on the results of your research and assume that the only known problematic combination is fixed, and therefore the fix can be shipped. That's a great result already.

Later I'll need to think how to ensure optimal test coverage for the upcoming Argh 1.0 (it won't support type aliases like List, BTW). At the moment I'm leaning towards writing tests specifically for what Argh knows how to interpret, and so far it's a pretty limited set of types. It also won't go too deep inside of those because of CLI limitations (e.g. pythonic foo: list[str] is foo, [foo ...] in the CLI world, but what could foo: list[list[str]] mean in CLI? how would you express a dict in a set of CLI arguments?). Of course we also need to ensure that the library doesn't crash on whatever it does not support, so a generator of permutations might be useful indeed, and I guess that's exactly the problem that your PR addresses.

@neithere
Copy link
Owner

P.S.: FYI, I'm trying to comment on the latest developments in typing hints support in #107 :)

@thorwhalen
Copy link
Author

@thorwhalen oh wow, thanks, that's a lot going on there :) I'll need some time to read the code itself.

I've enabled CI for that PR and it's failing with some strange error messages (and lots of them!). I don't have time to dig deep into that at the moment, so I think that for now I'll just rely on the results of your research and assume that the only known problematic combination is fixed, and therefore the fix can be shipped. That's a great result already.

Later I'll need to think how to ensure optimal test coverage for the upcoming Argh 1.0 (it won't support type aliases like List, BTW). At the moment I'm leaning towards writing tests specifically for what Argh knows how to interpret, and so far it's a pretty limited set of types. It also won't go too deep inside of those because of CLI limitations (e.g. pythonic foo: list[str] is foo, [foo ...] in the CLI world, but what could foo: list[list[str]] mean in CLI? how would you express a dict in a set of CLI arguments?). Of course we also need to ensure that the library doesn't crash on whatever it does not support, so a generator of permutations might be useful indeed, and I guess that's exactly the problem that your PR addresses.

Yes. Just wanted to see what a test covering a lot more combinations would look like. Just a proposal. Not essential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants