Pin all versions in requirements.txt to prevent against breakages #2648
Conversation
Cirq appears to be incompatible with the latest SymPy release of 1.5: https://github.com/sympy/sympy/releases/tag/sympy-1.5
|
If we do this, we should remove the part of We should also open an issue to fix Cirq to work with the latest sympy |
|
Done. Also opened #2650. |
|
@mpharrigan Why don't we want to use it directly? |
|
Please see the packaging.python.org link above, but in summary:
|
There was a problem hiding this comment.
Please only pin the major version (the first non-zero number). Otherwise we will create incompatibilities with other libraries. When the minor version changes and breaks us... we just have to take that and fix it, I think. If a particular library keeps doing it, we can pin it down for real.
|
Done. Is this change still desirable now that the breakage is fixed btw? Or do we want to just take breakages as they come and fix them on the fly? |
|
for reference: fixed by #2655 |
| # install_requires should not be overly specific. | ||
| # Change any == pinned deps to >=. | ||
| # https://packaging.python.org/discussions/install-requires-vs-requirements/ | ||
| requirements = [r.replace('==', '>=') for r in requirements] |
There was a problem hiding this comment.
Remove this change. Our requirements.txt file should be the literal source of truth. This also causes the code installed on user machines to be broken, e.g. it uses the bad version of sympy.
There was a problem hiding this comment.
So I suggested this change. I agree that as written it has sympy issues. In general, I'd advocate for never being overly specific on which versions are required for a developer-oriented library. Usually the way python packages can handle this is by pinning things in requirements.txt so e.g. CI runs under a consistent environment and users who don't care can pip install -r requirements.txt and get a good environment and leaving install_requires with a looser set of restrictions so pip install cirq has a low probability of upgrading or downgrading an existing install of a package (particularly heavy-weight ones like matplotlib or numpy).
If you insist on having one set of version specs for these two use cases, each restriction in requirements.txt should have a corresponding reason associated with it. If the reason is "there's a bug in the most recent release" then that's good and a future developer knows the condition under which it is safe to change the version spec. If the reason is "well I seem to have pandas 0.25.1 and I can't be bothered to check older or newer versions" ... that's less compelling
There was a problem hiding this comment.
If we are going to have reasons required for restrictions in requirements.txt, seems like this PR isn't useful anymore, because the SymPy stuff was fixed. I can revert if that's the direction we want to go in.
|
Forgot to revert my approval after @mpharrigan's comment. |
|
Seems like right now we will want to only pin requirements when necessary, and since SymPy is fixed there are nothing we need to actually pin. I'm going to revert this for now, but feel free to let me know if I should bring it back! |
|
Thanks @KevinVillela ! I think that's the right call for now |
Sympy v1.5.1 is now available, fixes sympy/sympy#18056. See https://github.com/sympy/sympy/releases/tag/sympy-1.5.1 & changelog: https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.1 Ref: #2648 #2655
Cirq appears to be incompatible with the latest SymPy release of 1.5: https://github.com/sympy/sympy/releases/tag/sympy-1.5. While we're here, might as well pin em all.
Fixes #2646
@vtomole This is a PR to replace #2647 because I messed up that PRs commit history something fierce.