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

Fix PyPy test failures #2793

Merged
merged 12 commits into from Jul 3, 2017
Merged

Fix PyPy test failures #2793

merged 12 commits into from Jul 3, 2017

Conversation

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Jun 15, 2017

Individual commits have explanations for why they are needed. They are mostly independent, they are either garbage collection related, or concerning some corner cases.

This also installs PyPyDispatcher library - it's an experimental PyDispatcher fork with PyPy support (https://github.com/lopuhin/pydispatcher) - I intend to make it less experimental first.

lopuhin added 7 commits Sep 1, 2016
For float(u'$10') PyPy includes "u'" in the error message,
and it's more fair to check error message on input we are really
passing.
It is not affected by Twisted bug #7989 and is more permissive
with pickling (especially with protocol=2).
One gc.collect() seems to be enough, but it's more reliable
to run it several times (at most 100), until all objects are collected.
On CPython get_func_args does not work correctly for built-in
methods.
Using https://github.com/lopuhin/pydispatcher, pypy branch.
This is executed as a separate step to avoid changing
default requirements.txt and setup.py. If just added to "deps"
in tox, this install command will be executed as one command
and PyPyDispatcher will not override PyDispatcher.
@kmike kmike self-requested a review Jun 15, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Jun 15, 2017

Codecov Report

Merging #2793 into master will decrease coverage by 0.02%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master    #2793      +/-   ##
==========================================
- Coverage   84.73%   84.71%   -0.03%     
==========================================
  Files         163      163              
  Lines        9164     9177      +13     
  Branches     1362     1363       +1     
==========================================
+ Hits         7765     7774       +9     
- Misses       1147     1150       +3     
- Partials      252      253       +1
Impacted Files Coverage Δ
scrapy/extensions/httpcache.py 95.51% <100%> (+0.01%) ⬆️
scrapy/cmdline.py 67.4% <100%> (+0.74%) ⬆️
scrapy/utils/python.py 84.09% <55.55%> (-1.54%) ⬇️
LevelDB does not have "official" close method, so we have
to rely on garbage collection to close it.
@kmike kmike removed their request for review Jun 15, 2017
@lopuhin lopuhin changed the title [WIP] Fix most PyPy test failures [WIP] Fix PyPy test failures Jun 15, 2017
@lopuhin
Copy link
Member Author

@lopuhin lopuhin commented Jun 15, 2017

Status: at the moment all tests pass under PyPy, and the build is taking less time than for CPython 3 (but much more than for CPython 2). I want to finish PyDispatcher PyPy support first (enable travis CI tests under PyPy, fix tests, merge to master).

@kmike
Copy link
Member

@kmike kmike commented Jun 15, 2017

Looks great!

What do you think about moving PyDispatcher fork to scrapy organization?

@lopuhin
Copy link
Member Author

@lopuhin lopuhin commented Jun 15, 2017

What do you think about moving PyDispatcher fork to scrapy organization?

Yes, that would be perfect! It is exported from original launchpad bzr source, the license is BSD-like. I wrote an email to the author asking if he is interested in merging PyPy fixes, but did not get a response.

tox.ini Outdated
@@ -57,6 +57,7 @@ commands =
[testenv:pypy]
basepython = pypy
commands =
pip install PyPyDispatcher>=2.0.6

This comment has been minimized.

@kmike

kmike Jun 19, 2017
Member

If we start supporting pypy then I think PyPyDispatcher>=2.0.6 should be added to setup.py instead of tox.ini.

It can be a bit tricky though: the best way is to use environment markers, but I'm still not sure how well are they supported. As I recall, they require pip > 6.0 and a recent setuptools, pip and setuptools used to be incompatible in evn markers format. But I've tried them a couple years ago; maybe it is better now.

An alternative is to have a condition in setup.py, but if we add a condition there we won't be able to use universal wheels.

This comment has been minimized.

@lopuhin

lopuhin Jun 19, 2017
Author Member

Thanks @kmike , I'll check the current state. At least PyPy ships with a recent pip, so if they are backwards compatible, we might be able to use them.

This comment has been minimized.

@kmike

kmike Jun 19, 2017
Member

Requiring new pip/setuptools only for pypy is fine.

This comment has been minimized.

@lopuhin

lopuhin Jun 19, 2017
Author Member

Looks like adding environment markers might result in issues with old setuptool (python-hyper/hyper#279) but it looks like more and more packages start to require relatively recent setuptools and use environment markers (e.g. requests: psf/requests#4006). But support of different kinds of markers varies, I'll check which versions are required in our case.

This comment has been minimized.

@lopuhin

lopuhin Jun 19, 2017
Author Member

I tried installing universal wheel on ancient pip 1.5.4, setuptools 3.3 (default in Ubuntu 14.04), and also on a couple of other random combinations (setuptools 17.0, 18.0, pip 7.1.2, 8.1.1), and all seems to work fine. Another thing we could do for extra safety is include a check for the setuptools version like this: https://github.com/PyCQA/astroid/pull/360/files

To summarize, it looks like it's fine to use the version marker, but it's really hard to check all possible combinations.

This comment has been minimized.

@lopuhin

lopuhin Jun 19, 2017
Author Member

Added environment markers in b0a9236.

@@ -11,6 +11,8 @@ matrix:
env: TOXENV=py27
- python: 2.7
env: TOXENV=jessie
- python: 2.7

This comment has been minimized.

@redapple

redapple Jun 19, 2017
Contributor

(python: 2.7, env: TOXENV=pypy) now appears twice.

This comment has been minimized.

@lopuhin

lopuhin Jun 19, 2017
Author Member

Thanks for the catch @redapple ! Removed the duplicate.

@lopuhin
Copy link
Member Author

@lopuhin lopuhin commented Jun 19, 2017

I want to finish PyDispatcher PyPy support first (enable travis CI tests under PyPy, fix tests, merge to master).

@kmike this part is finished, PyPy tests are running on Travis, so I'm basically ready to transfer the repo. Shall I enable auto-upload to PyPI from Travis too? Any other things to do before the transfer?

@kmike
Copy link
Member

@kmike kmike commented Jun 19, 2017

@lopuhin I think tests + pypi auto-uploads is all what is needed, can't ask for more :)
Enabling codecov should be also easy enough.

@lopuhin
Copy link
Member Author

@lopuhin lopuhin commented Jul 3, 2017

@kmike I make a PyPyDispatcher release via Triavic CI: here is the release https://pypi.python.org/pypi/PyPyDispatcher/2.1.1, this is the build https://travis-ci.org/lopuhin/pydispatcher/jobs/249606929#L403 and here is the travis config https://github.com/lopuhin/pydispatcher/blob/c86bc56cec66265fb512549074444dc3dcb88bfc/.travis.yml#L43. Code coverage is already enabled, it's not perfect but mostly it's ignored exceptions that are not covered. So I think everything is ready for moving PyPyDispatcher to the scrapy GH org.

I think this PR is ready for review. It's failing the coverage check because of pypy garbage collection function (I think it's the only reason for low coverage, but I'm not sure). pypy tests are executed without coverage enabled to be faster - it's possible to check that the function does not crash under CPython too (and so improve coverage), but this seems weird.

@lopuhin lopuhin changed the title [WIP] Fix PyPy test failures Fix PyPy test failures Jul 3, 2017
@kmike
Copy link
Member

@kmike kmike commented Jul 3, 2017

//cc @redapple @dangra any objections on moving PyPyDispatcher to Scrapy org and merging this pull request?

@redapple
Copy link
Contributor

@redapple redapple commented Jul 3, 2017

No objections

@redapple
Copy link
Contributor

@redapple redapple commented Jul 3, 2017

@kmike
Copy link
Member

@kmike kmike commented Jul 3, 2017

🎉 New kid's 11 years old!

@kmike kmike merged commit dd5ab6c into scrapy:master Jul 3, 2017
1 of 2 checks passed
1 of 2 checks passed
codecov/patch 73.33% of diff hit (target 84.73%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lopuhin lopuhin deleted the lopuhin:pypy2 branch Jul 3, 2017
@lopuhin
Copy link
Member Author

@lopuhin lopuhin commented Jul 3, 2017

🎉
Thanks @kmike and @redapple !

@kmike kmike added this to the v1.5 milestone Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.