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

Refs #34986 -- Confirmed support for PyPy 3.10 with PostgreSQL. #17500

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

ngnpope
Copy link
Member

@ngnpope ngnpope commented Nov 21, 2023

@mgorny
Copy link
Contributor

mgorny commented Nov 21, 2023

Thanks a lot for doing this! Could you also include something like:

diff --git a/tox.ini b/tox.ini
index 2e0a3b421a..c635a129b2 100644
--- a/tox.ini
+++ b/tox.ini
@@ -26,7 +26,7 @@ setenv =
     PYTHONDONTWRITEBYTECODE=1
 deps =
     -e .
-    py{3,310,311,312}: -rtests/requirements/py3.txt
+    py{3,310,311,312,py3}: -rtests/requirements/py3.txt
     postgres: -rtests/requirements/postgres.txt
     mysql: -rtests/requirements/mysql.txt
     oracle: -rtests/requirements/oracle.txt

@ngnpope
Copy link
Member Author

ngnpope commented Nov 21, 2023

@mgorny Sure. I'll see if there is anything else that makes sense while I'm at it.

@ngnpope
Copy link
Member Author

ngnpope commented Nov 21, 2023

Currently down to 2 failures and 1 error running on SQLite 😎

auth_tests.test_auth_backends.AuthenticateTests.test_aauthenticate_sensitive_variables
model_forms.tests.OtherModelFormTests.test_prefetch_related_queryset
queries.tests.CloneTests.test_evaluated_queryset_as_argument

The model_forms.tests.OtherModelFormTests.test_prefetch_related_queryset one is weird...

On CPython we get:

Captured queries were:
1. SELECT COUNT(*) AS "__count" FROM "model_forms_colourfulitem"
2. SELECT "model_forms_colourfulitem"."id", "model_forms_colourfulitem"."name" FROM "model_forms_colourfulitem"
3. SELECT ("model_forms_colourfulitem_colours"."colourfulitem_id") AS "_prefetch_related_val_colourfulitem_id", "model_forms_colour"."id", "mode
l_forms_colour"."name" FROM "model_forms_colour" INNER JOIN "model_forms_colourfulitem_colours" ON ("model_forms_colour"."id" = "model_forms_col
ourfulitem_colours"."colour_id") WHERE "model_forms_colourfulitem_colours"."colourfulitem_id" IN (1, 2)

On PyPy we get:

Captured queries were:
1. SELECT "model_forms_colourfulitem"."id", "model_forms_colourfulitem"."name" FROM "model_forms_colourfulitem"
2. SELECT ("model_forms_colourfulitem_colours"."colourfulitem_id") AS "_prefetch_related_val_colourfulitem_id", "model_forms_colour"."id", "mode
l_forms_colour"."name" FROM "model_forms_colour" INNER JOIN "model_forms_colourfulitem_colours" ON ("model_forms_colour"."id" = "model_forms_col
ourfulitem_colours"."colour_id") WHERE "model_forms_colourfulitem_colours"."colourfulitem_id" IN (1, 2)

Update: Aha! It turns out that PyPy isn't calling ModelChoiceField.__len__().

On CPython:

>>> class CustomIterator:
...     def __iter__(self):
...         yield from range(3)
...     def __len__(self):
...         print("CustomIterator.__len__()")
...         return 3
... 
>>> tuple(CustomIterator())
CustomIterator.__len__()
(0, 1, 2)

On PyPy:

>>>> class CustomIterator:
....     def __iter__(self):
....         yield from range(3)
....     def __len__(self):
....         print("CustomIterator.__len__()")
....         return 3
....         
>>>> tuple(CustomIterator())
(0, 1, 2)

django/utils/encoding.py Outdated Show resolved Hide resolved
django/utils/choices.py Outdated Show resolved Hide resolved
@ngnpope
Copy link
Member Author

ngnpope commented Nov 21, 2023

For auth_tests.test_auth_backends.AuthenticateTests.test_aauthenticate_sensitive_variables the snag is that PyPy executes async stuff differently...

I get the following traceback for CPython:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 277, in __call__
    return call_result.result()
  File "/usr/local/lib/python3.10/concurrent/futures/_base.py", line 451, in result
    return self.__get_result()
  File "/usr/local/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 353, in main_wrap
    result = await self.awaitable(*args, **kwargs)
  File "/tests/django/django/test/utils.py", line 434, in inner
    return await func(*args, **kwargs)
  File "/tests/django/tests/auth_tests/test_auth_backends.py", line 773, in test_aauthenticate_sensitive_variables
    await aauthenticate(
  File "/tests/django/django/contrib/auth/__init__.py", line 99, in aauthenticate
    return await sync_to_async(authenticate)(request, **credentials)
  File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 479, in __call__
    ret: _R = await loop.run_in_executor(
  File "/usr/local/lib/python3.10/site-packages/asgiref/current_thread_executor.py", line 40, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 538, in thread_handler
    return func(*args, **kwargs)
  File "/tests/django/django/views/decorators/debug.py", line 78, in sensitive_variables_wrapper
    return func(*func_args, **func_kwargs)
  File "/tests/django/django/contrib/auth/__init__.py", line 79, in authenticate
    user = backend.authenticate(request, **credentials)
  File "/tests/django/django/views/decorators/debug.py", line 78, in sensitive_variables_wrapper
    return func(*func_args, **func_kwargs)
  File "/tests/django/tests/auth_tests/test_auth_backends.py", line 717, in authenticate
    raise TypeError
TypeError

And the following for PyPy:

Traceback (most recent call last):
  File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/sync.py", line 277, in __call__
    return call_result.result()
  File "/opt/pypy/lib/pypy3.10/concurrent/futures/_base.py", line 451, in result
    return self.__get_result()
  File "/opt/pypy/lib/pypy3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "/opt/pypy/lib/pypy3.10/asyncio/tasks.py", line 304, in __wakeup
    future.result()
  File "/opt/pypy/lib/pypy3.10/asyncio/futures.py", line 201, in result
    raise self._exception.with_traceback(self._exception_tb)
  File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/current_thread_executor.py", line 40, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/sync.py", line 538, in thread_handler
    return func(*args, **kwargs)
  File "/opt/pypy/lib/pypy3.10/_contextvars.py", line 44, in run
    return callable(*args, **kwargs)
  File "/tests/django/django/views/decorators/debug.py", line 78, in sensitive_variables_wrapper
    return func(*func_args, **func_kwargs)
  File "/tests/django/django/contrib/auth/__init__.py", line 79, in authenticate
    user = backend.authenticate(request, **credentials)
  File "/tests/django/django/views/decorators/debug.py", line 78, in sensitive_variables_wrapper
    return func(*func_args, **func_kwargs)
  File "/tests/django/tests/auth_tests/test_auth_backends.py", line 717, in authenticate
    raise TypeError
TypeError

I suspect we get away with all the __traceback_hide__ stuff that was added to asgiref under CPython, but in PyPy we get a _contextvars.Context.run() call where callable is a functools.partial(). And that exposes the following:

name = 'callable'
value = functools.partial(<function authenticate at 0x0000000002c99f60>, None, username='testusername', password='mypassword')

We could extend SafeExceptionReporterFilter._cleanse_special_types() to handle functools.partial(), but that the problem here is that we had @sensitive_variables("credentials") but the **credentials makes it unpacked to username=..., password=... in the functools.partial() call.

Blergh. Not sure where to go from here.

@mgorny
Copy link
Contributor

mgorny commented Nov 22, 2023

CC @mattip, @cfbolz; could you particularly take a look at #17500 (comment) and advise?

@cfbolz
Copy link

cfbolz commented Nov 22, 2023

I'm currently fixing the pattern matching bug in pypy.

@ngnpope missing context to help here a little bit. is the problem that CPython's functools.partial instances are less inspectable/have different reprs? or the problem that _contextvars is an uninspectable C module in CPython, but just a regular Python file in PyPy? we could certainly add __tracebackhide__ to _contextvars.Context.run() if that is helpful.

@ngnpope
Copy link
Member Author

ngnpope commented Nov 22, 2023

Thanks for the pattern matching fix, @cfbolz. I'll give that a whirl when the next nightly is out 🎉

I'll try and work out a little more detail of what is going on and get back to you regarding this last test failure.

@ngnpope ngnpope marked this pull request as ready for review November 22, 2023 11:41
tests/queries/tests.py Outdated Show resolved Hide resolved
tests/model_enums/tests.py Outdated Show resolved Hide resolved
django/utils/version.py Outdated Show resolved Hide resolved
@ngnpope
Copy link
Member Author

ngnpope commented Nov 27, 2023

@ngnpope missing context to help here a little bit. is the problem that CPython's functools.partial instances are less inspectable/have different reprs? or the problem that _contextvars is an uninspectable C module in CPython, but just a regular Python file in PyPy? we could certainly add __tracebackhide__ to _contextvars.Context.run() if that is helpful.

@cfbolz Sorry for the delay getting back to you. This is the last failure, so we're making progress!

I've looked at the tracebacks and commented out the common bits between CPython and PyPy.

Here is the one for CPython:

# Traceback (most recent call last):
#   File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 277, in __call__
#     return call_result.result()
#   File "/usr/local/lib/python3.10/concurrent/futures/_base.py", line 451, in result
#     return self.__get_result()
#   File "/usr/local/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
#     raise self._exception

    File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 353, in main_wrap
      result = await self.awaitable(*args, **kwargs)
    File "/django/source/django/test/utils.py", line 434, in inner
      return await func(*args, **kwargs)
    File "/django/source/tests/auth_tests/test_auth_backends.py", line 773, in test_aauthenticate_sensitive_variables
      await aauthenticate(
    File "/django/source/django/contrib/auth/__init__.py", line 99, in aauthenticate
      return await sync_to_async(authenticate)(request, **credentials)
    File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 479, in __call__
      ret: _R = await loop.run_in_executor(

#   File "/usr/local/lib/python3.10/site-packages/asgiref/current_thread_executor.py", line 40, in run
#     result = self.fn(*self.args, **self.kwargs)
#   File "/usr/local/lib/python3.10/site-packages/asgiref/sync.py", line 538, in thread_handler
#     return func(*args, **kwargs)

#   File "/django/source/django/views/decorators/debug.py", line 73, in sensitive_variables_wrapper
#     return func(*func_args, **func_kwargs)
#   File "/django/source/django/contrib/auth/__init__.py", line 79, in authenticate
#     user = backend.authenticate(request, **credentials)
#   File "/django/source/django/views/decorators/debug.py", line 73, in sensitive_variables_wrapper
#     return func(*func_args, **func_kwargs)
#   File "/django/source/tests/auth_tests/test_auth_backends.py", line 717, in authenticate
#     raise TypeError
# TypeError

And the one for PyPy:

# Traceback (most recent call last):
#   File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/sync.py", line 277, in __call__
#     return call_result.result()
#   File "/opt/pypy/lib/pypy3.10/concurrent/futures/_base.py", line 451, in result
#     return self.__get_result()
#   File "/opt/pypy/lib/pypy3.10/concurrent/futures/_base.py", line 403, in __get_result
#     raise self._exception

    File "/opt/pypy/lib/pypy3.10/asyncio/tasks.py", line 304, in __wakeup
      future.result()
    File "/opt/pypy/lib/pypy3.10/asyncio/futures.py", line 201, in result
      raise self._exception.with_traceback(self._exception_tb)

#   File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/current_thread_executor.py", line 40, in run
#     result = self.fn(*self.args, **self.kwargs)
#   File "/opt/pypy/lib/pypy3.10/site-packages/asgiref/sync.py", line 538, in thread_handler
#     return func(*args, **kwargs)

    File "/opt/pypy/lib/pypy3.10/_contextvars.py", line 44, in run
      return callable(*args, **kwargs)

#   File "/django/source/django/views/decorators/debug.py", line 73, in sensitive_variables_wrapper
#     return func(*func_args, **func_kwargs)
#   File "/django/source/django/contrib/auth/__init__.py", line 79, in authenticate
#     user = backend.authenticate(request, **credentials)
#   File "/django/source/django/views/decorators/debug.py", line 73, in sensitive_variables_wrapper
#     return func(*func_args, **func_kwargs)
#   File "/django/source/tests/auth_tests/test_auth_backends.py", line 717, in authenticate
#     raise TypeError
# TypeError

I'm not sure why there is a difference with the first block - perhaps something different happens in PyPy when stitching frames together in relation to async code? I seem to be able to get some of that output if I fiddle with the way that exceptions are caught, etc. But I think we don't need to worry about this bit too much.

The issue is, as you thought, that _contextvars is opaque in CPython, whereas in PyPy callable is coming out as:

functools.partial(<function authenticate at 0x00007fec19875b00>, None, username='testusername', password='mypassword'```

So we could add __traceback_hide__ = True to _contextvars.Context.run() in PyPy which should solve this. See django/asgiref#383 for similar work in asgiref.

An alternative is to add something to always mask functools.partial() to always hide it's arguments:

diff --git a/django/views/debug.py b/django/views/debug.py
index c1265bfe6b..b280f96c66 100644
--- a/django/views/debug.py
+++ b/django/views/debug.py
@@ -195,6 +195,14 @@ class SafeExceptionReporterFilter:
                     multivaluedict[param] = self.cleansed_substitute
         return multivaluedict
 
+    def get_cleansed_partial(self, request, value):
+        """Replace all arguments to partial functions."""
+        return type(value)(
+            value.func,
+            *(None if v is None else self.cleansed_substitute for v in value.args),
+            **{key: self.cleansed_substitute for key in value.keywords},
+        )
+
     def get_post_parameters(self, request):
         """
         Replace the values of POST parameters marked as sensitive with
@@ -234,6 +242,10 @@ class SafeExceptionReporterFilter:
         if is_multivalue_dict:
             # Cleanse MultiValueDicts (request.POST is the one we usually care about)
             value = self.get_cleansed_multivaluedict(request, value)
+
+        if isinstance(value, (functools.partial, functools.partialmethod)):
+            value = self.get_cleansed_partial(request, value)
+
         return value
 
     def get_traceback_frame_variables(self, request, tb_frame):

This fixed the test for me. What are your thoughts on that @felixxm?

@felixxm
Copy link
Member

felixxm commented Nov 27, 2023

Merged two commits in 174369a and 9baaf89. This should help us avoid general issues when running tests. Let's wait for the next daily build.

@felixxm
Copy link
Member

felixxm commented Nov 27, 2023

This fixed the test for me. What are your thoughts on that @felixxm?

I'm skeptical about fixing something in Django that is not Django's fault.

@mattip
Copy link

mattip commented Nov 27, 2023

PyPy has a __pypy__.hidden_applevel decorator that can be used to do the same as __traceback_hide__. We already use this in functools.cmp_to_key or functools.reduce which in CPython are usually c-extension function. We can add this to the appropriate pure-python _contextvars.py functions and classes that should be hidden from tracebacks. Which are appearing in tracebacks and should not?

@ngnpope
Copy link
Member Author

ngnpope commented Nov 27, 2023

@mattip That sounds perfect! If you could add that it'd be great.

(I've also provided the minimal reproducers you requested above.)

@mattip
Copy link

mattip commented Nov 27, 2023

I hid _contextvars.Context.run to pypy3.9

I've also provided the minimal reproducers you requested above.

Thanks, I opened python/cpython#112451

@ngnpope
Copy link
Member Author

ngnpope commented Nov 27, 2023

Thanks @mattip. Could you also merge that into the pypy3.10 branch please?

@mattip
Copy link

mattip commented Nov 27, 2023

Pushed to py3.10 too

@ngnpope
Copy link
Member Author

ngnpope commented Nov 28, 2023

Let me check this more carefully.

Regarding using PostgreSQL, with the fix in psycopg/psycopg#686, and the non-binary version I only get the following errors:

======================================================================
FAIL: test_closed_server_side_cursor (backends.postgresql.test_server_side_cursors.ServerSideCursorsPostgres)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/django/source/tests/backends/postgresql/test_server_side_cursors.py", line 92, in test_closed_server_side_cursor
    self.assertEqual(len(cursors), 0)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 845, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 838, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 1 != 0

======================================================================
FAIL: test_server_side_cursors_setting (backends.postgresql.test_server_side_cursors.ServerSideCursorsPostgres)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/django/source/tests/backends/postgresql/test_server_side_cursors.py", line 101, in test_server_side_cursors_setting
    self.asserNotUsesCursor(Person.objects.iterator())
  File "/django/source/tests/backends/postgresql/test_server_side_cursors.py", line 57, in asserNotUsesCursor
    self.assertUsesCursor(queryset, num_expected=0)
  File "/django/source/tests/backends/postgresql/test_server_side_cursors.py", line 49, in assertUsesCursor
    self.assertEqual(len(cursors), num_expected)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 845, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/opt/pypy/lib/pypy3.10/unittest/case.py", line 838, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 1 != 0

For which we could do the following 😖

diff --git a/tests/backends/postgresql/test_server_side_cursors.py b/tests/backends/postgresql/test_server_side_cursors.py
index 705e798c23..712fff8371 100644
--- a/tests/backends/postgresql/test_server_side_cursors.py
+++ b/tests/backends/postgresql/test_server_side_cursors.py
@@ -1,3 +1,4 @@
+import gc
 import operator
 import unittest
 from collections import namedtuple
@@ -5,6 +6,7 @@ from contextlib import contextmanager
 
 from django.db import connection, models
 from django.test import TestCase
+from django.utils.version import PYPY
 
 from ..models import Person
 
@@ -88,9 +90,12 @@ class ServerSideCursorsPostgres(TestCase):
         persons = Person.objects.iterator()
         next(persons)  # Open a server-side cursor
         del persons
+        if PYPY:
+            gc.collect()
         cursors = self.inspect_cursors()
         self.assertEqual(len(cursors), 0)
 
+    @unittest.skipIf(PYPY, reason="Forced garbage collection breaks test.")
     def test_server_side_cursors_setting(self):
         with self.override_db_setting(DISABLE_SERVER_SIDE_CURSORS=False):
             persons = Person.objects.iterator()

@ngnpope ngnpope force-pushed the ticket-34986 branch 3 times, most recently from 56cf1ee to 3a7edd1 Compare December 4, 2023 16:56
@ngnpope
Copy link
Member Author

ngnpope commented Dec 4, 2023

Updated using an alternative unpickleable object as mentioned in #17500 (comment) and added some fixes for PostgreSQL. I've also added a commit to extend the scheduled testing to include PostgreSQL and add the ability to execute tests for PyPy using a label on a PR.

@felixxm
Copy link
Member

felixxm commented Dec 5, 2023

Updated using an alternative unpickleable object as mentioned in #17500 (comment) and added some fixes for PostgreSQL. I've also added a commit to extend the scheduled testing to include PostgreSQL and add the ability to execute tests for PyPy using a label on a PR.

I don't think we want more CI tests for PyPy. I don't see much value in labeled CI tests for PyPy, because how could we decide if something might not be compatible with PyPy? Daily builds seems to be sufficient in such rare cases.

@ngnpope
Copy link
Member Author

ngnpope commented Dec 5, 2023

I don't think we want more CI tests for PyPy. I don't see much value in labeled CI tests for PyPy, because how could we decide if something might not be compatible with PyPy? Daily builds seems to be sufficient in such rare cases.

Ok. I'll revert that one to only add a daily run with PostgreSQL.

tests/queries/tests.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Dec 6, 2023

I merged 4 commits from this PR, please rebase.

@ngnpope
Copy link
Member Author

ngnpope commented Dec 6, 2023

Thanks. Rebased.

@felixxm felixxm changed the title Fixed #34986 -- Added support for PyPy 3.10. Refs #34986 -- Confirmed support for PyPy 3.10 with PostgreSQL. Dec 6, 2023
@felixxm felixxm self-assigned this Dec 6, 2023
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ngnpope Thanks 👍

@felixxm felixxm merged commit 2dca98f into django:main Dec 7, 2023
33 checks passed
@ngnpope ngnpope deleted the ticket-34986 branch December 7, 2023 12:32
@mgorny
Copy link
Contributor

mgorny commented Dec 9, 2023

Thanks a lot for all your hard work! Will these changes make it to 5.0.x?

@felixxm
Copy link
Member

felixxm commented Dec 11, 2023

Will these changes make it to 5.0.x?

Unfortunately not.

@ngnpope
Copy link
Member Author

ngnpope commented Dec 11, 2023

@mgorny Most of the changes were to Django's tests to handle minor implementation differences. If you're not running those, then there are very few changes.

Note that you'll need to run using a nightly build of PyPy right now as there were a few tweaks there over PyPy v7.3.13. And we've only tested for SQLite and PostgreSQL (which requires psycopg>=3.1.14 without the binary build).

I think the only one that'll stop PyPy working for 5.0 is 5f9e5c1 to fix a crash in serializing for migrations. (This was related to something newly added in 5.0, but arguably wasn't a release blocker because PyPy support had stagnated anyway.)

And, if you want to use Django's autoreloader while using Python's -X options, you may need to backport 051dbb5. But this is probably quite a niche thing.

@mgorny
Copy link
Contributor

mgorny commented Dec 11, 2023

No problem. I've been able to backport them easily to 5.0, 4.2 and 4.1 branches in Gentoo. Thanks a lot, again!

@felixxm
Copy link
Member

felixxm commented Dec 15, 2023

@ngnpope GitHub action for PyPy and PostgreSQL crashes.

@ngnpope
Copy link
Member Author

ngnpope commented Dec 15, 2023

Thanks @felixxm. This was a presumption that the existing settings file was correct. Fixed in #17612.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants