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

Defers content schema initialization until needed. #8456

Merged
merged 1 commit into from Sep 22, 2021

Conversation

rtibbles
Copy link
Member

Summary

  • After some quick profiling of the load times for running the kolibri initialize method, it was clear that we were spending a fair amount of time preparing content schemas in SQLAlchemy
  • These are not required at startup, and unless importing from older content databases, most of these schemas are never used
  • This PR defers initialization of these content schemas until they are actually needed
  • In so doing, on local testing it seems to reduce initialization time from ~2.8 seconds to ~1.6 seconds. A roughly 40% improvement.

Reviewer guidance

Do all channel metadata import and manipulation operations still run as expected?


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added TODO: needs review Waiting for review DEV: backend Python, databases, networking, filesystem... labels Sep 22, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Sep 22, 2021
Copy link
Member

@jamalex jamalex left a comment

Choose a reason for hiding this comment

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

Awesome -- I love that it's not only lazy, but doesn't even ever load the schema for versions not being imported. It's super lazy!

I'm assuming (recalling?) that the import of sample databases with various schema versions are all covered in the test suite.

@rtibbles
Copy link
Member Author

I'm assuming (recalling?) that the import of sample databases with various schema versions are all covered in the test suite.

Yes, they are. Also, I think this behaviour of pre-preparing all the bases was initially implemented because of use of SQLAlchemy DB reflection at run time. But we have not done this in a very long time.

@rtibbles rtibbles merged commit a93fe7e into learningequality:develop Sep 22, 2021
@rtibbles rtibbles deleted the defer_metadata branch September 22, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants