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 requirement for more-itertools that works on python 2 #7296

Merged
merged 1 commit into from Mar 5, 2019

Conversation

Projects
None yet
4 participants
@cheister
Copy link
Contributor

commented Mar 1, 2019

Problem

The latest release of more-itertools (6.0.0) only works on Python 3. https://github.com/erikrose/more-itertools/releases. more-itertools is a requirement of pytest.

When running with python2 I get

  File "/tmp/.pants.d/test/pytest-prep/CPython-2.7.10/15d60b3366ef43c7a4ae6b5f5e2d7567a95fe5f4/.deps/pytest-3.6.4-py2.py3-none-any.whl/pytest.py", line 10, in <module>
    from _pytest.fixtures import fixture, yield_fixture
  File "/tmp/.pants.d/test/pytest-prep/CPython-2.7.10/15d60b3366ef43c7a4ae6b5f5e2d7567a95fe5f4/.deps/pytest-3.6.4-py2.py3-none-any.whl/_pytest/fixtures.py", line 8, in <module>
    from more_itertools import flatten
  File "/tmp/.pants.d/test/pytest-prep/CPython-2.7.10/15d60b3366ef43c7a4ae6b5f5e2d7567a95fe5f4/.deps/more_itertools-6.0.0-py2-none-any.whl/more_itertools/__init__.py", line 1, in <module>
    from more_itertools.more import *  # noqa
  File "/tmp/.pants.d/test/pytest-prep/CPython-2.7.10/15d60b3366ef43c7a4ae6b5f5e2d7567a95fe5f4/.deps/more_itertools-6.0.0-py2-none-any.whl/more_itertools/more.py", line 329
    def _collate(*iterables, key=lambda a: a, reverse=False):

This has been fixed on the latest version of pytest but I'm not sure if we can update to that version or not? pytest-dev/pytest#4774

Solution

Add requirements where we use pytest so we don't get incompatible versions of more-itertools

Result

pytest invocations continue to work on Python 2

@cheister cheister requested a review from stuhood Mar 1, 2019

@stuhood stuhood requested review from Eric-Arellano and cosmicexplorer Mar 1, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

See d0432df and #7238 -- we are currently unable to bump the pytest version beyond 3.0.7 as a result of this error. We would like to bump the version to the one fixed by pytest-dev/pytest#4774, but we cannot at this exact moment go past pytest 3.7 due to the issue described in #6282 (see the code around the change in #7238).

We probably don't want to land this change as is, for the reasons described in pytest-dev/pytest#4770 (comment) (linked in #7238 -- we don't want to impose requirement constraints on pants users running pytest). It seems like this error would arise if you have overridden the pytest requirement in another repo's pants.ini or similar -- on the face of it it seems like you may be able to apply this change (pinning more-itertools) to that repo's requirements.txt so that the version of pytest you've overridden works with python 2.

However, I might be mistaken -- are you seeing the error described when developing in the pants repo, or is overriding the more-itertools requirement in your own repo causing an error?

@cheister

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I hadn't seen #7238. I was getting the error on pants 1.13.0.

The comment you linked (pytest-dev/pytest#4770) is referring to pinning specific versions of more_itertools in pytest. This change doesn't pin a specific version, it merely limits the version of more_itertools to something that works with whatever version of python you're running. Basically the same thing pytest itself is doing with pytest-dev/pytest#4774.

That seems more flexible than pinning pytest to 3.0.7 specifically.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Ah, I misunderstood! Thank you for describing the difference -- this change sounds right enough to me then, although the specific implementation might be edited. @jsirois might the above^ be a reason to allow --pytest-requirements to be a list option, or would something like the current PR (hardcoding more-itertools<6.0.0 for py2, or maybe adding some more logic around it) be more appropriate?

@Eric-Arellano
Copy link
Contributor

left a comment

This looks right to me.

The key detail is python_version<3. Because more-itertools 6.0+ never will work with Python 2, users don't lose any compatibility here and I believe they can still specify a more specific version <6.0. If running with Python 3, there is no constraint at all. So, this change appears completely safe to make in that we won't be unreasonably constraining users at all.

Thank you for the patch!

--

Is this something we need to cherry-pick? cc @stuhood

@cosmicexplorer
Copy link
Contributor

left a comment

I think this is good to merge once both of the TODOs that @Eric-Arellano pointed out are added! Thanks again for the very specific and useful fix!

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Does this make it possible to revert (part of) #7238?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@stuhood yes I believe we can completely revert #7238, and it would be good to do that as part of this PR. We shouldn’t care which Pytest people use so long as it’s <3.7. That PR was solely designed to fix this more-itertools issue, and this PR resolves the issue in a more flexible way.

@cheister cheister force-pushed the cheister:more-itertools-python2 branch from aabf17b to 97f60f9 Mar 4, 2019

@cheister cheister force-pushed the cheister:more-itertools-python2 branch from 97f60f9 to 4be4f1c Mar 5, 2019

@cheister cheister merged commit b8ba105 into pantsbuild:master Mar 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cheister cheister deleted the cheister:more-itertools-python2 branch Mar 5, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Thanks @cheister !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.