Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#23745 closed Cleanup/optimization (fixed)

Migrations migrate is slow

Reported by: Claude Paroz Owned by: nobody
Component: Migrations Version: dev
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 by Claude Paroz, 9 years ago

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 by Claude Paroz, 9 years ago

Patch needs improvement: set

comment:3 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Claude Paroz, 9 years ago

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)
Last edited 9 years ago by Markus Holtermann (previous) (diff)

comment:5 by Tim Graham, 9 years ago

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 by Claude Paroz, 9 years ago

Severity: NormalRelease blocker

comment:7 by Tim Graham, 9 years ago

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 by Tim Graham, 9 years ago

Test for ignore_swappable from Markus has been added in fca866763acb6b3414c20ca3772b94cb5d111733.

comment:9 by Tim Graham, 9 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Claude Paroz <claude@…>, 9 years ago

In a159b1facd222c20dde12f1e0117643af528d511:

Replaced migration state render() by apps cached property

Refs #23745.

comment:11 by Claude Paroz <claude@…>, 9 years ago

In 057305e588ac90474fef47e2fb2890552a543731:

Added ignore_swappable to StateApps

Refs #23745.

comment:12 by Claude Paroz <claude@…>, 9 years ago

In 2a9c4b4901d385f68a2c3f9cf98b7a776c2976f0:

Passed around the state between migrations

Refs #23745.

comment:13 by Claude Paroz <claude@…>, 9 years ago

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 by sirex, 9 years ago

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 by sirex, 9 years ago

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 by sirex, 9 years ago

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 by Tim Graham, 9 years ago

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 by Venelin Stoykov, 8 years ago

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 by Tim Graham, 8 years ago

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