fix: fix _update_subsection_grades when restricted blocks are not cached #33963

asadali145 · 2023-12-20T14:06:58Z

Description

This PR fixes an issue with the grading when course blocks are cached without some of the restricted course blocks. Steps to reproduce the issue are mentioned in the testing steps.

Supporting information

Here is the detailed comment about the issue https://discuss.openedx.org/t/debugging-persistent-grade-issues/11155/8?u=asad_ali
Here are some of the GitHub issues:
https://github.com/mitodl/hq/issues/2460
https://github.com/mitodl/hq/issues/1978
https://github.com/mitodl/hq/issues/2687

Testing instructions

Setup master branch
Create a course with multiple subsections and add a few problems in all subsections.
Create 2 modes Audit and Verified for your course.
All problems of a subsection should be restricted to the Verified enrollments.
Create a new learner and enroll in Audit Mode.
Access the course with the Audit Learner.
Visit the subsections and verify that the restricted problems are not accessible.
Attempt the accessible problems, and verify the grades on the Progress page.
Create a new learner and enroll in verified mode.
Access the restricted problems.
Attempt problems. Submissions are successful but grades are not updated on the Progress page.
Now check out this branch, restart your project and repeat the above steps starting at 5.
Grades should be updated on the progress page for the restricted problems.

openedx-webhooks · 2023-12-20T14:07:03Z

Thanks for the pull request, @asadali145! 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.

arslanashraf7

I left a couple of comments.

arslanashraf7 · 2023-12-27T12:09:01Z

lms/djangoapps/grades/tasks.py

+        if not subsections_to_update:
+            update_course_in_cache(course_usage_key.course_key)
+
+            course_structure = get_course_blocks(student, store.make_course_usage_key(course_key))


We can use the course_usage_key we created above.

arslanashraf7 · 2023-12-27T12:11:35Z

lms/djangoapps/grades/tasks.py

+        # When access is restricted and course blocks are cached without the restricted blocks,
+        # we will update the cache from the store and get the course blocks.
+        if not subsections_to_update:
+            update_course_in_cache(course_usage_key.course_key)


I think this could be a long-running process and we should see the impact in ways like:

Can we use a cache updating method that just updates the cache for scored_block_usage_key and not the whole course?

How long with it generally take on larger courses?

I see a newer version of this method update_course_in_cache_v2 - would it be better to shift towards that?

Can we use a cache updating method that just updates the cache for scored_block_usage_key and not the whole course?

I tried to look more into it and initially thought there might be a way to do it for the scored block. It looked like BlockStructureManager might support but it didn't.
There is another way to get the block from the store directly but that does not cover all the use cases where recalculate_subsection_grade_v3 is used. Maybe someone having more in-depth knowledge could suggest a better approach if any.

How long with it generally take on larger courses?

I have tested it for 3 courses locally and here is the breakdown:
Demonstration Course: 5 Sections, 11 Subsections, 45 Units: 1.5 Secs
Course X: 40 Sections, 96 Subections, 185 Units: 10 Secs
Course Y: 13 Sections, 33 Subsections, 289 Units: 13 Secs

Also, we should note that this update won't be a frequent one. This approach is used at multiple places where we fetch the blocks of a course.

I see a newer version of this method update_course_in_cache_v2 - would it be better to shift towards that?

We are already doing the update inside a celery task and delaying it to another task will not serve the purpose.

@asadali145 I think problem with using update_course_in_cache is it would not update blockstructure cache if STORAGE_BACKING_FOR_CACHE switch is enabled and course published version is same as BlockStructureModel version. See implementation of is_up_to_date for details how it makes update decision.

I think we can clear the blockstructure cache using clear_course_from_cache and then raise exception SUBSECTION_NOT_FOUND(need add it in one of the known retry exceptions). This would retry the recalculate_subsection_grade_v3 task.

arslanashraf7 · 2023-12-27T12:12:43Z

lms/djangoapps/grades/tasks.py

+        # When access is restricted and course blocks are cached without the restricted blocks,
+        # we will update the cache from the store and get the course blocks.


Maybe a more readable version of this comment could be:

Suggested change

# When access is restricted and course blocks are cached without the restricted blocks,

# we will update the cache from the store and get the course blocks.

# Update the cache from the store and get the course blocks if access is restricted and course blocks are

# cached without those restricted blocks.

pdpinch · 2024-02-21T02:49:00Z

@mphilbrick211 @timmc-edx This is a critical PR for us that fixes a recurring problem where learner's grades aren't persisted, causing the progress page and the gradebook to be incorrect. There's more detail in this discourse post linked in the PR description.

Since this changes the caching behavior of grades, I think it would be best to have a 2U reviewer -- or at least someone who is familiar with running large instances.

pdpinch · 2024-02-21T02:55:13Z

Oh, I just realized there's a parallel conversation about recruiting a reviewer over on openedx/wg-coordination#106

ziafazal

@asadali145 Although updating the block structure cache might be an acceptable way to get rid of the issue, But is there anyway we can prevent course structure to cache only those blocks where user has access? It appears it as complete course blocks cache instead of a user access based cache.

ziafazal · 2024-02-29T15:31:24Z

lms/djangoapps/grades/tasks.py

+        # When access is restricted and course blocks are cached without the restricted blocks,
+        # we will update the cache from the store and get the course blocks.
+        if not subsections_to_update:
+            update_course_in_cache(course_usage_key.course_key)


@asadali145 I think problem with using update_course_in_cache is it would not update blockstructure cache if STORAGE_BACKING_FOR_CACHE switch is enabled and course published version is same as BlockStructureModel version. See implementation of is_up_to_date for details how it makes update decision.

I think we can clear the blockstructure cache using clear_course_from_cache and then raise exception SUBSECTION_NOT_FOUND(need add it in one of the known retry exceptions). This would retry the recalculate_subsection_grade_v3 task.

asadali145 · 2024-03-01T06:48:11Z

Although updating the block structure cache might be an acceptable way to get rid of the issue, But is there anyway we can prevent course structure to cache only those blocks where user has access?

I will look for it. It would be nice if we could do this.

I think we can clear the blockstructure cache using clear_course_from_cache and then raise exception SUBSECTION_NOT_FOUND(need add it in one of the known retry exceptions). This would retry the recalculate_subsection_grade_v3 task.

Sure, This makes much more sense. I will do this if I am unable to find an answer to your first question.

asadali145 · 2024-03-07T16:44:48Z

But is there anyway we can prevent course structure to cache only those blocks where user has access?

I was looking into this and it turns out to be using the runtime. Method create_from_modulestore is responsible for the issue when it gets the children of an xBlock. When users do not have access to the child blocks of a Unit, there will be no children and the block structure cache will have missing blocks.

get_block_for_descriptor is responsible for checking the access.

We might be able to fix the issue by manipulating a user attribute and passing the user deep to the block structure manager methods but that can be very tricky. Changes at such a deep level will require us to test so many use cases and will require us to write/update many unit tests.

I am going with your suggested change to clear the cache as it is very targeted and easy to manage.

asadali145 · 2024-03-08T06:34:40Z

@ziafazal This is ready for another look.

ziafazal · 2024-03-08T11:49:09Z

lms/djangoapps/grades/tasks.py

+        # cached without the restricted blocks.
+        if not subsections_to_update:
+            clear_course_from_cache(course_usage_key.course_key)
+            raise BlockStructureNotFound(scored_block_usage_key)


I think you should create a new exception named SubsectionNotFound here instead of raising BlockStructureNotFound because it appears block structure is available but few blocks are missing.

Can I use UsageKeyNotInBlockStructure?

Yes that can be used too.

ziafazal

LGTM 👍

pdpinch · 2024-03-11T12:56:50Z

@asadali145, can you squash your commits? The core contributor could do it when they merge, but there are so many, so I think it would be better if you did it so you can focus on the important ones and keep the commit history clean.

After that, I think this is ready to merge. @mphilbrick211, is there anything else we need to do?

mphilbrick211 · 2024-03-11T21:52:23Z

1 approval

@pdpinch nope, looks like you're good-to-go! Do we know who to ask for merge?

pdpinch · 2024-03-12T20:29:49Z

Thank you @mphilbrick211. I can take care of the merge.

openedx-webhooks · 2024-03-12T20:30:47Z

@asadali145 🎉 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-03-12T22:09:38Z

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

edx-pipeline-bot · 2024-03-12T22:37:21Z

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

edx-pipeline-bot · 2024-03-12T22:37:21Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 20, 2023

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch from c77864f to 8455f7b Compare December 26, 2023 07:58

asadali145 marked this pull request as ready for review December 26, 2023 07:59

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch from 775a1ab to 9e5fd0e Compare December 26, 2023 16:00

arslanashraf7 reviewed Dec 27, 2023

View reviewed changes

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch 3 times, most recently from 8bfe54f to b020f10 Compare January 4, 2024 07:47

antoviaque mentioned this pull request Jan 9, 2024

Debugging persistent grade issues openedx/wg-coordination#106

Closed

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024

ziafazal self-requested a review February 23, 2024 05:17

itsjeyd assigned ziafazal Feb 23, 2024

ziafazal reviewed Feb 29, 2024

View reviewed changes

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch from b020f10 to ae65d4a Compare March 7, 2024 17:31

asadali145 requested a review from ziafazal March 8, 2024 06:34

ziafazal reviewed Mar 8, 2024

View reviewed changes

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch 4 times, most recently from f816b76 to f828be0 Compare March 8, 2024 16:27

asadali145 requested a review from ziafazal March 8, 2024 16:50

mphilbrick211 removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Mar 8, 2024

ziafazal approved these changes Mar 11, 2024

View reviewed changes

fix: clear block structure cache when usage key is not found

82685c1

asadali145 force-pushed the asad/fix-grading-when-restricted-blocks-are-not-cached branch from f828be0 to 82685c1 Compare March 12, 2024 00:55

pdpinch merged commit 19c04fc into openedx:master Mar 12, 2024
65 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix _update_subsection_grades when restricted blocks are not cached #33963

fix: fix _update_subsection_grades when restricted blocks are not cached #33963

asadali145 commented Dec 20, 2023 •

edited

openedx-webhooks commented Dec 20, 2023 •

edited

arslanashraf7 left a comment

arslanashraf7 Dec 27, 2023

arslanashraf7 Dec 27, 2023

asadali145 Dec 28, 2023

ziafazal Feb 29, 2024

arslanashraf7 Dec 27, 2023

pdpinch commented Feb 21, 2024

pdpinch commented Feb 21, 2024

ziafazal left a comment

ziafazal Feb 29, 2024

asadali145 commented Mar 1, 2024

asadali145 commented Mar 7, 2024

asadali145 commented Mar 8, 2024

ziafazal Mar 8, 2024

asadali145 Mar 8, 2024

ziafazal Mar 8, 2024

ziafazal left a comment

pdpinch commented Mar 11, 2024

mphilbrick211 commented Mar 11, 2024

pdpinch commented Mar 12, 2024

openedx-webhooks commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

		# When access is restricted and course blocks are cached without the restricted blocks,
		# we will update the cache from the store and get the course blocks.

fix: fix _update_subsection_grades when restricted blocks are not cached #33963

fix: fix _update_subsection_grades when restricted blocks are not cached #33963

Conversation

asadali145 commented Dec 20, 2023 • edited

Description

Supporting information

Testing instructions

openedx-webhooks commented Dec 20, 2023 • edited

arslanashraf7 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

pdpinch commented Feb 21, 2024

pdpinch commented Feb 21, 2024

ziafazal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asadali145 commented Mar 1, 2024

asadali145 commented Mar 7, 2024

asadali145 commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziafazal left a comment

Choose a reason for hiding this comment

pdpinch commented Mar 11, 2024

mphilbrick211 commented Mar 11, 2024

pdpinch commented Mar 12, 2024

openedx-webhooks commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

edx-pipeline-bot commented Mar 12, 2024

asadali145 commented Dec 20, 2023 •

edited

openedx-webhooks commented Dec 20, 2023 •

edited