Opened 4 years ago

Closed 2 years ago

Last modified 17 months ago

#31169 closed New feature (fixed)

Allow parallel test runner to work with Windows/macOS `spawn` process start method.

Reported by: Brandon Navra Owned by: David Smith
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Ichlasul Affan, Ryan Siemens, Adam Wróbel Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Brandon Navra)

Python 3.8 on MacOS has changed the default start method for the multiprocessing module from fork to spawn: ​https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods.

When running tests with the --parallel flag, this causes the worker processes to fail with django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet. as they no longer have a copy of the parent memory state. It can also cause the workers to fail to find the cloned dbs ( {{django.db.utils.OperationalError: FATAL: database "xxx_1" does not exist}} ) as the db test prefix is missing.

I have attached a patch which changes django.test.runner._init_worker (the worker initialiser for ParallelTestSuite) to run django.setup() and set the db name to one with the test_ prefix.

Attachments (1)

Ensure_Django_is_setup_correctly_in_parallel_test_workers.patch (1.2 KB ) - added by Brandon Navra 4 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 by Brandon Navra, 4 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 4 years ago

spawn() is also a default method on Windows, and we don't encounter any issues with it 🤔.

comment:3 by Brandon Navra, 4 years ago

I'm still trying to research the exact root cause. The Python issue which triggered this change has snippets of info: https://code.djangoproject.com/ticket/31169
but nothing conclusive. My theory is that the memory copying semantics between MacOS and Windows are different and hence the spawn method doesn't have identical behaviour between the two.

comment:4 by Mariusz Felisiak, 4 years ago

Ahhh, sorry we don't use parallel on Windows.

comment:5 by Carlton Gibson, 4 years ago

Summary: Parallel test mode not working on MacOS with recent versions of Python 3 → Allow parallel test runner to work with Windows/macOS `spawn` process start method.
Triage Stage: Unreviewed → Accepted
Type: Bug → New feature
Version: 3.0 → master

Parallel running is disabled on Windows:

​https://github.com/django/django/blob/59b4e99dd00b9c36d56055b889f96885995e4240/django/test/runner.py#L286-L295

def default_test_processes():
    """Default number of test processes when using the --parallel option."""
    # The current implementation of the parallel test runner requires
    # multiprocessing to start subprocesses with fork().
    if multiprocessing.get_start_method() != 'fork':
        return 1
    try:
        return int(os.environ['DJANGO_TEST_PROCESSES'])
    except KeyError:
        return multiprocessing.cpu_count()

I'll accept this as a new feature: the limitation has been there since it was implemented.

Brandon, your patch is tiny. Is it really that simple? We'd need tests and a few other adjustments (like to the function above) but, fancy opening a PR?

Last edited 4 years ago by Carlton Gibson (previous) (diff)

comment:6 by Carlton Gibson, 4 years ago

So this occurs on macOS 10.15. (I have 10.14 currently so can't experiment there.)

Applying the patch on Windows, alas, doesn't immediately solve the issue, but it is INSTALLED_APPS/AppRegistry errors that are raised, so it's going to be in the right ball-park.

More investigating needed, but this would be a good one to land.

comment:7 by Brandon Navra, 4 years ago

I created a PR with the changes from my patch: ​https://github.com/django/django/pull/12321

FYI" I am on macOS 10.14.6

I'm not sure how best to adjust default_test_processes as i've always used the --parallel flag with a parameter.
Also, could you provide some guidance on how you'd like this tested

Last edited 4 years ago by Brandon Navra (previous) (diff)

comment:8 by Carlton Gibson, 4 years ago

FYI" I am on macOS 10.14.6

Super. I had a 3.7. env active. I can reproduce with Python 3.8.

comment:9 by Brandon Navra, 4 years ago

Has patch: set
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:10 by Thierry Bastian, 4 years ago

Thanks for the report. I had the same issue but did not find the root cause in https://code.djangoproject.com/ticket/31116.
I would love to see that being resolved.

comment:11 by Carlton Gibson, 4 years ago

Patch needs improvement: set

comment:12 by Peter Inglesby, 4 years ago

I ran into this while running the Django test suite, and when applying the patch in ​PR 12321, I get the same problem with a different exception:

Traceback (most recent call last):
  File "/Users/inglesp/.pyenv/versions/3.8.0/lib/python3.8/multiprocessing/process.py", line 313, in _bootstrap
    self.run()
  File "/Users/inglesp/.pyenv/versions/3.8.0/lib/python3.8/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/inglesp/.pyenv/versions/3.8.0/lib/python3.8/multiprocessing/pool.py", line 114, in worker
    task = get()
  File "/Users/inglesp/.pyenv/versions/3.8.0/lib/python3.8/multiprocessing/queues.py", line 358, in get
    return _ForkingPickler.loads(res)
  File "/Users/inglesp/src/django/django/tests/fixtures_regress/tests.py", line 18, in <module>
    from .models import (
  File "/Users/inglesp/src/django/django/tests/fixtures_regress/models.py", line 1, in <module>
    from django.contrib.auth.models import User
  File "/Users/inglesp/src/django/django/django/contrib/auth/models.py", line 3, in <module>
    from django.contrib.contenttypes.models import ContentType
  File "/Users/inglesp/src/django/django/django/contrib/contenttypes/models.py", line 133, in <module>
    class ContentType(models.Model):
  File "/Users/inglesp/src/django/django/django/db/models/base.py", line 113, in __new__
    raise RuntimeError(
RuntimeError: Model class django.contrib.contenttypes.models.ContentType doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.

I'm on OSX 10.15 with Python 3.8.

comment:13 by Abhijeet Viswa, 4 years ago

Cc: Abhijeet Viswa added

comment:14 by Ichlasul Affan, 4 years ago

Cc: Ichlasul Affan added

comment:15 by Ahmad A. Hussein, 4 years ago

Has patch: unset
Owner: changed from nobody to Ahmad A. Hussein
Patch needs improvement: unset
Status: new → assigned

comment:16 by Adam Johnson, 4 years ago

For those looking for a workaround, here's how to add the appropriate call to reset back to fork mode: ​https://adamj.eu/tech/2020/07/21/how-to-use-djangos-parallel-testing-on-macos-with-python-3.8-plus/

comment:17 by Ahmad A. Hussein, 3 years ago

Has patch: set

comment:18 by Abhijeet Viswa, 3 years ago

Cc: Abhijeet Viswa removed

comment:19 by Ryan Siemens, 3 years ago

Cc: Ryan Siemens added

comment:20 by Carlton Gibson, 3 years ago

Patch needs improvement: set

PR is working nicely on macOS but needs a rebase, and a refactoring for review comments.

comment:21 by Adam Wróbel, 2 years ago

Cc: Adam Wróbel added

comment:22 by David Smith, 2 years ago

Owner: changed from Ahmad A. Hussein to David Smith
Patch needs improvement: unset

comment:23 by Carlton Gibson, 2 years ago

Patch needs improvement: set

A few issues on the PR to resolve, but looking very promising: 39.813s vs 249.146s on macOS, which is a bit of a speed-up 🙂

comment:24 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In ae91ecf6:

Refs #31169 -- Added DatabaseCreation.setup_worker_connection() hook.

comment:25 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

comment:26 by GitHub <noreply@…>, 2 years ago

In 795da630:

Refs #31169 -- Prevented infinite loop in tests on failures.

Regression in ae91ecf6a1037fb67d14841b66ac19d4c2ccc4ac.

comment:27 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set
Triage Stage: Ready for checkin → Accepted

comment:28 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

comment:29 by Carlton Gibson <carlton@…>, 2 years ago

Resolution: → fixed
Status: assigned → closed

In 3b3f38b:

Fixed #31169 -- Adapted the parallel test runner to use spawn.

Co-authored-by: Valz <ahmadahussein0@…>
Co-authored-by: Nick Pope <nick@…>

comment:30 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In ba298a32:

Refs #31169 -- Prevented infinite loop in parallel tests with custom test runner when using spawn.

Regression in 3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@…>

comment:31 by GitHub <noreply@…>, 17 months ago

In 20d575b:

Refs #31169 -- Skipped test_get_test_db_clone_settings_not_supported on not in-memory SQLite database.

multiprocessing's start method is checked only for in-memory SQLite
databases.

Note: See TracTickets for help on using tickets.
Back to Top