Skip to content

Fixed #31169 -- Adapted the parallel test runner to use spawn. [GSoC] #12646

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fixed #31169 -- Adapted the parallel test runner to use spawn. [GSoC] #12646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions django/db/backends/base/creation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,3 +340,13 @@ def test_db_signature(self):
settings_dict['ENGINE'],
self._get_test_db_name(),
)

def setup_worker_connection(self, _worker_id):
Copy link
Member

Choose a reason for hiding this comment

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

I moved this hook to a separate PR, see #15457.

settings_dict = self.get_test_db_clone_settings(str(_worker_id))
# connection.settings_dict must be updated in place for changes to be
# reflected in django.db.connections. If the following line assigned
# connection.settings_dict = settings_dict, new threads would connect
# to the default database instead of the appropriate clone.
self.connection.settings_dict.update(settings_dict)
self.mark_expected_failures_and_skips()
self.connection.close()
25 changes: 24 additions & 1 deletion django/db/backends/sqlite3/creation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import multiprocessing
import os
import shutil
import sqlite3
import sys
from pathlib import Path

Expand Down Expand Up @@ -52,7 +54,10 @@ def get_test_db_clone_settings(self, suffix):
orig_settings_dict = self.connection.settings_dict
source_database_name = orig_settings_dict['NAME']
if self.is_in_memory_db(source_database_name):
return orig_settings_dict
if multiprocessing.get_start_method() == 'spawn':
return {**orig_settings_dict, 'NAME': f'{self.connection.alias}_{suffix}.sqlite3'}
elif multiprocessing.get_start_method() == 'fork':
return orig_settings_dict
Comment on lines +57 to +60
Copy link
Member

Choose a reason for hiding this comment

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

As the start method can also be forkserver we ought to guard against that and any potential future values instead of falling through to the implicit return None.

In addition, we can remove some indentation here:

    def get_test_db_clone_settings(self, suffix):
        settings_dict = self.connection.settings_dict

        source_database_name = settings_dict['NAME']
        if not self.is_in_memory_db(source_database_name):
            root, ext = os.path.splitext(source_database_name)
            return {**settings_dict, 'NAME': f'{root}_{suffix}{ext}'

        start_method = multiprocessing.get_start_method()
        if start_method == 'fork':
            return settings_dict
        if start_method == 'spawn':
            return {**settings_dict, 'NAME': f'{self.connection.alias}_{suffix}.sqlite3'}
        raise NotImplementedError(f'Cloning with start method {start_method!r} is not supported.')

Don't forget to add a test for the exception.

else:
root, ext = os.path.splitext(orig_settings_dict['NAME'])
return {**orig_settings_dict, 'NAME': '{}_{}{}'.format(root, suffix, ext)}
Expand Down Expand Up @@ -80,6 +85,10 @@ def _clone_test_db(self, suffix, verbosity, keepdb=False):
except Exception as e:
self.log('Got an error cloning the test database: %s' % e)
sys.exit(2)
else:
if multiprocessing.get_start_method() == 'spawn':
Comment on lines +88 to +89
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else:
if multiprocessing.get_start_method() == 'spawn':
elif multiprocessing.get_start_method() == 'spawn':

ondisk_db = sqlite3.connect(target_database_name, uri=True)
self.connection.connection.backup(ondisk_db)

def _destroy_test_db(self, test_database_name, verbosity):
if test_database_name and not self.is_in_memory_db(test_database_name):
Expand All @@ -101,3 +110,17 @@ def test_db_signature(self):
else:
sig.append(test_database_name)
return tuple(sig)

def setup_worker_connection(self, _worker_id):
alias = self.connection.alias
worker_db = f'file:memorydb_{str(alias)}_{str(_worker_id)}?mode=memory&cache=shared'
sourcedb = sqlite3.connect(f'file:{str(alias)}_{str(_worker_id)}.sqlite3', uri=True)
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

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

The naming is inconsistent worker_db, sourcedb, second_db, also f-strings should not contain function calls. This guideline is from Python coding style. IMO, it's more readable with %-formatting:

Suggested change
worker_db = f'file:memorydb_{str(alias)}_{str(_worker_id)}?mode=memory&cache=shared'
sourcedb = sqlite3.connect(f'file:{str(alias)}_{str(_worker_id)}.sqlite3', uri=True)
worker_db = 'file:memorydb_%s_%s?mode=memory&cache=shared' % (alias, _worker_id)
source_db = sqlite3.connect('file:%s_%s.sqlite3' % (alias, _worker_id), uri=True)

I would try to reuse get_test_db_clone_settings() 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing something, but why do these need to be wrapped with str() anyway? You could do {alias!s} instead of {str(alias)}, but if it isn't required, just use {alias}. In that case you can use f-strings.

second_db = sqlite3.connect(worker_db, uri=True)
sourcedb.backup(second_db)
sourcedb.close()
settings_dict = self.connection.settings_dict
settings_dict['NAME'] = worker_db
self.connection.settings_dict.update(settings_dict)
Comment on lines +121 to +123
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. Is there something incorrect here or can this just be:

Suggested change
settings_dict = self.connection.settings_dict
settings_dict['NAME'] = worker_db
self.connection.settings_dict.update(settings_dict)
self.connection.settings_dict['NAME'] = worker_db

If the intent was to avoid mutating the original then do this (although I'm not sure it's required):

        self.connection.settings_dict = {**self.connection.settings_dict, 'NAME': worker_db}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly not sure why I did it this way, will change it & see if something breaks

self.connection.connect()
second_db.close()
self.mark_expected_failures_and_skips()
3 changes: 2 additions & 1 deletion django/db/backends/sqlite3/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from django.db.backends.base.features import BaseDatabaseFeatures
from django.db.utils import OperationalError
from django.utils.functional import cached_property
from django.utils.version import PY37

from .base import Database

Expand All @@ -24,7 +25,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
can_rollback_ddl = True
can_create_inline_fk = False
supports_paramstyle_pyformat = False
can_clone_databases = True
can_clone_databases = PY37
Copy link
Member

Choose a reason for hiding this comment

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

Django 4.0+ will not support Python 3.7 anymore:

Suggested change
can_clone_databases = PY37
can_clone_databases = True

supports_temporal_subtraction = True
ignores_table_name_case = True
supports_cast_with_precision = False
Expand Down
Loading