Skip to content
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.

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

Fixed #32827 -- Adjusted docs on squashing migrations. #14408

Closed
wants to merge 2 commits into from

Conversation

mlissner
Copy link
Contributor

Pursuant to the discussion on the Django Dev list, this is a first pass at updating the squashmigrations documentation. The goal is to demote the squashmigrations approach and to instead prescribe a manual "trimming" process.

I think this is pretty good, but I'm sure it could benefit from other eyes. A couple notes:

  • I decided to coin a new term for this process called "Trimming migrations". The idea is that squashing is a way of trimming migrations as is manually deleting them. I think the term is pretty accurate (and unused in other contexts), but I'd be open to other ideas.

  • I tried to cover the use case of trimming all apps at the same time or just trimming a subset of them. I opted not to get into the details too much about migration dependencies.

  • I think there's a lot more manual guidance in here than in most of the Django docs (e.g., run this, run that). Maybe that's fine and it's what the process requires. I couldn't think of a good way around it. I think the manual process here basically says loudly to the reader the honest truth than this is a hacky situation, and that's probably fine. It's not the polish we want in Django, but, frankly, this area of Django isn't polished.

  • I searched around for references in the docs to squashing and didn't find any others that needed updates.

  • In this post, they have a different approach they use for custom User models. I don't understand why, so I left it out, they make it seem important. Anybody know what's going on there?

  • I haven't tried this process out yet (I'm just barely finding time to push through this PR), but I think I understand migrations enough that the process is sound.

I'd welcome any comments or suggestions. This is the biggest documentation PR I've filed in Django. Hopefully it's good!

Copy link

@iamdevvalecha iamdevvalecha left a comment

Choose a reason for hiding this comment

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

LGTM!

@rfleschenberg
Copy link
Contributor

In this post, they have a different approach they use for custom User models. I don't understand why, so I left it out, they make it seem important. Anybody know what's going on there?

@jessamynsmith if you have a few minutes, can you give some input on this maybe?

Copy link
Contributor

@rfleschenberg rfleschenberg left a comment

Choose a reason for hiding this comment

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

Looks quite good to me. The custom user model case warrants some investigation / testing, I think.

docs/topics/migrations.txt Outdated Show resolved Hide resolved
@mlissner
Copy link
Contributor Author

@jessamynsmith, I used your blog post to write part of this PR on the django docs, but I don't completely understand what you're doing with the User model. Any chance you can chime in?

@mekhami
Copy link

mekhami commented May 19, 2021

Personally (and this is extremely nitpicky) but I'd leave out the part with the find command and give generic instructions on 'removing all the .py files' or something similar, in order to make the documentation less unix-specific.

@mlissner
Copy link
Contributor Author

Not sure why the test is failing. I don't think it's my fault.

For the bit about the find command, I debated it too, and I'll defer to others, but I put it in because it will actually make the process easier for most folks, which I figured was the goal. But if others think it should go, I'll yank it.

@jessamynsmith
Copy link

In this post, they have a different approach they use for custom User models. I don't understand why, so I left it out, they make it seem important. Anybody know what's going on there?

@jessamynsmith if you have a few minutes, can you give some input on this maybe?

It has been a long time since I did this, but if I recall correctly, deleting the migration that created the custom user model put me in a state were I could not even run makemigrations. Which makes sense in a way, since Django needs a user model. I do not know if this is still true in the most recent version of Django.

@mekhami
Copy link

mekhami commented May 19, 2021

Not sure why the test is failing. I don't think it's my fault.

For the bit about the find command, I debated it too, and I'll defer to others, but I put it in because it will actually make the process easier for most folks, which I figured was the goal. But if others think it should go, I'll yank it.

maybe leave it in there as a sidenote, but the main point of the docs might read "Delete the .py files in the migrations folder for that application. (If you're on a unix system, blah blah etc will do the trick.)

something to that effect, more elegantly written than i can do in a github comment =P

@WhyNotHugo
Copy link
Contributor

Huh, I've always taken a hybrid approach. I deleted migrations and created new ones from scratch, but then added the same replaces field that squashmigrations would add.

class Migration(migrations.Migration):

    # Mark this as replacing all previous migrations, as if this were a squash.
    replaces = [
        ("idf", "0001_initial"),
        #...
    ]

It's then treated like a squash, but it's generated like a trim. The upside is that no extra commands need to be run in production services (it's even doable with reusable apps).

@mlissner
Copy link
Contributor Author

In freelawproject/courtlistener#1694, I just used these instructions to clean up about 300 extraneous migrations. The instructions worked well, but I did make some tweaks which I just force pushed.

I think they're pretty good now. Anything else needed before we merge?

@mlissner
Copy link
Contributor Author

mlissner commented Jun 7, 2021

I'm a bit crazy about leaving work half done for fear that it never gets done. Is there something I need to do here to make sure it gets back in the review queue or something?

@jacobtylerwalls
Copy link
Member

I'm a bit crazy about leaving work half done for fear that it never gets done. Is there something I need to do here to make sure it gets back in the review queue or something?

Yes a new ticket for it would be great, thanks (unless there is a dupe somewhere!)

@mlissner
Copy link
Contributor Author

mlissner commented Jun 7, 2021

OK, I created one here and it has the has_patch bool set to true: https://code.djangoproject.com/ticket/32827

@carltongibson carltongibson changed the title Docs: Trim migrations manually, don't squash them Fixed #32827 -- Adjusted docs advice on squashing migrations. Jun 8, 2021
@carltongibson carltongibson changed the title Fixed #32827 -- Adjusted docs advice on squashing migrations. Fixed #32827 -- Adjusted docs on squashing migrations. Jun 8, 2021
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hey @mlissner. Thanks for this.

I'm not sure about the new Trimming vs Squashing distinction. It seems there's Automatically Squashing vs Manually Squashing. 🤔

I think for a lot of projects squashmigrations works perfectly well. (No? 🤔)
I'd be inclined to put that at the first option and then say, "For complex projects... you may wish to manually squash..."

(Do we describe the Nuke 'em strategy ever? 🤔)

What do you think?

Can I ask you to squash and rename commits as per the patch review checklist. (I renamed the PR as a clue.)

docs/topics/migrations.txt Show resolved Hide resolved
@mlissner
Copy link
Contributor Author

mlissner commented Jun 8, 2021

It seems there's Automatically Squashing vs Manually Squashing.

I'm not married to anything, but I felt like squashing was different enough from deleting files manually b/c when you do it the manual way, you don't end up with squash migration files that summarize all the previous migrations. E.g., after a squash, you might wind up with 0001_initial, 0002_some_tweak, and 0003_squash, so there's a squash. After the manual way, you wind up back at just 0001_initial, which isn't a squash.

I think for a lot of projects squashmigrations works perfectly well. (No?)

I haven't done a survey or anything, but the group discussion seemed to indicate that it mostly doesn't work for folks — there was no ringing endorsement at least. A few responses from there:

I've had similar issues. I just avoid squashing anymore.

I suspect that squashing works a lot better if you aren’t trying to clean up a mess of hundreds of migrations files

My experience also corroborates the idea that squashmigrations may be unsuitable for many situation that are similar to mine,

I agree that it would be good to extend the docs and to describe how to reset a project's migrations.

On the other side (kinda):

We’ve done it successfully a few times since 1.11… but not without a sacrifice of the virgins-in-volcano variety....sometimes it’s easier to blow them all away and create fresh migrations...I have faith in migration squashing. It feels like more art than science,

The only problem I have is a pbcak problem

So I don't know. My initial question was whether squashing works generally for folks, because it totally failed for me. What inspired me to ask was that I had put off squashing for a long time, was excited to finally do it, but then totally disappointed by how completely unusable it was. From my experience, Django has a problem that migrations pile up and the one documented way to fix it often doesn't work, so my solution was to write up some docs, and I prioritized nuke 'em over squashing based on the comments above.

I also said in the discussion:

the docs could go on to explain first how to do this manually, then move onto the squashmigrations docs. This disfavors squashmigrations by putting it after the manual approach, but after this conversation (and my experience) that seems right to me.

Nobody disagreed with that plan, so I wrote up this PR.

Finally, you asked:

(Do we describe the Nuke 'em strategy ever?)

I'm not sure what you mean by this. Don't most of the instructions in the PR describe the Nuke 'em strategy? Maybe we have a miscommunication...hopefully not!

Can I ask you to squash and rename commits as per the patch review checklist. (I renamed the PR as a clue.)

Sure. I'll do that once we have consensus re the above.

@carltongibson
Copy link
Member

carltongibson commented Jun 8, 2021

Hey @mlissner.

By Nuke 'em I mean "Delete all the migrations entirely, clear out the migrations table, and create a fresh set". Update: I see now that that's what your Trimming amounts to.

I use squashmigrations regularly/frequently. In my experience if you do that it kind of just works most times. (If you try and squash 200 migrations, I'd expect less success... — depends on the project I suppose 🤔 — but I'd probably Nuke 'em in that case.)

I guess I don't really see that we should steer folks away from squashmigrations as a good first option. 🤔 Maybe others think otherwise. Maybe it's that folks don't use this until they're quite a way in...

Comment on lines +655 to +662
The easiest and most reliable way to trim migrations is to do so manually,
instead of using the ``squashmigrations`` command. When doing this process, any
apps that have foreign key interdependencies will need to be trimmed at the same
time so that old migrations do not depend on deleted migrations. For example,
if your ``pizza`` app has a ``ForeignKey`` to an ``Ingredients`` model in your
``food`` app, you will need to manually trim both the ``food`` and the
``pizza`` apps simultaneously. It is often easiest to trim all apps in your
project at the same time.
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs an introductory paragraph saying what you're going to do, then this warning about the need to be up to date, then the steps.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

I haven't done a survey or anything, but the group discussion seemed to indicate that it mostly doesn't work for folks — there was no ringing endorsement at least.

My impression from reading the thread plus these proposed changes was that the chief pain point was needing to manually rectify circular dependencies (ticket-23337). I left a suggestion below to make this more explicit. It would be shame for it to sound like we're telling folks "here be dragons" but "we're not saying where". Unless we want to give off the undertone that we're done maintaining the feature, but folks are still doing patches (🤚🏻 !), so it would also be a shame to discourage new contributions in this area. If that makes any sense.

Aymeric has comments in an old thread about resolving circular dependencies BEFORE squashing by restructuring apps, where feasible. I'm not saying we need to add this to this PR, I'm just mentioning how squashmigrations isn't totally useless even when you get into trouble:

We did only one complicated thing. We moved our custom user model because of a
circular import problem. (Pro-tip 1: put your custom user model in an app
that doesn't depend on anything else. Perhaps we should document that.) We
papered over the resulting mess by squashing migrations and life was good.
(Pro-tip 2: if you change AUTH_USER_MODEL, throw your migrations history away
and regenerate it. It's easy. We should definitely document how to do that.)

I agree with Carlton that "trimming" is an extra metaphor that becomes difficult to track alongside "squashing". I already find squashing difficult to keep straight (which one is "squashed", the one that got squashed "away" or the one that represents "the entire squashed set" -- I know it's the latter, but I always have to think twice. And I'm a native English speaker.)

rolled into :class:`~django.db.migrations.operations.CreateModel`.
There are two common approaches to trimming migrations. The first is to use the
``squashmigrations`` command to create a merged migration file, however this
approach can be impractical on complex projects. The second approach is to
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
approach can be impractical on complex projects. The second approach is to
approach can be impractical for projects exhibiting circular dependencies between
apps, which will require manual resolution anyway. The second approach is more
manual but potentially simpler: to coordinate ... and to ... delete and recreate ...

@mlissner
Copy link
Contributor Author

mlissner commented Jun 8, 2021

Thank you both. Lots of good points. I'll take a stab at revising and incorporating these thoughts soon.

@nessita
Copy link
Contributor

nessita commented Apr 28, 2023

Hello! I'm doing some PR cleanup. Do you have time to keep working on this? Thank you!

@mlissner
Copy link
Contributor Author

Sorry, I don't think so. I guess I'll close this out. Hopefully folks doing migration squashing/trimming will find it useful, even if it's not in the docs.

@mlissner mlissner closed this Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants