-
Notifications
You must be signed in to change notification settings - Fork 4.2k
refactor: Simplify ModuleStore Runtimes now that XModules are gone #35523
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
refactor: Simplify ModuleStore Runtimes now that XModules are gone #35523
Conversation
a70620e to
2f85b66
Compare
2f85b66 to
b4bc421
Compare
1ebb8cb to
94a2d4c
Compare
|
@bradenmacdonald @ormsbee @feanil This is an old refactoring I had on the backburner for a while. Not high-importance, but if any of you have a chance to review next week, it'd be nice to get this merged in before 2U forks. |
|
My understanding is that 2U has already forked, and they did so pre-5.2-upgrade. Though it could still be worth doing if they want to cherry-pick it into their fork. @jristau1984: Can you please clarify the status of 2U's fork? |
|
Oh wait, they're doing it directly on https://github.com/edx/edx-platform, so we can see it. |
|
@Agrendalath: Is this something that you might have time to review? |
|
JR told me that they'll be manually syncing in master up until the point of the actual Ulmo cutoff, at which point they'll be on the release cadence. |
|
@ormsbee, sure, I can review it later this week. |
94a2d4c to
575fe6b
Compare
|
@Agrendalath do you think you'll be able to review this week before the cutoff? |
|
@kdmccormick, ah, sorry, I missed this one. I'll review it tomorrow. Would you be able to take a look at the test failures? |
575fe6b to
815741b
Compare
* Consolidates and renames the runtime used as a base for all the others:
* Before: `xmodule.x_module:DescriptorSystem` and
`xmodule.mako_block:MakoDescriptorSystem`.
* After: `xmodule.x_module:ModuleStoreRuntime`.
* Co-locates and renames the runtimes for importing course OLX:
* Before: `xmodule.x_module:XMLParsingSystem` and
`xmodule.modulestore.xml:ImportSystem`.
* After: `xmodule.modulestore.xml:XMLParsingModuleStoreRuntime` and
`xmodule.modulestore.xml:XMLImportingModuleStoreRuntime`.
* Note: I would have liked to consolidate these, but it would have
involved nontrivial test refactoring.
* Renames the stub Old Mongo runtime:
* Before: `xmodule.modulestore.mongo.base:CachingDescriptorSystem`.
* After: `xmodule.modulestore.mongo.base:OldModuleStoreRuntime`.
* Renames the Split Mongo runtime, the which is what runs courses in LMS and CMS:
* Before: `xmodule.modulestore.split_mongo.caching_descriptor_system:CachingDescriptorSystem`.
* After: `xmodule.modulestore.split_mongo.runtime:SplitModuleStoreRuntime`.
* Renames some of the dummy runtimes used only in unit tests.
815741b to
fcbf6d3
Compare
|
Thank @Agrendalath . Build is green now. |
|
Sandbox deployment failed 💥 |
Agrendalath
left a comment
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.
This is a great cleanup, @kdmccormick!
Warning
I did not test the following parts because they do not work for me locally, even on the master branch (so they are not related to this PR):
- The Studio editor, because it fails with
xblockElement is empty or not definedconsole error.tutor dev exec lms npm run build-devfails with[webpack-cli] TypeError: Merge.merge is not a function. - Exporting a course, because it fails with
[Errno 2] No such file or directory: '/openedx/media/video-transcripts/0cba5d92734e40969d855cac9534fa3a.srt'.
👍
- I tested this: rendering blocks in the LMS and importing a course
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
|
Thanks @Agrendalath ! Thanks for the heads up--I just re-verified that those parts are working fine on the PR sandbox. |
|
@kdmccormick: You had written:
Can you clarify what “handling this” means? Is there some action we need to take? I’m not quite clear on the benefit to 2U of having merged sooner rather than later. Since this already merged, I’m not debating the decision, but simply wanted to understand what you are referring to. Thank you. |
|
Sure. Upstream refactorings make rebases more difficult, especially as the rebases get larger. Rebasing a week of private commits from your fork onto this commit will likely be much easier than rebasing 6 months of private commits onto it. |
|
Thanks for clarifying @kdmccormick. I'm hoping 2U keeps to very few private commits that aren't on master, and we plan to start fresh with each named release, but we'll see what happens. In either case, BTR is about to test the release, so from that standpoint getting changes merged sooner rather than later means it won't be sitting and possibly broken for a long while. |
…nedx#35523) * Consolidates and renames the runtime used as a base for all the others: * Before: `xmodule.x_module:DescriptorSystem` and `xmodule.mako_block:MakoDescriptorSystem`. * After: `xmodule.x_module:ModuleStoreRuntime`. * Co-locates and renames the runtimes for importing course OLX: * Before: `xmodule.x_module:XMLParsingSystem` and `xmodule.modulestore.xml:ImportSystem`. * After: `xmodule.modulestore.xml:XMLParsingModuleStoreRuntime` and `xmodule.modulestore.xml:XMLImportingModuleStoreRuntime`. * Note: I would have liked to consolidate these, but it would have involved nontrivial test refactoring. * Renames the stub Old Mongo runtime: * Before: `xmodule.modulestore.mongo.base:CachingDescriptorSystem`. * After: `xmodule.modulestore.mongo.base:OldModuleStoreRuntime`. * Renames the Split Mongo runtime, the which is what runs courses in LMS and CMS: * Before: `xmodule.modulestore.split_mongo.caching_descriptor_system:CachingDescriptorSystem`. * After: `xmodule.modulestore.split_mongo.runtime:SplitModuleStoreRuntime`. * Renames some of the dummy runtimes used only in unit tests.
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
Description
Other info
This should have no user or operator impact.
Testing instructions
Smoke test general LMS and CMS usage on linked sandbox. Confirm that the demo course can be round-tripped through OLX (import, export, re-import), as that uses a different runtime class than typical LMS/CMS rendering.
Deadline
Before the Ulmo cutoff would be nice, as it would save 2U the trouble of having to handle this in the Ulmo->Verawood. But, it's not critical.