Skip to content

Conversation

@chris48s
Copy link
Member

@chris48s chris48s commented Oct 15, 2025

Refs https://app.asana.com/1/1204880536137786/project/1204880927741389/task/1211648460534610?focus=true

Quick re-cap on this one:

In this PR, I am doing the upgrade a second time but this time in a way that retains reasonable performance. I've broadly succeeded. This version makes a lot fewer round-trips to the DB than the previous version we deployed. That said, it is worth noting that this version of django-formtools does make more DB requests than the previous one we were on and I don't think that is completely fixable in userspace.

There is an open PR upstream that might help with this if it is ever merged jazzband/django-formtools#291

@coveralls
Copy link

coveralls commented Oct 15, 2025

Coverage Status

coverage: 72.504% (+0.08%) from 72.424%
when pulling 9270a32 on form-tools20251015
into 8d39d10 on master.

@chris48s chris48s changed the title upgrade django-formtools==2.5.1 (attempt 2) upgrade django-formtools==2.5.1 Oct 15, 2025
@chris48s chris48s marked this pull request as ready for review October 15, 2025 14:26
@chris48s chris48s changed the title upgrade django-formtools==2.5.1 upgrade django-formtools==2.5.1 (attempt 2) Oct 15, 2025
Copy link
Collaborator

@GeoWill GeoWill left a comment

Choose a reason for hiding this comment

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

I think you've tried this out on the dev site. I've had a go locally. Nothing broken, and query counts aren't stupid.

I did have a thought over the weekend that one way we could test this is by looking at the pg_stat_* tables. But I just had a very quick play and didn't get anywhere with it, so don't think it's worth blocking over. Just thought I'd throw it out there as a possible future idea.

@chris48s chris48s merged commit 1559ae7 into master Oct 21, 2025
2 checks passed
@chris48s
Copy link
Member Author

Bah I meant to revert 9270a32 before I merged this 🤦
I'll fix it when I merge #2536 in a sec

@knyghty
Copy link

knyghty commented Oct 23, 2025

Hi @chris48s, I noticed this when working again on the upstream PR for formtools.

It would be great if you could test the branch of my PR on this project, even if only locally, and report back upstream if you run into any issues. It may help get the PR merged faster.

@chris48s
Copy link
Member Author

Hi @knyghty

Thanks for your upstream work on this.

I installed

django-formtools = { git = "https://github.com/knyghty/django-formtools", rev = "f6665ed0db1b7238872fa2751ce74b808e8194d1" }

locally and had a quick play with django debug toolbar open.

The good news is:

  • Using your changes reduces the number of round-trips to the DB needed to render each step in the wizard
  • I tried rolling back all the optimisations I applied in this PR and the number of round-trips to the DB stayed about the same without them. So if your PR is merged upstream, we could get rid of some messy workarounds and optimisations we've had to apply here
  • I did not encounter any bugs or errors while testing.

That said, cache invalidation bugs can often be subtle and the testing I've done was not exhaustive.

Thanks again for your help on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants