ADR proposals for how learning packages relate to courses/runs - #812
ADR proposals for how learning packages relate to courses/runs#812bradenmacdonald wants to merge 4 commits into
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
open-source-contribution
| openedx_learning | ||
|
|
||
| # Catalog: defines how content is organized into enrollable courses | ||
| openedx_catalog |
There was a problem hiding this comment.
While I was at it, I've clarified that the catalog models can be aware of the content models, but not vice versa.
There was a problem hiding this comment.
I'll leave a more detailed review in the next couple of days, but some high level thoughts/reactions:
Media de-duplication
- I think the
blob_namespaceidea is a clever workaround for our biggest storage issues, and I really like it. - I do wonder if we should just force the namespace to a shared org level for new entries though, since there seems to be very little downside to it.
- I'm trying to think through whether it's really okay for it to be immutable, what the use cases might be for wanting to change it, or what cleanup looks like in the shared scenario.
- I might quibble with the naming a bit. Something like
media_file_namespacemight make the intent clearer.
That being said, I'm broadly supportive of this, and I think whatever wrinkles there are can be worked out.
Dependency relationship between openedx_content and openedx_catalog
The Pathways ADR assumes the opposite relationship between openedx_content and openedx_catalog. (In retrospect, we should have spun off a separate ADR for this decision in one of those applets.) I was thinking that any context -> content mappings could happen from the openedx_content side.
1:1 relationship between CourseRun and LearningPackage
This is the one that I'm most concerned about. We are aiming for a future where we can have wildly flexible arrangements of content. I don't want to assume that the authoring and learning contexts are 1:1. I would like to be in a place where someday where we can export a self-contained LearningPackage that contains a Pathway, its half dozen courses, and a bunch of smaller things. Maybe this means that we'll need to make a new model to capture namespaced identifiers to resolve potential conflicts between different course runs, but I think this sort of thing is an acceptable trade-off.
Note that I'm not saying that we should force all runs of a course into the same LearningPackage, or try to force re-runs to share the same PublishableEntities. That presents permissions headaches for interconnected content. But I do think that we should leave the door open for scenarios where there is a team working on content in a more centralized way, and it makes sense for that team to have such broad permissions across multiple courses or smaller learning contexts (e.g. bootcamps). We could even have scenarios where people are doing a library sync for the course run as a whole--if it's mapped to the same blob_namespace, that will be relatively cheap.
| - **Containers cannot be shared anyway.** A :class:`ContainerVersion` points at an :class:`EntityList` whose rows reference specific child entities. A copy needs its own container versions regardless, so only leaf component versions would ever be shared. | ||
| - **The blast radius is large.** Both rejected designs below change public models exposed through ``models_api``, require a DEPR of parts of the ``versioning`` helper, touch every applet, and need coordinated changes in ``openedx-platform`` (the XBlock runtime's version lookups, library history, restore, upstream sync and the clipboard). | ||
|
|
||
| A future ADR will address reducing storage space by consolidating media across the learning packages that hold different versions of the same course, which captures most of the storage benefit without changing the publishing model. |
There was a problem hiding this comment.
This is addressed in ADR 12. Would it be good to reference it directly here?
| One LearningPackage per CatalogCourse, with run-scoped entities | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| The main alternative. All runs of a catalog course share one learning package; a new ``scope`` column on :class:`PublishableEntity` records which run each entity belongs to, with a null scope meaning "shared across runs". A rerun creates only the container spine, pointing at the previous run's components pinned to their published versions, and forks a component into its own scope on first edit. |
There was a problem hiding this comment.
A rerun creates only the container spine, pointing at the previous run's components pinned to their published versions, and forks a component into its own scope on first edit.
I feel this approach can be decoupled from the decision not to use a LearningPackage per CatalogCourse. My question is: can't this approach be used with "One LearningPackage per CourseRun"?
| Creating a rerun copies the source run's entities into a new learning package. To avoid also duplicating the asset *bytes*, :class:`LearningPackage` will gains an immutable ``blob_namespace`` UUID field, and :meth:`Media.path` becomes:: | ||
|
|
||
| content/{learning_package.blob_namespace}/{hash_digest} | ||
|
|
There was a problem hiding this comment.
This doesn't break the public URL contract in ADR-0005, but we should state it explicitly in the text.
|
|
||
| When a learning package is created for a rerun, it copies the ``blob_namespace`` of a sibling run's learning package rather than generating a new one. All runs of a catalog course therefore share one namespace, and identical asset files are stored once across all of them. :meth:`Media.write_file` already returns without writing when a file of matching size exists at the target path, so deduplication happens automatically on write with no change to the media API. A namespace never spans organizations, because the runs that share one are by construction runs of the same catalog course. | ||
|
|
||
| :class:`Media` *rows* remain scoped to a learning package: the ``(learning_package, media_type, hash_digest)`` constraint is unchanged, and each package has its own rows even when they resolve to a shared blob. This preserves per-package accounting, cascading cleanup on delete, and the borrowing-by-copy model. **No code may depend on two learning packages sharing a blob namespace; it is a storage optimization only.** |
There was a problem hiding this comment.
No code may depend on two learning packages sharing a blob namespace; it is a storage optimization only
How will this be achieved in practice?
| 3. CourseRun holds the relationship, and openedx_catalog layers above openedx_content | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| :class:`CourseRun` gains a nullable, unique foreign key to :class:`LearningPackage`. It is nullable because a course run may exist purely as a marketing or enrollment placeholder, or may still have its content in modulestore. It is unique because the relationship is one-to-one. This is also exactly analogous to how the ``ContentLibrary`` model in openedx-platform stores a relationship to :class:`LearningPackage`. |
There was a problem hiding this comment.
Since the LearningPackage can be null, was there any consideration of deferring the course copy when creating a rerun? That is, the LearningPackage would remain null until a specific trigger occurs (opening the course in Studio comes to mind, though that is open for discussion). Only after the trigger is met would the course copy be generated and the LearningPackage set.
4 participants
Before I propose
AssetSet, I want to settle some open questions around Learning Packages, and copy-on-write with these two ADRs.View the rendered ADRs: