Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#23745 closed Cleanup/optimization (fixed)

Migrations migrate is slow

Reported by: Claude Paroz Owned by: nobody
Component: Migrations Version: master
Severity: Release blocker Keywords:
Cc: 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

Slowness around the optimizer is tracked in #22608. This ticket is specifically targeted to slowness with the migrate command. To repeat a comment previously done in #22608, each migration in the Django test suite is taking up 3.5 seconds. That should be improved.

Profiling migrations in the current test suite:

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000   27.714   27.714 django/db/migrations/executor.py:55(migrate)
        8    0.000    0.000   27.714    3.464 django/db/migrations/executor.py:83(apply_migration)
        8    0.042    0.005   13.157    1.645 django/db/migrations/executor.py:128(detect_soft_applied)
        8    0.000    0.000   14.500    1.812 django/db/migrations/migration.py:79(apply)
        8    0.042    0.005   12.945    1.618 django/db/migrations/operations/models.py:32(database_forwards)
       17    0.094    0.006   27.447    1.615 django/db/migrations/state.py:42(ProjectState.render)
    21511    0.791    0.000   21.881    0.001 django/db/migrations/state.py:293(ModelState.render)
    21488    0.549    0.000    5.290    0.000 django/db/migrations/state.py:164(ModelState.from_model)
21515/21511    0.766    0.000   15.861    0.001 django/db/models/base.py:63(ModelBase.__new__)

Change History (19)

comment:1 Changed 5 years ago by Claude Paroz

Has patch: set

See https://github.com/django/django/pull/3458. With that patch, each migration is now only taking 0.5 sec. (down from 3.5 sec.).

comment:2 Changed 5 years ago by Claude Paroz

Patch needs improvement: set

comment:3 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:4 Changed 5 years ago by Claude Paroz

Patch needs improvement: unset

New try: https://github.com/django/django/pull/3484

Before:

Applying admin.0001_initial... OK (2.9538629055)
Applying commands_sql_migrations.0001_initial... OK (3.98874592781)
Applying fixtures_migration.0001_initial... OK (2.9278049469)
Applying sites.0001_initial... OK (2.65014600754)
Applying flatpages.0001_initial... OK (2.63765001297)
Applying migration_test_data_persistence.0001_initial... OK (2.87452602386)
Applying migration_test_data_persistence.0002_add_book... OK (2.58737111092)
Applying postgres_tests.0001_setup_extensions... OK (1.30905008316)
Applying postgres_tests.0002_create_test_models... OK (9.76085591316)
Applying redirects.0001_initial... OK (3.11744999886)
Applying sessions.0001_initial... OK (2.7917330265)

After:

Applying admin.0001_initial... OK (2.94719195366)
Applying commands_sql_migrations.0001_initial... OK (0.251703977585)
Applying fixtures_migration.0001_initial... OK (0.135380983353)
Applying sites.0001_initial... OK (0.141701936722)
Applying flatpages.0001_initial... OK (0.408670186996)
Applying migration_test_data_persistence.0001_initial... OK (0.138519048691)
Applying migration_test_data_persistence.0002_add_book... OK (0.0787410736084)
Applying postgres_tests.0001_setup_extensions... OK (0.284782886505)
Applying postgres_tests.0002_create_test_models... OK (0.369908094406)
Applying redirects.0001_initial... OK (0.170851945877)
Applying sessions.0001_initial... OK (0.371510028839)

Version 0, edited 5 years ago by Claude Paroz (next)

comment:5 Changed 5 years ago by Tim Graham

Patch needs improvement: set

As noted on the PR, "There is still an unsolved issue, though, with the line if app_label == settings.AUTH_USER_MODEL and ignore_swappable: which is untested and would fail as ignore_swappable is no more."

comment:6 Changed 5 years ago by Claude Paroz

Severity: NormalRelease blocker

comment:7 Changed 5 years ago by Tim Graham

I did some research, and I can hit the logic in question:

if app_label == settings.AUTH_USER_MODEL and ignore_swappable:
   continue

on both master and with Claude's patch if I follow the steps in this comment: https://code.djangoproject.com/ticket/22563#comment:8

Andrew first added an error message saying that changing AUTH_USER_MODEL after creating an initial migration was invalid:
https://github.com/django/django/commit/fc974313b85da932a70f1f993b33207d78d31831

and then someone reporting hitting that error with a false positive and he changed it to the current version:
https://github.com/django/django/commit/5182efce8d73ec552a1d7f3e86a24369bb261044

His comment was "I've committed a patch that suppresses the error in that one case (where we know it's safe to do so), and I can now switch AUTH_USER_MODEL even midway through a migration set without errors."

comment:8 Changed 5 years ago by Tim Graham

Test for ignore_swappable from Markus has been added in fca866763acb6b3414c20ca3772b94cb5d111733.

comment:9 Changed 5 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 Changed 5 years ago by Claude Paroz <claude@…>

In a159b1facd222c20dde12f1e0117643af528d511:

Replaced migration state render() by apps cached property

Refs #23745.

comment:11 Changed 5 years ago by Claude Paroz <claude@…>

In 057305e588ac90474fef47e2fb2890552a543731:

Added ignore_swappable to StateApps

Refs #23745.

comment:12 Changed 5 years ago by Claude Paroz <claude@…>

In 2a9c4b4901d385f68a2c3f9cf98b7a776c2976f0:

Passed around the state between migrations

Refs #23745.

comment:13 Changed 5 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 1aa3e09c2043c88a760e8b73fb95dc8f1ffef50e:

Fixed #23745 -- Reused states as much as possible in migrations

Thanks Tim Graham and Markus Holtermann for the reviews.

comment:14 Changed 5 years ago by sirex

Recently I started to work on a project, that uses Django 1.7, and found, that running one small test, takes about 10 seconds, white the test itself takes 0.001s! I love TDD, but with Django 1.7 it became impossible.

With South, there was SOUTH_TESTS_MIGRATE option and with this option running single test was instant.

Also, I have a helper script, that allows to switch database to in memory SQlite easily, this makes tests even faster, because 98% of all tests works perfectly without PostgreSQL. So I only switch to PostgreSQL for running tests, when I need to.

In other words, running one single test has to be instant fast. If Django itself takes 10 seconds to start, it is deal breaker for everyone who uses TDD.

comment:15 Changed 5 years ago by sirex

I think, that would be great if Django could have option for test command to pretend, that none of apps have migrations and would run good old syncdb.

comment:16 Changed 5 years ago by sirex

Here is my suggestion:

if settings.TESTS_MIGRATE:
    call_command(
        'migrate',
        verbosity=max(verbosity - 1, 0),
        interactive=False,
        database=self.connection.alias,
        test_database=True,
        test_flush=True,
    )
else:
    from django.apps import apps
    from django.core.management.commands.migrate import Command as MigrateCommand
    command = MigrateCommand()
    command.verbosity = 0
    command.interactive = False
    command.load_initial_data = False
    app_labels = [app.label for app in apps.get_app_configs()]
    created_models = command.sync_apps(self.connection, app_labels)

This is just a quick and dirty modification of django/db/backends/creation.py:create_test_db, but the idea is to add option for not running migrations in tests.

With this modification, my tests became instant fast again.

comment:17 Changed 5 years ago by Tim Graham

I proposed a similar idea on the mailing list and no one rejected it right away at least. I think it might be accepted as a new feature for 1.9 -- probably as a test runner flag instead of a setting. Could you please open a new ticket (and if you are interested in working on this feature, a pull request)?

comment:18 Changed 4 years ago by Venelin Stoykov

I found a snippet for disabling migrations https://gist.github.com/NotSqrt/5f3c76cd15e40ef62d09 and assembled it in my settings.py to easy turn off them when I want.

if os.getenv('DISABLE_MIGRATIONS'):

    class DisableMigrations(object):

        def __contains__(self, item):
            return True

        def __getitem__(self, item):
            return "notmigrations"


    MIGRATION_MODULES = DisableMigrations()

This is dirty hack put in settings.py but do the job. For Django 1.9 even can be simplified by return None instead of return "notmigrations"

Probably some switch to test command --no-migrations or something that will do something like that can be useful.

comment:19 Changed 4 years ago by Tim Graham

New in Django 1.9, you can disable migrations on a per app basis using the MIGRATION_MODULES setting by setting the app label to None (#24919).

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