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.

Sign up for GitHub

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

Jump to bottom

fix(form): optimize calculated question performance #2339

Merged
merged 13 commits into from
Jan 9, 2025
Merged

fix(form): optimize calculated question performance #2339

merged 13 commits into from
Jan 9, 2025

Conversation

czosel
Copy link
Contributor

@czosel czosel commented Dec 2, 2024 •
edited
Loading

  • drop several signals in favor of explicit calls in domain logic to allow reuse of form structure
  • add prefetching for document structure
  • apply topological sorting before calculating answers on document creation

@luytena
Copy link
Contributor

luytena commented Dec 10, 2024

@czosel Note: Instance creation is still slow, and seems to be triggered by create in SaveDocumentLogic. I'm having a look.

@luytena
Copy link
Contributor

luytena commented Dec 10, 2024

Form changes (e.g. through form-builder) are probably also still slow.

@czosel czosel force-pushed the fix-calculated-question-performance branch 4 times, most recently from a47c6e1 to 33746a0 Compare December 31, 2024 08:18
@czosel czosel changed the title fix(form): optimize calculated question performance (wip) fix(form): optimize calculated question performance Dec 31, 2024
@czosel czosel force-pushed the fix-calculated-question-performance branch 2 times, most recently from 4584728 to d1bd793 Compare December 31, 2024 08:27
czosel
czosel commented Dec 31, 2024
View reviewed changes
caluma/caluma_form/signals.py Outdated Show resolved Hide resolved
@czosel czosel force-pushed the fix-calculated-question-performance branch from d1bd793 to ad4d2e7 Compare December 31, 2024 08:34
@czosel czosel marked this pull request as ready for review December 31, 2024 08:38
@czosel czosel force-pushed the fix-calculated-question-performance branch 2 times, most recently from 0426441 to af7e014 Compare December 31, 2024 11:08
@czosel czosel requested a review from winged December 31, 2024 12:01
winged
winged requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Nice work so far! Not sure we're all the way there yet tho :-)

caluma/caluma_form/tests/test_question.py Show resolved Hide resolved
caluma/caluma_form/tests/test_question.py Outdated Show resolved Hide resolved
caluma/caluma_form/utils.py Outdated Show resolved Hide resolved
caluma/caluma_form/domain_logic.py Outdated Show resolved Hide resolved
caluma/caluma_form/domain_logic.py Outdated Show resolved Hide resolved
@open-dynaMIX open-dynaMIX mentioned this pull request Jan 6, 2025
Open
@czosel czosel force-pushed the fix-calculated-question-performance branch from 3ba8809 to 0788067 Compare January 9, 2025 10:15
winged
winged requested changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@winged winged left a comment

Choose a reason for hiding this comment

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

Nice work so far, just a few suggestions

caluma/caluma_form/signals.py Outdated Show resolved Hide resolved
caluma/caluma_form/utils.py Outdated Show resolved Hide resolved
@czosel czosel force-pushed the fix-calculated-question-performance branch from 2fecfe2 to a5eb854 Compare January 9, 2025 10:45
@czosel czosel force-pushed the fix-calculated-question-performance branch from 1d879c7 to dc0e4eb Compare January 9, 2025 12:19
@czosel
Copy link
Contributor Author

czosel commented Jan 9, 2025

@winged I think we should squash this on merge. I enabled squashing in the repo settings, just not sure if it'll mess up the commit message.

@winged winged enabled auto-merge (squash) January 9, 2025 12:40
@winged winged merged commit db6468b into main Jan 9, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winged winged winged approved these changes

@open-dynaMIX open-dynaMIX Awaiting requested review from open-dynaMIX

@luytena luytena Awaiting requested review from luytena

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@czosel @luytena @winged @open-dynaMIX