Fixes build for PyPy3#6635
Conversation
mjhoffman65
changed the title
|
I think we should stop couchbase testing and eventually remove it from the build. I am not sure how much popular it is. |
Since we're getting PRs for it, it is in use. |
|
What we can do is specify that couchebase cannot be installed in pypy using markers. |
968dd76 to
5b0a979
Compare
|
We now see actual errors. |
Yep! Working now to fix them. |
5b0a979 to
c0ca554
Compare
|
This pull request introduces 1 alert and fixes 2 when merging c0ca554 into 3384937 - view on LGTM.com new alerts:
fixed alerts:
|
| self.patching('celery.backends.mongodb.MongoBackend.encode') | ||
| self.patching('celery.backends.mongodb.MongoBackend.decode') | ||
| self.patching('celery.backends.mongodb.Binary') | ||
| self.patching('datetime.datetime') |
There was a problem hiding this comment.
Removing that line fixes this error when running the MongoDB backend tests.
There was a problem hiding this comment.
Why does it pass on CPython?
There was a problem hiding this comment.
tox.ini appears to need a change to run these tests.
diff --git a/tox.ini b/tox.ini
index f62ea3cdf..51cf5d020 100644
--- a/tox.ini
+++ b/tox.ini
@@ -31,7 +31,7 @@ deps=
3.6,3.7,3.8,3.9: -r{toxinidir}/requirements/test-ci-default.txt
3.5,3.6,3.7,3.8,3.9: -r{toxinidir}/requirements/docs.txt
3.6,3.7,3.8,3.9: -r{toxinidir}/requirements/docs.txt
- pypy3: -r{toxinidir}/requirements/test-ci-base.txt
+ pypy3: -r{toxinidir}/requirements/test-ci-default.txt
integration: -r{toxinidir}/requirements/test-integration.txt
There was a problem hiding this comment.
This patch was originally added back in the depths of time: 3ea8824 It seems to be a vestigial thing, so I think it'll be fine to remove it.
The breakage is because pypy3 appears to be a little less duck-type-y with its datetime subtraction implementation. We're giving it a Mock that looks like a datetime object, but isn't probably isn't using a spec= to pass isinstance() checks.
Actually, this is super interesting. It seems to be mocking confusion because we do a bunch of from datetime import datetime imports which means that by only mocking datetime.datetime in this suite, we're causing datetime to be different in various places and when pypy3 does an isinstance() check for in its __sub__() implementation, it's actually checking if the arguments are subclasses of the mock class!.
tl;dr easiest fix is to avoid the issue entirely by using the change @mjhoffman65 put up. Correct fix is to not use from imports so that we're always accessing via the defining module and mocks will work without weird contortions.
|
What about that event is a boolean failure that pops up every now and then? |
@thedrow I've ran it 4 times in docker and so far it has always passed. I'm not sure how to trigger reruns with the github actions, but I haven't seen that error pop up yet. I'm assuming it's this one: #6489 |
|
Yes, exactly. |
|
Was able to recreate #6489. Will reopen after I fix it. |
There's no need to close this PR. Just mark it as a draft if it's not ready for merge. |
c0ca554 to
3712de6
Compare
|
This pull request introduces 1 alert and fixes 2 when merging 3712de6 into cfa1b41 - view on LGTM.com new alerts:
fixed alerts:
|
|
It seems like the failing test is not related to your change. |
|
I've rebased this PR but I'm still waiting for your response @mjhoffman65. |
Mocking `datetime.datetime` was causing an issue with `datetime.utcnow()`. This mock doesn't appear to be needed. See https://github.com/celery/celery/pull/6635/checks?check_run_id=1944166896.
4dd514e to
a3b2af7
Compare
|
Freshened on master and also with a few extra fixes for the same kind of I'm hoping this will go green now, with the fix from #6806 pulled in via the rebase as well. |
Codecov Report
@@ Coverage Diff @@
## master #6635 +/- ##
==========================================
+ Coverage 70.70% 75.82% +5.12%
==========================================
Files 138 138
Lines 16603 16603
Branches 2092 2092
==========================================
+ Hits 11739 12590 +851
+ Misses 4668 3797 -871
- Partials 196 216 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Huh, weird, it's trying to run flake8 under action environments without python 3.9 installed :/ Edit: Oh 0295d7a did that. Probably no point in linting in each of the build jobs though, that's kind of weird |
|
This pull request introduces 1 alert and fixes 1 when merging a3b2af7 into d9d8250 - view on LGTM.com new alerts:
fixed alerts:
|
60a8122 to
1ce3b20
Compare
Also fix some flakes which may have been added by some other autoformatter in celery#6804. The 4 space non-visual-indentation should keep most formatters fairly happy.
1ce3b20 to
f486c0d
Compare
|
This pull request fixes 1 alert when merging f486c0d into d9d8250 - view on LGTM.com fixed alerts:
|
|
Thanks to everyone who helped us stablize the build :) |
* installs packages the same way docker does * removes couchbase dependency for PyPy * removes ephem dependency for PyPy * fixes mongo unit tests for PyPy3 Mocking `datetime.datetime` was causing an issue with `datetime.utcnow()`. This mock doesn't appear to be needed. See https://github.com/celery/celery/pull/6635/checks?check_run_id=1944166896. * fix: Avoid shadowing `Thread` attributes Fixes #6489 * ci: Install default deps for pypy3 toxenvs * ci: Run unit tests with `tox` * ci: Lint source in separate action using `tox` * ci: Redent codecov action * test: Rework some mocking in `test_platforms.py` Also fix some flakes which may have been added by some other autoformatter in #6804. The 4 space non-visual-indentation should keep most formatters fairly happy. * style: Fix some flakes Co-authored-by: maybe-sybr <[email protected]>
* installs packages the same way docker does * removes couchbase dependency for PyPy * removes ephem dependency for PyPy * fixes mongo unit tests for PyPy3 Mocking `datetime.datetime` was causing an issue with `datetime.utcnow()`. This mock doesn't appear to be needed. See https://github.com/celery/celery/pull/6635/checks?check_run_id=1944166896. * fix: Avoid shadowing `Thread` attributes Fixes celery#6489 * ci: Install default deps for pypy3 toxenvs * ci: Run unit tests with `tox` * ci: Lint source in separate action using `tox` * ci: Redent codecov action * test: Rework some mocking in `test_platforms.py` Also fix some flakes which may have been added by some other autoformatter in celery#6804. The 4 space non-visual-indentation should keep most formatters fairly happy. * style: Fix some flakes Co-authored-by: maybe-sybr <[email protected]>
4 participants
@auvipy, I took at stab at fixing the PyPy failures. My first step is to try to install all the requirements and recreate the unit test failure in the docker environment. I ran into issues installing the python requirements and am wondering if more work needs to be done before we can expect celery to run with PyPy. Looks like at least a couple of the dependencies do not work with PyPy since they rely on a C backend.
couchbase: https://docs.couchbase.com/python-sdk/current/hello-world/start-using-sdk.html#pypy-support
ephem: celery/django-celery-beat#69 (comment) (Note we may be able to replace this with skyfield. See here for more.)
This PR illustrates the same error I'm getting in the docker environment: https://github.com/celery/celery/runs/1930241932?check_suite_focus=true
How should I move forward?