-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: [FC-0092] Optimize Course Info Blocks API #37122
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
Changes from 16 commits
3a0ab4b
24e7bfe
92d50b9
86c42f3
1845775
2fc81d4
5bc490e
7ef8fd1
b40d755
76b9cb3
a25a123
9e0d736
e2ad6ec
e6d8fc1
3ec750c
2a0b398
f7bc539
74bb27f
37e7a9e
9565fc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,6 +237,7 @@ def list(self, request, usage_key_string, hide_access_denials=False): # pylint: | |
| params.cleaned_data['return_type'], | ||
| params.cleaned_data.get('block_types_filter', None), | ||
| hide_access_denials=hide_access_denials, | ||
| for_blocks_view=True | ||
| ) | ||
| ) | ||
| # If the username is an empty string, and not None, then we are requesting | ||
|
|
@@ -339,9 +340,52 @@ def list(self, request, hide_access_denials=False): # pylint: disable=arguments | |
| if not root: | ||
| raise ValidationError(f"Unable to find course block in '{course_key_string}'") | ||
|
|
||
| # Earlier we included blocks with future start dates in the collected/cached block structure. | ||
| # Now we need to emulate allow_start_dates_in_future=False by removing any such blocks. | ||
| include_start = "start" in request.query_params['requested_fields'] | ||
| self.remove_future_blocks(course_blocks, include_start) | ||
|
|
||
| recurse_mark_complete(root, course_blocks) | ||
| return response | ||
|
|
||
| @staticmethod | ||
| def remove_future_blocks(course_blocks, include_start: bool): | ||
| """ | ||
| Mutates course_blocks in place: | ||
| - removes blocks whose 'start' is in the future | ||
| - also removes references to them from parents' 'children' lists | ||
| - removes 'start' key from all blocks if it wasn't requested | ||
| """ | ||
| from datetime import datetime, timezone | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need for this to be a function-local import, is there?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't indeed, moved the import to module level |
||
|
|
||
| if not course_blocks: | ||
| return course_blocks | ||
|
|
||
| now = datetime.now(timezone.utc) | ||
|
|
||
| # 1. Collect IDs of blocks to remove | ||
| to_remove = set() | ||
| for block_id, block in course_blocks.items(): | ||
| get_field = block.get if include_start else block.pop | ||
| start = get_field("start") | ||
| if start and start > now: | ||
| to_remove.add(block_id) | ||
|
Comment on lines
+370
to
+371
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like this function is ignoring
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, we are dealing with the start dates that have been computed by |
||
|
|
||
| if not to_remove: | ||
| return course_blocks | ||
|
|
||
| # 2. Remove the blocks themselves | ||
| for block_id in to_remove: | ||
| course_blocks.pop(block_id, None) | ||
|
|
||
| # 3. Clean up children lists | ||
| for block in course_blocks.values(): | ||
| children = block.get("children") | ||
| if children: | ||
| block["children"] = [cid for cid in children if cid not in to_remove] | ||
|
Comment on lines
+381
to
+384
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to check my understanding: Is it the case that it's okay to do this simple child removal (and not go down into further descendants) because all the inheritance has already been pre-computed, and the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is correct: by this point, |
||
|
|
||
| return course_blocks | ||
|
|
||
|
|
||
| @method_decorator(transaction.non_atomic_requests, name='dispatch') | ||
| @view_auth_classes(is_authenticated=False) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| """ | ||
| Code used to get and cache the requested course-data | ||
| """ | ||
|
|
||
| from edx_django_utils.cache import RequestCache | ||
|
|
||
| from lms.djangoapps.course_blocks.api import get_course_blocks | ||
| from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager | ||
|
|
@@ -56,7 +56,15 @@ def location(self): # lint-amnesty, pylint: disable=missing-function-docstring | |
| @property | ||
| def structure(self): # lint-amnesty, pylint: disable=missing-function-docstring | ||
| if self._structure is None: | ||
| self._structure = get_course_blocks( | ||
| # The get_course_blocks function proved to be a major time sink during a request at "blocks/". | ||
| # This caching logic helps improve the response time by getting a copy of the already transformed, but still | ||
| # unfiltered, course blocks from RequestCache and thus reducing the number of times that | ||
| # the get_course_blocks function is called. | ||
|
|
||
| request_cache = RequestCache("unfiltered_course_structure") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the names are used multiple time, let's create a variable at the appropriate scope for these.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, constants added to |
||
| cached_response = request_cache.get_cached_response("reusable_transformed_blocks") | ||
| reusable_transformed_blocks = cached_response.value if cached_response.is_found else None | ||
| self._structure = reusable_transformed_blocks or get_course_blocks( | ||
| self.user, | ||
| self.location, | ||
| collected_block_structure=self._collected_block_structure, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this argument doesn't really express what it does -- to me at least. I also question the API design:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the naming, I guess we could come up something better:
cache_with_future_datesseems to reflect the intent more clearly.As for the other questions:
only call
get_blocks()once and never need to reuse the collected course structure; ordo call
get_blocks()more than once, but they either pass the same arguments (benefiting from@request_cached) or they pass a differentuser(the cached structure has to be recollected from scratch).Yes, this abstraction seems reasonable, and I think it can be nicely paired with the other refactoring you suggest - creating variables for cache key names.
The logic here was indeed influenced by the existing codebase in a lot of ways, that's for sure. And while some of the previous design choices might seem questionable, this particular decision seems to make sense: we want to get_blocks and we specify a filtering criteria - whether to include future start dates or not. Filtering after the fact in each caller would basically mean trying to reproduce what the existing transformers are already designed to do. This doesn't seem like a very clean approach, and the only reason we resort to it here is because of the constraints (in terms of scope and performance) of this particular api view.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only for you - I've also had this question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache_with_future_datesutils.py