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

[Python] Bump attrs, cattrs versions for Python 3.7+ #374

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

setu4993
Copy link
Contributor

@setu4993 setu4993 commented Nov 5, 2020

Ran into a weird edge case today where import for looker_sdk failed because of a version issue with attrs, which was on 19.3.0 but cattrs required attr>=20.1.0 (this line requires attrs > 20.1.0). Tracelog below, but running a pip install -U attrs fixed the issue. Bumping it up in here, along with the version (feel free to update the version appropriately if that's an error on my end).

Tracelog:

...
/opt/conda/lib/python3.7/site-packages/looker_sdk/__init__.py in <module>
     25 from looker_sdk.rtl import api_settings
     26 from looker_sdk.rtl import requests_transport
---> 27 from looker_sdk.rtl import serialize
     28 from looker_sdk.rtl import auth_session
     29 

/opt/conda/lib/python3.7/site-packages/looker_sdk/rtl/serialize.py in <module>
     37 )
     38 
---> 39 import cattr
     40 
     41 from looker_sdk.rtl import model

/opt/conda/lib/python3.7/site-packages/cattr/__init__.py in <module>
----> 1 from .converters import Converter, GenConverter, UnstructureStrategy
      2 from .gen import override
      3 
      4 __all__ = (
      5     "global_converter",

/opt/conda/lib/python3.7/site-packages/cattr/converters.py in <module>
     14 )
     15 
---> 16 from attr import fields, resolve_types
     17 
     18 from ._compat import (

ImportError: cannot import name 'resolve_types' from 'attr' (/opt/conda/lib/python3.7/site-packages/attr/__init__.py)

@joeldodge79
Copy link
Contributor

thanks for finding this. We need to maintain support for python 3.6 and it looks like that was dropped in cattrs 1.1.0 so we'll need to either pin cattrs == 1.0.0 or find a fancy way of switching based on python version (similar to how python-dateutil and typing-extensions are added)

@joeldodge79 joeldodge79 self-requested a review November 5, 2020 18:08
Copy link
Contributor

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have cycles to figure out allowing higher cattrs versions conditional on python version that would be much appreciated! Otherwise for now let's just pin it to 1.0.0 so new installations aren't broken

@setu4993
Copy link
Contributor Author

setu4993 commented Nov 5, 2020

Thanks Joel! I can get this in place later today.

@setu4993 setu4993 requested a review from joeldodge79 November 6, 2020 06:33
@setu4993
Copy link
Contributor Author

setu4993 commented Nov 6, 2020

@joeldodge79 : Done. Tested installing on my brand new venvs on 36 and 37, and the right versions get installed on both.

@setu4993 setu4993 changed the title [Python] Bump attrs version [Python] Bump attrs, caters versions for Python 3.7+ Nov 6, 2020
Copy link
Contributor

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you! I'll get this published shortly

@joeldodge79 joeldodge79 merged commit a80ccd3 into looker-open-source:master Nov 6, 2020
@setu4993
Copy link
Contributor Author

setu4993 commented Nov 6, 2020

Thanks Joel!

@setu4993 setu4993 deleted the fixup/python-attrs branch November 6, 2020 18:19
@joeldodge79
Copy link
Contributor

https://pypi.org/project/looker-sdk/0.1.3b20/ is up

@setu4993 setu4993 changed the title [Python] Bump attrs, caters versions for Python 3.7+ [Python] Bump attrs, cattrs versions for Python 3.7+ Nov 6, 2020
This was referenced Mar 24, 2021
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 this pull request may close these issues.

2 participants