Open humitos opened 2 years ago
In fact, pytest
shows that we are running these migrations twice. I'm not sure yet why this is happening. Currently, running 1 single test consumes 1m8.35s.
Commenting the telemetry
database, these migrations are run just once and the same test consumes 0m41s. 27s less than using the telemetry
database: https://github.com/readthedocs/readthedocs.org/blob/bdf154a35ab40343a3897b9db99cd09a2e1ac67f/readthedocs/settings/dev.py#L22-L25
Why these migrations are run in both databases? In telemetry
database we only need migrations from readthedocs.telemetry
application. cc @stsewd
Why these migrations are run in both databases? In telemetry database we only need migrations from readthedocs.telemetry
Only the migrations from the telemetry app are run, the others are skipped, I assume they are faked, since you'll always see those migrations as applied, but the DB has only the telemetry tables.
This could potentially be a low-hanging fruit. Have your tried quickly to squash @humitos? The typical issue would be that data migrations (stuff with RunPython
) needs to be manually fixed in the auto-created squash migration.
@benjaoming yeah, I did a quick try and I found that we have some RunPython
code on the migrations. However, I think most of them could be omitted in the resulting squashed migration since this code is not run in production.
The immediate main issue that I found was the inter-dependency order the migrations need to be run, meaning that we can't squash them all into just one migration per application. I'm not sure how to easily find where they should be split to make this dependency order work fine.
@stsewd
Only the migrations from the telemetry app are run, the others are skipped, I assume they are faked, since you'll always see those migrations as applied, but the DB has only the telemetry tables.
There is something else happening behind the scenes, because commenting the telemetry
database the execution time for the same single test is reduced in 27 seconds.
The verbose output of tox/pytest (tox -e py310 --verbose -- -vvv -s -k test_revoke_expired_invitation
) shows the migrations are effectively run twice with the current configuration and you can see the time each one of them takes next to the name of the migration.
Another thing that I found is that djstripe
migrations take about 20s
So, there are some work required here that are related but they don't depend each other:
readthedocs
application migrations on telemetry
databasedjstripe
migrations on testsThe verbose output of tox/pytest (tox -e py310 --verbose -- -vvv -s -k test_revoke_expired_invitation) shows the migrations are effectively run twice with the current configuration and you can see the time each one of them takes next to the name of the migration.
Yes, they appear as "run", but only the migrations from the telemetry app are in the telemetry db. This same behavior happens when you first create the docker container for development, all migrations appear in the output as applied, but they aren't really applied in the telemetry db. And I just checked the telemetry db inside a test, and only the telemetry table is there. So I guess django still takes some time to fake those migrations.
Yeah, I'm not saying the tables are created on the telemetry database. I'm saying that the migrations are run twice and consuming the double of time they should
I usually run tox
without the migrations, which should solve this for the most part: https://pytest-django.readthedocs.io/en/latest/database.html#no-migrations-disable-django-migrations. Not necessarily a solution, but at least a workaround.
@ericholscher I've been testing the --nomigrations
suggestion here. It works super fast. However, there are cases where it leads to different results (i.e. all my tests were passing locally, but they failed on CI).
Definitely all the migrations are run one time per database configured. This is how Django works with multiple databases. Then, while running Django decide if the migrations has to be applied to the database or not; but it's run. That's why we are seeing the migrations ran twice but not all tables are not created in telemetry
db.
Applying djstripe.0007_2_4... OK (11.026s)
...
Applying djstripe.0007_2_4... OK (11.014s)
Just by running those 2 djstripe
migrations, we are wasting 22s.
When running tests locally we are executing 252 migrations that consume 33.43 seconds. This is tedious when running just a single test. I think we can reduce this time by squashing all the migrations so we have only one migration per application. What do you think?
Documentation: https://docs.djangoproject.com/en/4.1/topics/migrations/#migration-squashing