-
Notifications
You must be signed in to change notification settings - Fork 429
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 Django 5.0 with PyPy 3.10 #708
Conversation
(DAYS, _('Day')), | ||
(HOURS, _('Hour')), | ||
(MINUTES, _('Minute')), | ||
(SECONDS, _('Second')), | ||
(MICROSECONDS, _('Microsecond')), | ||
) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!!
Can we please change this only on PyPy and in a way that allows it to be removed after PyPy v7.3.14 is released?
Tuples take less memory and signal to the reader that these values should normally not be modified at runtime. Also, given comments in the first few episodes of the core.py podcast, tuples should perform better in the multi-GIL world of Python 3.13+.
Something like...
import platform
if platform.python_implementation() == "PyPy": # Workaround for PyPy < v7.3.14
PERIOD_CHOICES = list(PERIOD_CHOICES)
SINGULAR_PERIODS = list(SINGULAR_PERIODS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense, since Django always converts them to lists under the hood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why is there a bug at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all about test_models_match_migrations
that was failing in your PR https://github.com/celery/django-celery-beat/actions/runs/7267638477/job/19801878411?pr=705#step:6:2073
I think the PR description should cover the problem pretty well. If it's unclear, let me know and I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR fixes the problem of generating redundant migrations in PyPy 3.10/Django 5.0 environment.
In Django 5.0 they changed the way of normalizing choices, using
match
.CPython works fine with
case Iterable()
, so the choices always go through this list comprehension, being converted from tuples to lists every time.PyPy 3.10-7.3.13 has a bug, it can't match
case Iterable()
, but works withcase Iterable
instead (which is wrong). So the test was failing, because the rendered schema containedchoices=[..., ...]
, while schema generated from the models containtedchoices=(..., ...)
, which caused a redundant auto generated migration.PyPy fixed it in 7.3.14 though.
Changelog:
(The issue itself is unavailable, probably because they're moving to GitHub.)
So this small change of converting choices to lists fixes the problem.
Related issues and PRs: