feat: [FC-0056] Implement Sidebar Navigation #34457

NiedielnitsevIvan · 2024-04-02T09:09:29Z

Settings

COURSEWARE_MICROFRONTEND_NAVIGATION_SIDEBAR_BLOCKS_DISABLE_CACHING:
  - name: courseware.disable_navigation_sidebar_blocks_caching
    everyone: true

Description

This PR addresses to the need to add an API for Sidebar Navigation that returns the course structure with sections, subsections, and units, according to user rights.

To improve the performance of the API, was added caching of the course structure for the user, which makes it much easier to calculate the block structure for the user at each request. However, there may be cases when this caching can lead to an overflow of the cache storage in high-loaded LMS with large courses, so the corresponding flag "courseware. disable_navigation_sidebar_blocks_caching" was added so that this caching can be disabled.

Testing instructions

Run master devstack.
Start platform make dev.up and make checkout on this branch.
Create a course with different access rights to course blocks (sections/subsections/subsections) for different users.
Make GET requests to the API (/api/course_home/sidebar/{course_id}) from different users and check if the API returns the correct blocks depending on the user's permissions.
Update the course and check if there are any new elements in the API response.

openedx-webhooks · 2024-04-02T09:09:34Z

Thanks for the pull request, @NiedielnitsevIvan! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

ormsbee

I wrote some questions and comments. Overall, I think this is a pretty pragmatic PR that balances correctness with performance without requiring too much new code. My main concerns are related to the cache cleanup performance and what happens when a person's user partition group changes for the purposes of Unit-level access (e.g. enrollment track).

I would like to know what kinds of response times you're seeing locally on large courses, and where the bottlenecks are presently. You don't have to give detailed profiling traces or anything–I'm just trying to understand if you know generally how this behaves for a larger course in terms of response time and database queries made. Traditionally completion code was one of the slowest things because of n+1 queries, but it looks like you're doing a batch fetch for that, which is great.

Thank you!

ormsbee · 2024-04-09T16:24:01Z

lms/djangoapps/course_home_api/urls.py

@@ -44,6 +48,11 @@
        OutlineTabView.as_view(),
        name='outline-tab'
    ),
+    re_path(
+        fr'sidebar/{settings.COURSE_KEY_PATTERN}',


Is there a name we could pick that better reflects what it is from the server's point of view (e.g. course navigation), rather than where it's being placed in the UI?

ormsbee · 2024-04-09T16:30:18Z

lms/djangoapps/course_home_api/outline/views.py

+        completions = BlockCompletion.objects.filter(user=self.request.user, context_key=course_key).values_list(
+            'block_key',
+            'completion',
+        )


I'm fuzzy on how BlockCompletion works, but do we need everything in the course for this user? Can we just grab completion data for Units and up? Or do we need all the low level stuff because we're recalculating completion on the fly?

Unfortunately, BlockCompletion doesn't store data for Units, only for lower-level blocks, so we have to grab all the data for the user in the course and calculate Completions for the level of Units and up on our own.

Bleh. Yeah, that makes sense. Unfortunate though.

ormsbee · 2024-04-09T16:35:23Z

lms/djangoapps/course_home_api/outline/views.py

+        return list(filter(
+            lambda seq_data: seq_data['id'] in available_sequence_ids or seq_data['type'] != 'sequential',
+            course_sequences
+        ))


Nit (optional): In general, please prefer list comprehensions to list + filter, since they're more commonly used and familiar to Python devs.

ormsbee · 2024-04-09T16:44:09Z

lms/djangoapps/course_home_api/outline/serializers.py

+        children = block.get('children', [])
+        child_classes = {child.get('type') for child in children}
+        new_class = 'other'
+        icon_call_priority = ['video', 'problem']


Comment (not required): If "problem" is a proxy for "is this something that you as a student need to submit", we could scan through the classes to see if have the has_score class attribute set to True. That would catch things like ORA, drag and drop, etc. Another oddball case is library_content which is just a container, but is used for problems the vast majority of the time.

lms/djangoapps/course_home_api/outline/views.py

ormsbee · 2024-04-09T18:38:43Z

lms/djangoapps/course_home_api/outline/views.py

+        return list(filter(
+            lambda section_data: section_data['id'] in available_section_ids, course_sections
+        ))


Nit (optional): In general, please prefer list comprehensions to list + filter, since they're more commonly used and familiar to Python devs.

Thank you, I agree with you that the list comprehensions syntax is more familiar and readable, but in this case I deliberately used list + filter because it has better performance, which can be useful for large courses.

However, if you are in favor of the list comprehensions approach, I can change this.

Ah. I didn't realize it was done for performance reasons. How big of a difference does it make on large courses?

I took measurements and the difference is actually 200-300 ms on average, which can be explained by the error. Therefore, it makes sense to return to list comprehensions for the sake of readability.

ormsbee · 2024-04-09T18:45:31Z

lms/djangoapps/course_home_api/outline/views.py

+        """
+        if 'children' in block:
+            block['children'] = [self.mark_complete_recursive(child) for child in block['children']]
+            block['complete'] = all(child['complete'] for child in block['children'] if child['type'] != 'discussion')


Should this be looking for the class attribute completion_mode on the relevant XBlock classes?

No, because in our case, block is not an XBlock object, but a dict with block data, and therefore we cannot get the value of the completion_mode field here.

Sorry, let me ask this another way: Why is the "discussion" block explicitly excluded here?

The "discussion" block is explicitly excluded here because it cannot be marked as completed, in which case units that have a "discussion" block will never be marked as completed either.

I also considered the option of checking the complete status for blocks that have has_score=True, but in this case it would only apply to problems, which is also not entirely correct.

Wouldn't this get the right tags then?

from xblock.core import XBlock from xblock.completable import XBlockCompletionMode completable_tags = { tag for (tag, cls) in XBlock.load_classes() if XBlockCompletionMode.get_mode(cls) == XBlockCompletionMode.COMPLETABLE }

Oh, thank you very much, I didn't know that we could get completable block types in this way.
Added this change.

ormsbee · 2024-04-09T18:47:14Z

lms/djangoapps/course_home_api/outline/views.py

+            for section_data in course_sections:
+                section_data['children'] = self.get_available_sequences(
+                    user_course_outline,
+                    section_data.get('children', [])


These are definitely necessary? I'm surprised the API building course_blocks would return no children attribute, rather than returning an empty list.

I do not fully understand what you mean, can you explain in more detail please?

I would have expected section_data to always have a children key, even if the value is []. But the fact that you're doing section_data.get('children', []) implies that sometimes that key is missing entirely. That surprised me.

ormsbee · 2024-04-09T18:51:04Z

lms/djangoapps/course_home_api/outline/views.py

+                )
+                accessible_sequence_ids = {str(usage_key) for usage_key in user_course_outline.accessible_sequences}
+                for sequence_data in section_data['children']:
+                    sequence_data['accessible'] = sequence_data['id'] in accessible_sequence_ids


Please add a comment explaining the difference between accessible and available in the above code. I've stared at outline code long enough to get it, but it's probably going to confuse developers looking at this for the first time.

Yep, I think it can be fixed by renaming get_available_sections to get_accessible_sections and so on.

ormsbee · 2024-04-09T18:58:03Z

cms/djangoapps/contentstore/signals/handlers.py

@@ -141,6 +142,7 @@ def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=
    # register special exams asynchronously after the data is ready
    course_key_str = str(course_key)
    transaction.on_commit(lambda: update_special_exams_and_publish.delay(course_key_str))
+    drop_course_sidebar_blocks_cache(course_key_str)


I think this could be problematic at scale, and that it's better to rely on the version changing in the key for invalidation rather than trying to iterate through the whole list of cache keys.

Removed, after adding the course version to the cache key.

GlugovGrGlib · 2024-04-09T19:34:20Z

@ormsbee Please check this the performance analysis on tutor dev - https://raccoongang.atlassian.net/wiki/external/MDhhMjdlMDhjODVkNDBjYjkzZjAzMDdhOGZmMGM2MTk

ormsbee · 2024-04-10T13:56:22Z

@GlugovGrGlib: Thank you! Do you have a broad sense of what the bottlenecks are for those really large courses?

ormsbee · 2024-04-11T17:11:05Z

@GlugovGrGlib: Just as a heads up, when I run this locally on my laptop with the large test course and try loading just this REST endpoint in isolation, I get speeds of about 4.7 seconds for staff and around 6 seconds for a student. That's about 3X faster than when I try to hit those endpoints on the sandbox (I was loading the REST endpoint directly there as well, instead of using the MFE, since I didn't want it to get slowed down by other requests). And that difference is with the debug toolbar left on. Is there a possibility that course blocks caching isn't properly configured on the sandbox?

ormsbee · 2024-04-11T17:31:37Z

Or maybe profiling was left on?

ormsbee

A couple small requests (one optional), but I'd still like to know the answers to the questions I left in my last review before approving.

ormsbee · 2024-04-12T15:00:51Z

lms/djangoapps/course_home_api/outline/serializers.py

+        for higher_class in icon_call_priority:
+            if higher_class in child_classes:
+                new_class = higher_class
+        return new_class


Nit: When I first read icon_call_priority, I assumed that the things that came first had higher priority, rather than the things that came last. I think this might read more clearly if you use returns instead of a new_class var, so something like:

@staticmethod def get_vertical_icon_class(block): """ Get the icon class for a vertical block based on its children. """ children = block.get('children', []) child_classes = {child.get('type') for child in children} icon_call_priority = ['problem', 'video'] for item_type in icon_call_priority: if item_type in child_classes: return item_type return 'other' # default

You could also make it more explicit/obvious like this, since there are only a few classes that matter at the moment:

@staticmethod def get_vertical_icon_class(block): """ Get the icon class for a vertical block based on its children. """ children = block.get('children', []) child_classes = {child.get('type') for child in children} if 'problem' in child_classes: return 'problem' if 'video' in child_classes: return 'video' return 'other'

Either way, please describe the intended ordering in the docstring.

ormsbee · 2024-04-12T15:03:33Z

lms/djangoapps/course_home_api/outline/views.py

+        completions = BlockCompletion.objects.filter(user=self.request.user, context_key=course_key).values_list(
+            'block_key',
+            'completion',
+        )


Bleh. Yeah, that makes sense. Unfortunate though.

ormsbee · 2024-04-12T15:06:55Z

lms/djangoapps/course_home_api/outline/views.py

+        if cached:
+            # If the data was cached, we need to mark the blocks as complete or not complete.
+            course_blocks = self.mark_complete_recursive(course_blocks)


Please explain in a comment why we need to mark the blocks as complete only when we get a cache hit, and not when there's a cache miss.

Comment updated.

NiedielnitsevIvan · 2024-04-19T07:56:57Z

@ormsbee Hello!
During testing, we found a bug that after changing the name in the course blocks (sections, subsections, and units), their name does not immediately change in the sidebar. A similar problem with the names of sections and subsections is currently present on the Course Outline page. The problem lies in the caching of the course structure at the BlockStore level.

One of the solutions to make the cache update instantly or much more often is to change the BlockStructureConfiguration, where you can specify the cache lifetime, but this can affect performance globally.

ormsbee · 2024-04-19T14:39:08Z

@NiedielnitsevIvan: Is that because the collect phase of block transformers takes time to run, so the version you get immediately after making a change is still the course blocks of the old course, and then that gets cached by this new code?

NiedielnitsevIvan · 2024-04-19T15:56:18Z

@NiedielnitsevIvan: Is that because the collect phase of block transformers takes time to run, so the version you get immediately after making a change is still the course blocks of the old course, and then that gets cached by this new code?

That's right, because the course version changes, and the blocks returned from get_course_outline_block_tree are still old, so cache invalidating by course version doesn't work in this case.

GlugovGrGlib · 2024-04-19T21:45:03Z

Do you have a broad sense of what the bottlenecks are for those really large courses?

Sorry, I really didn't go into the depth of the specific code that is the slowest to execute for some time already. Last time we needed to troubleshoot these issues for a client, it was around 2020-2021, when there weren't Learning MFE, and in parallel with your inputs to this discourse post, we got similar results at the time.

Additionally to this, recently we have tested fetching and processing the course structure to find the optimal course setup for a client, you might find those results insightful https://raccoongang.atlassian.net/wiki/external/NWJlYzYxYzRlYzRlNGE5YmFkYjkxYmE0ZTdlNTZjOWE.

Is there a possibility that course blocks caching isn't properly configured on the sandbox?

I believe you were right, initially I was confused about waffle flag, as It should be used to turn off caching, but was implemented the other way. Latter Ivan inverted the behavior for waffle flag, and the performance for loading from cache on the sandbox was enhanced.

ormsbee

That's right, because the course version changes, and the blocks returned from get_course_outline_block_tree are still old, so cache invalidating by course version doesn't work in this case.

In that case, can you grab the cache value from the BlockStructureModel (I think you might have to use the UsageKey of the root Course block instead of the CourseKey)?

edx-platform/openedx/core/djangoapps/content/block_structure/models.py

Lines 168 to 184 in 3852358

    
           data_usage_key = UsageKeyWithRunField( 
        
               'Identifier of the data being collected.', 
        
               blank=False, 
        
               max_length=255, 
        
               unique=True, 
        
           ) 
        
           data_version = models.CharField( 
        
               'Version of the data at the time of collection.', 
        
               blank=True, 
        
               null=True, 
        
               max_length=255, 
        
           ) 
        
           data_edit_timestamp = models.DateTimeField( 
        
               'Edit timestamp of the data at the time of collection.', 
        
               blank=True, 
        
               null=True, 
        
           )

And then fall back to the modulestore's root CourseBlock version if that's not available for some reason? Does that resolve the cache invalidation issue?

ormsbee · 2024-04-19T16:25:28Z

lms/djangoapps/course_home_api/outline/views.py

+COMPLETABLE_BLOCK_TYPES = {
+    block_type for (block_type, block_cls) in XBlock.load_classes()
+    if XBlockCompletionMode.get_mode(block_cls) == XBlockCompletionMode.COMPLETABLE
+}


Sorry, I should have specified when I gave this example: Please don't put this at the module level. You can put it in a function and wrap it in a @functools.cache decorator (or a method and wrap in @functools.cached_property. But we've had problems in the past where XBlock machinery was being initialized earlier than expected as side-effect of module-level statements like this, and it was hard to track down. Putting it in a function or method will help to make sure we don't actually invoke it until the view itself is executed.

I didn't know about this problem.
Fixed it.

ormsbee

Please squash your commits and consider looking into the BlockStructureModel thing I mentioned w.r.t. cache invalidation as a followup. Thank you.

openedx-webhooks · 2024-04-26T16:04:45Z

@NiedielnitsevIvan 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-04-26T17:27:26Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-04-26T18:03:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-04-26T18:38:18Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-04-26T19:02:00Z

2U Release Notice: This PR has been deployed to the edX production environment.

GlugovGrGlib · 2024-05-09T05:27:11Z

This PR is the part of the following product feature - openedx/platform-roadmap#329

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 2, 2024

NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/feat/Implement-Sidebar-Navigation branch from 6632124 to bb1ba59 Compare April 2, 2024 09:12

arbrandes requested review from arbrandes and ormsbee April 4, 2024 14:44

ormsbee requested changes Apr 9, 2024

View reviewed changes

NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/feat/Implement-Sidebar-Navigation branch from eab71a1 to 9a6afaa Compare April 10, 2024 16:58

NiedielnitsevIvan requested a review from ormsbee April 10, 2024 17:00

ormsbee requested changes Apr 12, 2024

View reviewed changes

NiedielnitsevIvan requested a review from ormsbee April 17, 2024 09:31

arbrandes mentioned this pull request Apr 25, 2024

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) openedx/frontend-app-learning#1349

Closed

ormsbee requested changes Apr 25, 2024

View reviewed changes

NiedielnitsevIvan requested a review from ormsbee April 26, 2024 06:59

NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/feat/Implement-Sidebar-Navigation branch from ef36dca to 9edc20f Compare April 26, 2024 07:08

ormsbee approved these changes Apr 26, 2024

View reviewed changes

feat: [FC-0056] Implement Sidebar Navigation

8f23703

NiedielnitsevIvan force-pushed the Ivan_Niedielnitsev/feat/Implement-Sidebar-Navigation branch from 9edc20f to 8f23703 Compare April 26, 2024 14:53

ormsbee merged commit 3083672 into openedx:master Apr 26, 2024
67 checks passed

ihor-romaniuk mentioned this pull request Apr 30, 2024

[FC-0056] Course outline sidebar openedx/frontend-app-learning#1375

Merged

GlugovGrGlib mentioned this pull request Jun 14, 2024

Reintroduce left-sidebar navigation openedx/platform-roadmap#329

Closed

	data_usage_key = UsageKeyWithRunField(
	'Identifier of the data being collected.',
	blank=False,
	max_length=255,
	unique=True,
	)
	data_version = models.CharField(
	'Version of the data at the time of collection.',
	blank=True,
	null=True,
	max_length=255,
	)
	data_edit_timestamp = models.DateTimeField(
	'Edit timestamp of the data at the time of collection.',
	blank=True,
	null=True,
	)

feat: [FC-0056] Implement Sidebar Navigation #34457

feat: [FC-0056] Implement Sidebar Navigation #34457

Conversation

NiedielnitsevIvan commented Apr 2, 2024 • edited Loading

Settings

Description

Testing instructions

openedx-webhooks commented Apr 2, 2024

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlugovGrGlib commented Apr 9, 2024

ormsbee commented Apr 10, 2024

ormsbee commented Apr 11, 2024 • edited Loading

ormsbee commented Apr 11, 2024

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiedielnitsevIvan commented Apr 19, 2024

ormsbee commented Apr 19, 2024

NiedielnitsevIvan commented Apr 19, 2024

GlugovGrGlib commented Apr 19, 2024 • edited Loading

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Apr 26, 2024

edx-pipeline-bot commented Apr 26, 2024

edx-pipeline-bot commented Apr 26, 2024

edx-pipeline-bot commented Apr 26, 2024

edx-pipeline-bot commented Apr 26, 2024

GlugovGrGlib commented May 9, 2024

NiedielnitsevIvan commented Apr 2, 2024 •

edited

Loading

ormsbee commented Apr 11, 2024 •

edited

Loading

GlugovGrGlib commented Apr 19, 2024 •

edited

Loading