ADR proposals for how learning packages relate to courses/runs [FC-0138] - #812
bradenmacdonald wants to merge 11 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
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.
| 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"?
There was a problem hiding this comment.
That's true, and it does sound like we may still want something like this in the future. Let me think about it.
| 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.
There was a problem hiding this comment.
I have a separate PR coming that makes major revisions to ADR 0005.
|
|
||
| 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?
There was a problem hiding this comment.
I don't think we can strictly enforce it in practice, though we could try to write test fixtures that include two runs that don't share a namespace, to try to catch any issues. That would likely live in the platform tests though.
| 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.
There was a problem hiding this comment.
That's an interesting idea. We certainly could do that, and it would make sense to let people start enrolling in the new version while deferring the actual creation of it so that it can be based on the very latest version of the previous course once it actually gets created. I think it would be possible, but it's not something I planned for here.
Why don't I spin that off to a separate ADR then, and we can figure it out in a dedicated PR. It is a bit of a problem if Pathways are already doing the opposite of what libraries are doing. Edit: here's the PR: #818 |
This seems simpler and just as good. Confirming my understanding: We would have a site-wide namespace if not for the possibility of a birthday attack, right? |
+1
Could this be the Usage table? |
| @@ -0,0 +1,50 @@ | |||
| .. _openedx-content-adr-0011: | |||
|
|
|||
| 11. Do not implement copy-on-write | |||
There was a problem hiding this comment.
ADR 11 looks good to me, thank you! ✅
Hadn't considered "Containers cannot be shared anyway" but that seems like a slam-dunk reason on its own.
There was a problem hiding this comment.
Yeah. I know it's a bit unusual to have an ADR about not doing something and keeping the status quo, but my head kept trying to loop back to some variant of that copy on write idea, so I wanted to document clearly why we should go in a different direction. And the container thing was something I hadn't realized before but is definitely a strong reason.
I wouldn't have thought of a birthday attack as the main risk; based on our usage of BLAKE2b truncated to 20 bytes, Claude says that finding any two files with the same digest costs about 2^80 hash evaluations which is "infeasible for realistic attackers today, but below the margin you would choose if you were designing for cross-tenant safety." More trivially exploitable is an existence oracle where org B can determine if org A already uploaded a file with specific contents (specific hash) by looking for how quickly it is "uploaded" and stored, or other details of the write request. In our context, that's not exactly a worrying possibility. Generally I think the main reason to prefer an org-specific namespace instead of site-wide is just the general principle of tenant isolation and also to a lesser extent sharding of the content (even if it's purely virtual in this case). Following that principle in general provides a more robust system that's less vulnerable to security issues, and it's always less of a concern when security is compromised within a tenant's data rather than across tenants, even if the security risks are pretty low in this case. |
bradenmacdonald
changed the title
|
Oh: the other big argument for maintaining per-org separation: combining all assets into a single namespace is not likely to consolidate many identical assets, so it doesn't really achieve much other than simplicity. The only exception I can think of is the case where an instance has course(s) that are white-labelled and offered by many different orgs on the same instance. In that one case, a global namespace would be a big win. (Technically it could be achieved via some sort of "use global namespace" vs. "use org namepsace" vs. "use course-specific namespace" setting, but that seems like a configuration setting that's not worth the complexity it entails. And by the time any particular instance realizes they may want to change that setting, it's likely too late anyways.) So I think we should pick whichever one we feel is the best overall balance. |
I agree with the points that @bradenmacdonald made. In practice, almost all sharing of content happens within an org today, so we don't lose much. And in addition to potential long term security issues, there are also cost and potentially performance issues:
|
This reverts commit 1fbd5d1.
@ormsbee I'm not opposed to this idea by any means, and I'll try to incorporate that as a future option into the ADR. But I don't really grasp how it will be useful. If LearningPackages are a "behind the scenes" thing, how and why would users even do this? Do we have any requests for this from users/product teams? Or is this sort of package a developer-oriented thing that is created and imported to various instances?
Definitely, but I don't think LearningPackage is a user-visible permissions boundary, is it? Permissions are usually set at the LearningContext level instead. The authz system should allow creating arbitrary groups of courses (e.g. all courses that match pattern |
It could, but that would only namespace Components. Everything else in the course that may exist as versioned content (static assets, pages, grading policy, etc.) would also need some way of being associated with a specific run. In #360 there was some discussion about making "Files & Uploads" and other content part of the hierarchy. In that case, a single table could be used both to represent the complete content hierarchy and to provide namespacing of component/container codes. But it seems weird to me to make everything part of the outline, when the learners don't see it that way. I will try to incorporate this option into the ADR, but I'm leaning toward sketching it out in detail as a "rejected alternative" that we can consider in the future. |
|
I have made some updates to the ADR:
|
| We rejected it because it pays most of the costs of the run-scoped alternative for a smaller benefit than ``media_file_namespace`` delivers on its own: | ||
|
|
||
| - Every package-level operation that decision 6 gets for free becomes run-aware by prefix filtering: publishing one run, listing its drafts, reading its change logs, pruning it, exporting it, and deleting it. Deleting a run is a filtered bulk delete rather than a cascade, and ``backup_restore`` cannot export a single run without new filtering support. | ||
| - Codes stop being opaque. Every lookup, URL (see :ref:`openedx-content-adr-0005`), import, export and upstream link must compose and parse the prefix, which is contrary to the existing assumption that these identifiers are opaque. |
| Link table: ``PublishableEntityVersionLink`` | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Move the entity relationship into a many-to-many table ``(entity, version, version_num)`` with unique constraints on ``(entity, version)`` and ``(entity, version_num)``. A copy is made by copying the source's link rows, so each entity keeps its own unique, monotonic numbering, the concurrency constraint survives, and the ``versioning`` helpers keep their contract. An optional ``previous_version`` field can still record lineage. |
There was a problem hiding this comment.
One thing we could consider for re-runs is to have optional fields on PublishableEntity that capture another entity and version that it was derived from. So when a re-run happens, we could create complete copies of everything, but basically say, "Component C2 in CourseRun2 was derived from Component C1 in CourseRun1, and the draft version of C1 was v10 at the time C2 was created." If we decided to support multiple runs in the same LearningPackage, it could be a lightweight way of tracking/differentiating what would otherwise be confusing duplicate data.
Or I guess we could also re-purpose copied_from_block, though I feel like this is a bit different.
In any case, none of this contradicts this ADR. It just came to mind when considering the idea of links.
There was a problem hiding this comment.
Yes, we already have copied_from_block and ComponentLink, so I do think there's plenty of precedent for models that track where the copies came from. That's also what I was trying to get at with this part of the ADR:
There are other ways to track a component’s history. Between DraftChangeLog, PublishLog, openedx-platform’s ComponentLink, and potential future RerunLog models, we can find ways to show a component’s complete provenance history without necessarily implementing copy-on-write nor completely duplicating it.
@bradenmacdonald: I admit that it's still a bit fuzzy in my head, but I think of LearningPackages as the authoring equivalent of a learning context, i.e. the standard owned thing that can be authored, imported, and exported. Maybe that one thing is a library that has a bunch of assorted content in it. Or maybe that shared, authorable thingy has a bunch of course runs. Or maybe all LearningPackages are Libraries, and we just happen to put various combinations of larger things in those Libraries. But I like the idea of the bounds of the LearningPackage being whatever is convenient and useful for the purposes of authoring. So if it makes sense for one team to own a bootcamp pathway and a bunch of mini-courselets that are associated with that bootcamp, then they should be able to operate on it in a single place. |
|
One of the reasons I assume that the LearningPackage is going to be the boundary for this is that our content is so interconnected that doing permissions on only subsets of content within a LearningPackage is going to be a real pain. In my mind, different authoring ownership == different LearningPackage + sync mechanism if they need to collaborate. |
Is this something that we could actually leverage Collections for? Introduce a new, nullable field to |
|
I suppose at that point, the confusing part would be when to use which identifier, particularly if we think there would be fallback rules. Still, I think there's something there we can use without it turning into a giant mess. |
|
Riffing on the Collection thing a bit more—in the longer run, we could introduce Collection types, one of which is a CourseRunCollection. Each Collection type could be associated with its own libraries detail view. One place where there's a mismatch in semantics is that creating child elements for something in a Collection doesn't automatically add those things to that Collection. I'm not sure if that would be a feature or a bug in this case... |
If we're going to be designing around that idea, then I'd really like to see a product definition, and at least some sort of minimal UX plan and input from the authz team. Otherwise, I think the idea is too vague to be useful for architecture planning purposes. I'm also wary of the many times I've personally built out a backend feature that never became useable by actual users (e.g. pinned versions of containers). |
I take the point on containers pinning their target versions. But when it comes to LearningPackage permissions, I think we're already kind of locked in by existing UX and Product decisions. We've explicitly made content very interconnected in terms of edit and publish behavior. The last time we tried to examine this issue with the perspective of split ownership, I don't think we came up with a good way to do that split and keep that connected vision. One of the possible paths was to do a massive amount of implicit forking, where different versions of things were draft/publish at different versions depending on where they're referenced, but that has some really concerning knock-on effects that I think make it impractical. |
6 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:
AI disclosure: Claude helped me with research, writing, and formatting the RST. I wrote and edited much of the content myself.