Basic fix of GenericModel cache to detect order of arguments in Union models

[sveinugu]

Change Summary

Basic fix of GenericModel cache to include the order of arguments in Union models into the cache key.
This fixes the basic problem described in issue #4474, so that e.g. Model[Union[float, int]] is not used as a
blueprint for Model[Union[float, int]]. Added tests for basic and nested versions of the problem. Fixing the
nested problem depends on the fixing of the almost identical issue described in
python/cpython#86483 in CPython. The test was thus skipped, but kept for
documentation.

Related issue number

fix #4474

Checklist

• [ x ] Unit tests for the changes exist
• [ x ] Tests pass on CI and coverage remains at 100%
• [ x ] Documentation reflects the changes where applicable
• [ x ] changes/<pull request or issue id>-<github username>.md file added describing change
  (see changes/README.md for details.
  You can skip this check if the change does not need a change file.)
• [ x ] My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

[samuelcolvin]

otherwise LGTM, you just need to add "fix " to the issue ID in the PR body to satisfy the checks and also allow github to auto close the issue.

[samuelcolvin]

Just use, this is an internal method, so that's sufficient.

Suggested change

	def _cache_key(
	_params: Union[Type[Any], Tuple[Type[Any], ...]]
	) -> Tuple[Type[GenericModelT], Union[Type[Any], Tuple[Type[Any], ...]], Tuple[Any, ...]]:
	def _cache_key(_params: Any) -> Tuple[Type[GenericModelT], Any, Tuple[Any, ...]]:

[sveinugu]

Ah! Tried to replace with Type[Any] instead of Any but that did not work.

Also, I was wondering about closing the issue, as the nested variant of the issue is not fixed. Do you want to keep the issue open for this or is the documented and skipped test good enough?

[samuelcolvin]

I don't mind, if you want to keep the issue open, use the PR ID in the change file.

Either way this is urgent since people are (understandably) waiting on the patch release.

I just want this to pass so I can merge it and get the release done.

[sveinugu]

Added 'fix'. You can reopen the issue after, if needed.

[samuelcolvin]

great, thanks.

[samuelcolvin]

LGTM.

@PrettyWood please review, you happy with this?

[PrettyWood]

LGTM! Thanks

[samuelcolvin]

Thanks so much @sveinugu.

[sveinugu]

Thanks so much @sveinugu.

You're welcome! Happy to get the fix in. And thank you for creating such a useful library! Looking forward to v2.

[pepastach]

Hi @sveinugu @samuelcolvin

We recently bumped Pydantic from 1.10.1 to 1.10.2 and our tests started failing. After some lengthy investigation I found this change to be the root cause. I'd like to ask you for your opinions.

We have a Pydantic model with a field such as

f: MyGenericModel[Callable[[Task, Dict[str, Any]], Iterable[Result]]] = Field(...)

Just importing Python module with this code gives me:

cls = <class 'foo.MyGenericModel'>
params = typing.Callable[[foo.Task, typing.Dict[str, typing.Any]], typing.Iterable[foo.Result]]

    def __class_getitem__(cls: Type[GenericModelT], params: Union[Type[Any], Tuple[Type[Any], ...]]) -> Type[Any]:
        """Instantiates a new class from a generic class `cls` and type variables `params`.
    
        :param params: Tuple of types the class . Given a generic class
            `Model` with 2 type variables and a concrete model `Model[str, int]`,
            the value `(str, int)` would be passed to `params`.
        :return: New model class inheriting from `cls` with instantiated
            types described by `params`. If no parameters are given, `cls` is
            returned as is.
    
        """
    
        def _cache_key(_params: Any) -> Tuple[Type[GenericModelT], Any, Tuple[Any, ...]]:
            return cls, _params, get_args(_params)
    
>       cached = _generic_types_cache.get(_cache_key(params))
E       TypeError: unhashable type: 'list'

The problem seems to be that the function GenericModel._cache_key() now calls get_args() which in turns calls Python's typing.get_args() -> and this function returns a tuple with a list in it. And that makes it unhashable.

def get_args(tp):
    """Get type arguments with all substitutions performed.

    For unions, basic simplifications used by Union constructor are performed.
    Examples::
        get_args(Dict[str, int]) == (str, int)
        get_args(int) == ()
        get_args(Union[int, Union[T, int], str][int]) == (int, str)
        get_args(Union[int, Tuple[T, int]][str]) == (int, Tuple[str, int])
        get_args(Callable[[], T][int]) == ([], int)
    """
    if isinstance(tp, _GenericAlias) and not tp._special:
        res = tp.__args__
        if get_origin(tp) is collections.abc.Callable and res[0] is not Ellipsis:
            res = (list(res[:-1]), res[-1])             # <======== this list is a problem
        return res
    return ()

If I skip the callable typing and only define the field as

f: MyGenericModel[Callable] = Field(...)

then it runs okay.

Thanks in advance!

[sveinugu]

@pepastach Hmm... Seems we did not think of that. Surprised that typing uses lists and not tuples for Callable arguments, but there is probably a good reason for this (or it might be just that it would be too much a nuisance to switch between brackets and parentheses...). In any case, there might be other examples of unhashable return values from get_params, so a simple solution could be to just catch TypeError and in that case default to the previous _cache_key generation.

I am new as contributor to pydantic, but I assume it would be cleanest if you could create another issue for this.

[samuelcolvin]

Yes, new issue which references this PR.

But honestly, generics are one of the things that needs to be refactored properly in V2.

This patches-on-patches-on-patches are getting more and more complex - no one's fault, but I really hope we can get away from some of these anti-patterns in V2.

Anyway, apart from me whinging, this shouldn't be too hard to fix/work around as @sveinugu suggests.

[samuelcolvin]

Yes, new issue which references this PR.

I've creatd #4551