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

Remove old squashed migrations #1778

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Remove old squashed migrations #1778

merged 1 commit into from
Dec 14, 2023

Conversation

brianhelba
Copy link
Collaborator

@brianhelba brianhelba commented Dec 8, 2023

We should at least wait until production servers are stable to merge this.

@brianhelba brianhelba changed the title Squash migrations for "api", "analytics", and "zarr" apps Remove old squashed migrations Dec 8, 2023
Base automatically changed from squash-migrations to master December 9, 2023 00:16
@brianhelba brianhelba marked this pull request as ready for review December 9, 2023 00:17
@brianhelba brianhelba marked this pull request as draft December 9, 2023 00:24
@waxlamp
Copy link
Member

waxlamp commented Dec 11, 2023

I've never understood the rationale for squashing migrations. Other frameworks (like SQLAlchemy?) do it as a matter of course, IIUC, so there must be a good reason for it. To me it's a sort of lossy compression so there's some value in keeping it around (to detail the history of migrations). I'm curious what the downside is.

@brianhelba
Copy link
Collaborator Author

One benefit is test performance: each time the tests are run, since a fresh database is used, all migrations must be applied fresh. This isn't typically too big of a performance cost, but eventually it adds up if numerous intermediate tables / columns / constraints need to be created and destroyed just to get to the end state.

This PR was more motivated by the fact that many old the old migrations had names which violated Python naming guidelines (they contained -, instead of only _). Also, some of the now-elided migrations contained raw SQL which was triggering security warnings.

@brianhelba brianhelba marked this pull request as ready for review December 14, 2023 21:50
@brianhelba
Copy link
Collaborator Author

Now that #1777 has been applied to production, this should be safe to merge.

@danlamanna Can you confirm?

@brianhelba
Copy link
Collaborator Author

@danlamanna just confirmed it.

@brianhelba brianhelba merged commit bac29c5 into master Dec 14, 2023
10 checks passed
@brianhelba brianhelba deleted the squash-migrations2 branch December 14, 2023 22:46
@dandibot
Copy link
Member

🚀 PR was released in v0.3.68 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Dec 20, 2023
@aaronkanzer
Copy link
Member

aaronkanzer commented Jan 5, 2024

One benefit is test performance: each time the tests are run, since a fresh database is used, all migrations must be applied fresh. This isn't typically too big of a performance cost, but eventually it adds up if numerous intermediate tables / columns / constraints need to be created and destroyed just to get to the end state.

This PR was more motivated by the fact that many old the old migrations had names which violated Python naming guidelines (they contained -, instead of only _). Also, some of the now-elided migrations contained raw SQL which was triggering security warnings.

Hi @waxlamp and @brianhelba -- just wanted to follow up here....

Squashing of migrations in dandi-archive is bit tricky now for us in linc-archive land, as we rountinely sync our fork with dandi-archive, thus we are susceptible to changes downstream -- e.g. squashing of migrations with not much consultation or planning is disruptive to any linc-archive re-deploys. Furthermore, any changes outside of the routinely automatic Django migrations workflow presents risk.

Also, @brianhelba, in regards to performance bottlenecks in testing, even with limited migrations, I'd encourage the usage of the --reuse-db flag, as it is intended to specifically resolve your pain point here. pytest also provides additional mechanisms to use parallelism/speed up test suites.

I think I'd like to have a larger discussion here (perhaps Dandi standup would be the right forum) for how we navigate over-arching changes in dandi-archive moving forward, as ideally more apps will inherit functionality (linc-archive and hopefully other scientific initiatives) and the intricacies that come with such functionality -- let me know your thoughts.

Cc @kabilar @satra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants