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

Third-party tests failed on Sat Jun 10 2023 #230

Closed
github-actions bot opened this issue Jun 10, 2023 · 11 comments · Fixed by #234
Closed

Third-party tests failed on Sat Jun 10 2023 #230

github-actions bot opened this issue Jun 10, 2023 · 11 comments · Fixed by #234

Comments

@github-actions
Copy link

Runs listed here: https://github.com/python/typing_extensions/actions/workflows/third_party.yml

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 10, 2023

Looks like making typing_extensions.TypedDict a function may have broken cattrs on Python 3.8 (cc. @JelleZijlstra)

https://github.com/python/typing_extensions/actions/runs/5227707943/jobs/9439548290

AlexWaygood added a commit to AlexWaygood/typing_extensions that referenced this issue Jun 10, 2023
Haven't had a chance to look into the exact cause of the failures in python#230, but here's a stopgap PR
@AlexWaygood
Copy link
Member

#231 is a stopgap PR

srittau pushed a commit that referenced this issue Jun 11, 2023
Temporary workaround for the the failures in #230.
@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 11, 2023

I tried to figure out what was going on here. I confirmed that the cattrs failures on Python 3.8 bisect to d826561, but wasn't able to get much further than that :(

@Tinche, any idea what might be causing the cattrs failures this time? :) (Sorry to keep tagging you on these! At least we haven't released a version of typing_extensions that breaks cattrs this time ;)

@Tinche
Copy link

Tinche commented Jun 11, 2023

Sure, I'm happy to take a look. Gimme 24 hours 😉

@JelleZijlstra
Copy link
Member

I haven't had a chance to look either but possibly relevant is that my PR changes the behavior of is_typeddict(TypedDict) in some cases. (It should return False, but previously returned True sometimes.)

@Tinche
Copy link

Tinche commented Jun 11, 2023

I found the issue.

A change in typing_extensions changed the behavior of the following snippet:

from typing_extensions import TypedDict

from cattrs._compat import is_typeddict

print(is_typeddict(TypedDict))

Before it used to return True, now it returns False.

(Here's the helper function:

  def is_typeddict(cls) -> bool:
      return (
          cls.__class__ is _TypedDictMeta
          or (is_generic(cls) and (cls.__origin__.__class__ is _TypedDictMeta))
          or (
              ExtensionsTypedDictMeta is not None
              and cls.__class__ is ExtensionsTypedDictMeta
              or (
                  is_generic(cls)
                  and (cls.__origin__.__class__ is ExtensionsTypedDictMeta)
              )
          )
      )

)

That said, this actually exposed a bug in the Hypothesis strategy cattrs uses to generate TypedDicts. It broke the following case:

from cattrs import structure
from typing_extensions import TypedDict

structure({}, TypedDict)

But that use-case is non-sensical anyway. You're supposed to structure subclasses of TypedDict, not TypedDicts directly. I've pushed a fix to cattrs main to stop doing this.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 12, 2023

A change in typing_extensions changed the behavior of the following snippet:

from typing_extensions import TypedDict

from cattrs._compat import is_typeddict

print(is_typeddict(TypedDict))

Before it used to return True, now it returns False.

Yeah, I think that's probably desirable behaviour? Part of the motivation for the refactor was so that we could have typing_extensions.is_typeddict(typing_extensions.TypedDict) return False, on the basis that TypedDict itself is not actually a TypedDict class, it's just a factory for creating TypedDict classes. It makes sense that the change would have the same effect on cattrs's implementation of is_typedict.

(I'm curious why you reimplement is_typeddict in cattrs btw, rather than using typing(_extensions).typeddict. Are there some things we're missing in our implementation? :)

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 12, 2023

I've pushed a fix to cattrs main to stop doing this.

Can confirm this gets everything green again (https://github.com/python/typing_extensions/actions/runs/5238165985/jobs/9456790264?pr=234). Thanks so much for the quick fix @Tinche!

@Tinche
Copy link

Tinche commented Jun 12, 2023

(I'm curious why you reimplement is_typeddict in cattrs btw, rather than using typing(_extensions).typeddict. Are there some things we're missing in our implementation? :)

Historically, cattrs has been very lax with versions of typing_extensions it depends on, and maybe a secondary goal is to not even depend on typing_extensions on newest Python versions (for example, we don't depend on it on 3.11 currently). So I just made do with carrying a small _compat module which contains just enough of this logic to pass my tests. I think in some cases I was able to make my versions faster since they can be optimized for my exact use-cases, but at a certain point cattrs was rearchitected to not do any inspection on hot paths anyway, so this is probably moot.

I'm not super attached to this approach, and would be willing to reconsider in the future. It's a duplication of effort for sure, and you folks are likely to have better versions anyway.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 12, 2023

Here's a proof-of-concept for simplifying cattrs._compat.is_typeddict that wouldn't require any changes to your current typing-extensions dependency (and also wouldn't break if you dropped the typing-extensions dependency on Python 3.10): AlexWaygood/cattrs@bf4f1fe.

Unfortunately it causes a test failure on 3.11, and I can't say I really understand the cause:

Test traceback
=================================== FAILURES ===================================
  ________________________________ test_generics _________________________________
  
      @pytest.mark.skipif(not is_py311_plus, reason="3.11+ only")
  >   @given(generic_typeddicts(total=True), booleans())
  
  f          = <function given.<locals>.run_test_as_given.<locals>.wrapped_test at 0x7f1bcd1a2660>
  
  tests/test_typeddicts.py:[148](https://github.com/AlexWaygood/cattrs/actions/runs/5246225332/jobs/9474753687#step:5:149): 
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  tests/test_typeddicts.py:[164](https://github.com/AlexWaygood/cattrs/actions/runs/5246225332/jobs/9474753687#step:5:165): in test_generics
      restructured = c.structure(unstructured, cls)
          c          = <cattrs.converters.Converter object at 0x7f1bb03276a0>
          cls        = HypTypedDict[typing.List[int]]
          cls_and_instance = (HypTypedDict[typing.List[int]], {'a': []})
          detailed_validation = False
          instance   = {'a': []}
          unstructured = {'a': []}
  src/cattrs/converters.py:334: in structure
      return self._structure_func.dispatch(cl)(obj, cl)
          cl         = HypTypedDict[typing.List[int]]
          obj        = {'a': []}
          self       = <cattrs.converters.Converter object at 0x7f1bb03276a0>
  :2: in structure_mapping
      ???
          _          = HypTypedDict[typing.List[int]]
          mapping    = {'a': []}
  :2: in <dictcomp>
      ???
          .0         = <dict_itemiterator object at 0x7f1bb03e2a20>
          k          = 'a'
          v          = []
  src/cattrs/converters.py:550: in _structure_list
      res = [handler(e, elem_type) for e in obj]
          cl         = typing.List[int]
          elem_type  = <class 'int'>
          handler    = <function BaseConverter._structure_call at 0x7f1bcd6793a0>
          obj        = 'a'
          self       = <cattrs.converters.Converter object at 0x7f1bb03276a0>
  src/cattrs/converters.py:550: in <listcomp>
      res = [handler(e, elem_type) for e in obj]
          .0         = <str_ascii_iterator object at 0x7f1bb0208cd0>
          e          = 'a'
          elem_type  = <class 'int'>
          handler    = <function BaseConverter._structure_call at 0x7f1bcd6793a0>
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  
  obj = 'a', cl = <class 'int'>
  
      @staticmethod
      def _structure_call(obj: Any, cl: Type[T]) -> Any:
          """Just call ``cl`` with the given ``obj``.
      
          This is just an optimization on the ``_structure_default`` case, when
          we know we can skip the ``if`` s. Use for ``str``, ``bytes``, ``enum``,
          etc.
          """
  >       return cl(obj)
  E       ValueError: invalid literal for int() with base 10: 'a'
  E       Falsifying example: test_generics(
  E           cls_and_instance=(HypTypedDict[typing.List[int]], {'a': []}),
  E           detailed_validation=False,
  E       )
  E       
  E       You can reproduce this example by temporarily adding @reproduce_failure('6.76.0', b'AAEBAAAAAAA=') as a decorator on your test case
  
  cl         = <class 'int'>
  obj        = 'a'
  
  src/cattrs/converters.py:441: ValueError
  =========================== short test summary info ============================
  FAILED tests/test_typeddicts.py::test_generics - ValueError: invalid literal for int() with base 10: 'a'
  Falsifying example: test_generics(
      cls_and_instance=(HypTypedDict[typing.List[int]], {'a': []}),
      detailed_validation=False,
  )
  
  You can reproduce this example by temporarily adding @reproduce_failure('6.76.0', b'AAEBAAAAAAA=') as a decorator on your test case

Happy to open a PR anyway if it's helpful, though :)

@Tinche
Copy link

Tinche commented Jun 13, 2023

Sure, open it up, always happy to foster contributors ;)

The issue seems to be with generic TypedDicts. Can take a deeper look over there if you end up doing it.

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

Successfully merging a pull request may close this issue.

3 participants