fix: prefetch user state only for selected library_content children - #409
Open
mraman-2U wants to merge 1 commit into
Open
fix: prefetch user state only for selected library_content children#409mraman-2U wants to merge 1 commit into
mraman-2U wants to merge 1 commit into
Conversation
When building FieldDataCache for render, walk get_child_blocks() for dynamic blocks (library_content, itembank) instead of all modulestore children. This aligns prefetch with vertical render behavior and reduces StudentModule over-fetch on large randomized quizzes. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates LMS FieldDataCache descendant traversal so that blocks with dynamic, learner-specific children (e.g., library_content/item bank/randomize-like blocks) prefetch user state only for the children that will be rendered for the current learner, reducing unnecessary user-state prefetch and improving correctness for dynamic content selection.
Changes:
- Added
_children_for_field_data_cache()helper to choose child blocks viaget_child_blocks()for dynamic blocks, falling back toget_children()+get_required_block_descriptors()for static blocks. - Updated
FieldDataCache.add_block_descendentstraversal to use the new helper (and renamed the internal traversal function tocollect_descendant_blocks). - Added tests validating that dynamic blocks only prefetch selected children and do not prefetch unselected children.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lms/djangoapps/courseware/model_data.py | Switches descendant traversal to prefer get_child_blocks() for dynamic blocks via a new helper. |
| lms/djangoapps/courseware/tests/test_model_data.py | Adds coverage to ensure only learner-selected dynamic children are prefetched. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+671
to
+675
| get_child_blocks = getattr(block, 'get_child_blocks', None) | ||
| has_dynamic_children = getattr(block, 'has_dynamic_children', None) | ||
| if callable(has_dynamic_children) and has_dynamic_children() and callable(get_child_blocks): | ||
| return list(get_child_blocks()) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request improves how the
FieldDataCacheprefetches child blocks, especially for dynamic blocks such aslibrary_content, ensuring that only the relevant learner-specific children are prefetched. This change prevents unnecessary loading of user state for blocks that will not be rendered for the current learner, optimizing performance and correctness. Additionally, comprehensive tests are added to verify the new behavior.Enhancements to child block prefetching:
_children_for_field_data_cachehelper function inmodel_data.pyto determine which child blocks should be prefetched, usingget_child_blocks()for dynamic blocks and falling back toget_children()andget_required_block_descriptors()for static blocks.add_block_descendentsto use_children_for_field_data_cache, ensuring only the appropriate children are traversed and prefetched.get_child_blockstocollect_descendant_blocksfor clarity and to support the new child selection logic.Testing improvements:
TestFieldDataCacheDynamicChildrenintest_model_data.pyto verify that dynamic blocks only prefetch learner-selected children and static blocks continue to prefetch all children. Also tested that unselected children are not prefetched.test_model_data.pyto include the new helper function and related classes.When building FieldDataCache for render, walk get_child_blocks() for dynamic blocks (library_content, itembank) instead of all modulestore children. This aligns prefetch with vertical render behavior and reduces StudentModule over-fetch on large randomized quizzes.Description
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.