fix: Fix course-overviews sink not ordering blocks correctly #59
Conversation
We've been using Modulestore's get_items, but even though it's often correct the ordering isn't guaranteed. The new method walks the course tree itself, then adds in the detached models separately.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
| # get_items call does not guarantee ordering. It does not return | ||
| # detached blocks, so we gather them separately below. | ||
| course_block = modulestore.get_course( | ||
| course_key, revision="rev-opt-published-only" |
There was a problem hiding this comment.
where is that revision key coming from?
There was a problem hiding this comment.
It comes from here, where it has been unchanged for 10 yrs: https://github.com/openedx/edx-platform/blob/749e18bcee396755f411f46d79b25f42628af7d1/xmodule/modulestore/__init__.py#L68
I'll make it a local constant and document it better if Dave says this is an ok direction, but I'd rather not import it from modulestore since that means a shim function for the import, mocking it in tests, etc, which doesn't seem worth it since it would only guard against the very unlikely change in the string value.
| course_block = modulestore.get_course( | ||
| course_key, revision="rev-opt-published-only" | ||
| ) | ||
| items = self.get_xblocks_recursive(course_block) |
There was a problem hiding this comment.
Pulling all of a course's blocks into memory like this might not be a good idea..
Holding just the parts of the blocks that we need (usage key, display_name, ...) would be more memory efficient than holding the whole block(s).
There was a problem hiding this comment.
I don't think this is related to this PR, but I'm seeing Course Overview and XBlock sink events in the logs when I just add a section or subsection to a course, but I haven't hit Publish yet. Are these structure blocks are automatically "published"?
I mention it because it seems to block the "new section/subsection" request response in Studio, which was already noticeably slow.
There was a problem hiding this comment.
Sure, I can refactor it to do some kind of generator thing to avoid holding everything in memory.
If this is getting triggered that should mean that COURSE_PUBLISHED for fired, but I'd guess the slowness you're seeing is in dev where all of the heavyweight Celery tasks are happening in-process. I wouldn't expect that slowness to be a thing that happens in a production setting. It's sub-second on the demo course for me.
There was a problem hiding this comment.
@pomegranited I took a stab at the memory consumption in the latest commit, but I'm not sure how much it helps. Depends on modulestore internals that I don't fully understand yet.
There was a problem hiding this comment.
Excellent -- serializing the blocks down to minimal properties should help, thank you!
but I'd guess the slowness you're seeing is in dev where all of the heavyweight Celery tasks are happening in-process
Oh absolutely, my poor little devstack isn't representative, just a good canary.
This attempts to keep memory usage down by keeping copies of the blocks around for as little time as possible.
There was a problem hiding this comment.
👍 Working great for me, thanks @bmtcril
- I tested this on my Aspects tutor dev stack, watched data sync to clickhouse whenever course structure is changed or a course is published.
- I read through the code, and checked that the tests validate this change.
-
I checked for accessibility issuesN/A -
Includes documentationN/A -
User-facing strings are extracted for translationN/A
We've been using Modulestore's get_items, but even though it's often correct the ordering isn't guaranteed. The new method walks the course tree itself, then adds in the detached models separately.
Potential concerns:
This was already a pretty heavy operation, calling into Modulestore twice to walk all of the blocks may not be performant enough or cause issues in the LMS/CMS. It may be preferable to ignore detached blocks if we are not currently using them, but I wanted to preserve parity with what we've been doing unless/until there is an actual problem.
Closes: #61
Merge checklist:
Check off if complete or not applicable: