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

Add attrs lower bound in requirements.txt #6633

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Add attrs lower bound in requirements.txt #6633

merged 2 commits into from
Jun 11, 2024

Conversation

ryanhill1
Copy link
Contributor

$ pip install cirq-core
$ pip install 'attrs==21.2.0'
$ python
>>> import cirq
...
File ~/anaconda3/envs/py312/lib/python3.12/site-packages/cirq/transformers/gauge_compiling/gauge_compiling.py:23
     20 import functools
     22 from dataclasses import dataclass
---> 23 from attrs import frozen, field
     24 import numpy as np
     26 from cirq.transformers import transformer_api

ModuleNotFoundError: No module named 'attrs'

$ pip install 'attrs==21.3.0'
$ python
>>> import cirq
>>> # works

Any attrs<21.3.0 fails for me when I try to import cirq. And any attrs>=21.3.0 seems to work.

Reference:

@ryanhill1 ryanhill1 requested review from vtomole, cduck and a team as code owners June 4, 2024 00:40
@ryanhill1 ryanhill1 requested a review from pavoljuhas June 4, 2024 00:41
@CirqBot CirqBot added the Size: XS <10 lines changed label Jun 4, 2024
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Unfortunately, this breaks the pip dependency resolver and the ci tests even whilst uv pip compile dev_tools/requirements/dev.env.txt can find a viable set of packages.

Installation of cirq-core to a fresh virtual environment brings in the latest attrs-23.2.0.
Installation of the whole cirq suite brings attrs-21.4.0 - both of these cases satisfy the required minimum version.

I suggest to hold this for later until after the merge of #6281 which will loosen the maximum attrs version from a transitive dependency.

@pavoljuhas pavoljuhas mentioned this pull request Jun 4, 2024
32 tasks
@NoureldinYosri
Copy link
Collaborator

I don't see how ModuleNotFoundError: No module named 'attrs' could be caused by an old attrs version?! ... if the error was that it can't find one of the functions/classes of the library then it would have made sense ... but module not found is a different thing

@maffoo
Copy link
Contributor

maffoo commented Jun 4, 2024

@NoureldinYosri, see the attrs changelog. While the package on pypi has always been called attrs so that is what you would install (pip install attrs), the python package that you would import was previously called attr (import attr). This was confusing, so they added an attrs python package which matches the install name, and this first landed in 21.3.0.

@quantumlib quantumlib deleted a comment Jun 8, 2024
@pavoljuhas pavoljuhas enabled auto-merge (squash) June 11, 2024 21:24
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (5f7881f) to head (f55d42e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6633      +/-   ##
==========================================
- Coverage   97.81%   97.81%   -0.01%     
==========================================
  Files        1067     1066       -1     
  Lines       91547    91693     +146     
==========================================
+ Hits        89546    89687     +141     
- Misses       2001     2006       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@pavoljuhas pavoljuhas merged commit 72f0542 into quantumlib:main Jun 11, 2024
34 checks passed
@ryanhill1 ryanhill1 deleted the ryanhill1-patch-1 branch June 11, 2024 23:22
Yash-10 pushed a commit to Yash-10/Cirq that referenced this pull request Jun 12, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
vmscw pushed a commit to Sonderfall/Cirq that referenced this pull request Jun 14, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
pavoljuhas pushed a commit to pavoljuhas/Cirq that referenced this pull request Jun 26, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
pavoljuhas pushed a commit to pavoljuhas/Cirq that referenced this pull request Jun 26, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
pavoljuhas pushed a commit to pavoljuhas/Cirq that referenced this pull request Jun 26, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Require attrs>=21.3.0 to ensure `import attrs` works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants