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

looker-sdk 21.20.0 throws TypeError for python 3.9.10 #944

Closed
rowedonalde opened this issue Jan 19, 2022 · 8 comments · Fixed by #1015
Closed

looker-sdk 21.20.0 throws TypeError for python 3.9.10 #944

rowedonalde opened this issue Jan 19, 2022 · 8 comments · Fixed by #1015
Labels
bug Regression or bug

Comments

@rowedonalde
Copy link

Due to a change in Python's functools module that was included in the update from 3.9.9->3.9.10, looker_sdk breaks on import. The error seems to be through the cattr dependency:

  File "/opt/app/src/tvsapi/views.py", line 8, in <module>
    import looker_sdk
  File "/usr/local/lib/python3.9/site-packages/looker_sdk/__init__.py", line 32, in <module>
    from looker_sdk.sdk.api31 import methods, methods as methods31  # noqa:F401
  File "/usr/local/lib/python3.9/site-packages/looker_sdk/sdk/api31/methods.py", line 32, in <module>
    from . import models as mdls
  File "/usr/local/lib/python3.9/site-packages/looker_sdk/sdk/api31/models.py", line 11935, in <module>
    sr.converter31.register_structure_hook(
  File "/usr/local/lib/python3.9/site-packages/cattr/converters.py", line 186, in register_structure_hook
    self._structure_func.register_cls_list([(cl, func)])
  File "/usr/local/lib/python3.9/site-packages/cattr/multistrategy_dispatch.py", line 45, in register_cls_list
    self._single_dispatch.register(cls, handler)
  File "/usr/local/lib/python3.9/functools.py", line 855, in register
      raise TypeError(
TypeError: Invalid first argument to `register()`. ForwardRef('AccessToken') is not a class.
@rowedonalde
Copy link
Author

Relevant discussion in cattrs from others who have experienced this outside of looker_sdk: python-attrs/cattrs#206

@ps-jmorrow
Copy link

We're experiencing this issue as well and it's blocking our release via GHA.

@joeldodge79
Copy link
Contributor

@ps-jmorrow @rowedonalde Can you pin python at <= 3.9.9 as a workaround till I can take a look?

@ps-jmorrow
Copy link

That worked as a workaround, thanks.

@jkaster jkaster added the bug Regression or bug label Jan 21, 2022
@joeldodge79
Copy link
Contributor

I think for now we need to restrict to python_requires=">=3.6, <=3.9.9"

@jeremytchang
Copy link
Collaborator

jeremytchang commented Jan 25, 2022

Have you all been able to pull in latest with the PR change and confirm the issue is fixed?

@alekseiloginov
Copy link
Contributor

alekseiloginov commented Jan 27, 2022

I just got some details from cattrs folks on what's need to be done for us to avoid this issue and to stop restricting the python version to <=3.9.9: python-attrs/cattrs#206 (comment)

@joeldodge79 Any idea how much of an effort this might be?

@joeldodge79
Copy link
Contributor

probably a few hours to check if switching to register_structure_hook_func will work for our cases like

python/looker_sdk/sdk/api40/models.py:sr.converter40.register_structure_hook(
python/looker_sdk/sdk/api40/models.py-    ForwardRef("AccessToken"),  # type: ignore
python/looker_sdk/sdk/api40/models.py-    forward_ref_structure_hook,  # type:ignore
python/looker_sdk/sdk/api40/models.py-)

and then another few hours to update packages/sdk-codegen/src/python.gen.ts and looker_sdk.rtl.serialize.forward_ref_structure_hook

so maybe a day or two's worth of work?

joeldodge79 added a commit that referenced this issue Mar 4, 2022
fixes #944

the main fix here is switching from `cattr.register_structure_hook` to
`cattr.register_structure_hook_func`. I re-organized some of the code
in an effort to reproduce the issue in the test_serialize.py suite but
I could not reproduce there, only in integration tests.

Now that it's a function, we don't need a generated hook per type.

I further isolated the converter usage switching the serializing to use
each 31/40 dedicated converter.

I also switched to the newer GenConverter.

setup testing against python 3.10
joeldodge79 added a commit that referenced this issue Mar 4, 2022
fixes #944

the main fix here is switching from `cattr.register_structure_hook` to
`cattr.register_structure_hook_func`. I re-organized some of the code
in an effort to reproduce the issue in the test_serialize.py suite but
I could not reproduce there, only in integration tests.

Now that it's a function, we don't need a generated hook per type.

I further isolated the converter usage switching the serializing to use
each 31/40 dedicated converter.

I also switched to the newer GenConverter.

setup testing against python 3.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Regression or bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants