Another pass at basic content models #13

ormsbee · 2022-05-26T14:58:44Z

This splits Item-related models into a new app and otherwise follows the logical split discussed here.

openedx_learning/apps/core/itemstore/models.py

kdmccormick

Sorry I took so long to review this. Most of my comments are just me trying to make sure I grok everything.

kdmccormick · 2022-06-06T20:20:29Z

projects/dev.py

+    # Learning Core Apps
    'openedx_learning.apps.core.compose.apps.ComposeConfig',
+    'openedx_learning.apps.core.itemstore.apps.ItemStoreConfig',
+    'openedx_learning.apps.core.publish.apps.PublishConfig',


I'm aware these comments are superficial & tangential to the PR, but I ought to express them now before anyone starts using this repo :)

The apps namespace package seems unnecessary and out-of-step with other edx-platform libraries. Why not just openedx_learning.core.itemstore?

I notice that you are using verbs for some of the core app names. Wouldn't it be more standard to use nouns? (composition, publishing)

The apps namespace package seems unnecessary and out-of-step with other edx-platform libraries. Why not just openedx_learning.core.itemstore?

Mostly because I wanted a place to hang lib for things like the field types.

I notice that you are using verbs for some of the core app names. Wouldn't it be more standard to use nouns? (composition, publishing)

Yeah, I was going for the verb form for a while before because it was shorter, before I landed on itemstore for one of them. I'll switch it back over to nouns.

Mostly because I wanted a place to hang lib for things like the field types.

The 2nd-level packages could be openedx_learning.core, openedx_learning.lib, and openedx_learning.contrib.

I remember as a new dev being frazzled by the long import paths of openedx.core.djangoapps... in edx-platform. I could never remember the paths; I was always copying and pasting, and it made the platform feel more overwhelming.

Okay, I'll give that a shot. I do like not having apps appear twice in that path (package + apps.py). Thank you!

kdmccormick · 2022-06-06T20:33:55Z

olx_importer/management/commands/load_course_data.py

+            data_bytes = xml_file_path.read_bytes()
+            hash_digest = create_hash_digest(data_bytes)
+            data_str = codecs.decode(data_bytes, 'utf-8')
+            mime_type = f'application/vnd.openedx.xblock.{block_type}+xml'


XBlocks are installed under the entrypoint xblock.v1. I don't think I have my head around the potential consequences of which MIME types we choose, so I'll leave it up to you whether or not you want to add the "v1" namespace here.

Suggested change

mime_type = f'application/vnd.openedx.xblock.{block_type}+xml'

mime_type = f'application/vnd.openedx.xblock.v1.{block_type}+xml'

Ah, right. I guess it doesn't hurt to add the v1, though I'm skeptical we'll ever have a v2. (I'm guessing we'd cali it something entirely different, to avoid confusion.) Will fix.

kdmccormick · 2022-06-06T20:35:45Z

olx_importer/management/commands/load_course_data.py

+            continue
+
+
+            static_file_paths = frozenset(
+                f"static/{static_reference_path}"
+                for static_reference_path in re.findall(static_files_regex, data_str)    
+            )
+            for static_file_path in static_file_paths:
+                if static_file_path in static_asset_paths_to_atom_ids:
+                    atom_id = static_asset_paths_to_atom_ids[static_file_path]
+                    ContentObjectPart.objects.create(content_object=content_obj, content_atom_id=atom_id, identifier=static_file_path)


Ugh, right, because I didn't ever put the static asset mapping stuff back. Thank you.

kdmccormick · 2022-06-06T20:37:46Z

olx_importer/management/commands/load_course_data.py

+        return paths_to_item_raw_ids
+
+
+    def import_block_type(self, block_type, content_path, static_asset_paths_to_atom_ids, item_raw_id_cache, now):


🎉 I know it's just a temporary management command, but it is cool and helpful for me to read some code that uses these models that we've been discussing!

kdmccormick · 2022-06-06T20:43:09Z

openedx_learning/apps/core/itemstore/models.py

+    class Meta:
+        constraints = [
+            models.UniqueConstraint(
+                fields=["learning_context_id", "identifier"],


From this I'm deducing that two Items in different LearningContexts could share the same identifier. Is that right?

Yeah. I think that in general, the identifier fields have to be namespaced so that it's entirely local to one particular LearningContext.

In the case of XBlocks, do you see this identifier as being the usage key (block-v1:a+b+c+type@video+block@xyz) or the block id (just xyz)?

I'd prefer not to have the full learning context key encoded, but since we currently allow blocks of different types to have the same ID, I think we have to encode at least something like video+xyz

kdmccormick · 2022-06-06T20:59:11Z

openedx_learning/apps/core/itemstore/models.py

+
+    TODO: Size limit thoughts–configurable? Start at 10 MB?
+    """
+    learning_context = models.ForeignKey(LearningContext, on_delete=models.CASCADE)


for some item_info, will item_info.learning_context always be equal to item_info.item_raw.learning_context?

Yes. It's mostly there for ownership/cleanup.

Cool. This would be helpful to note in a comment.

kdmccormick · 2022-06-06T21:14:30Z

openedx_learning/apps/core/itemstore/models.py

+    uuid = immutable_uuid_field()
+    item_info = models.ForeignKey('ItemInfo', on_delete=models.RESTRICT)
+    item = models.ForeignKey(Item, on_delete=models.CASCADE)
+    title = models.CharField(max_length=1000, blank=True, null=True)


Why is title here and not in ItemInfo? Is it because it's example of policy, which we're distinguishing from metadata (like type)?

Yes. Renaming is desirable for libraries, because you don't know what kind of convention that library is using, e.g. "Problem 22a" may not make as much sense for the new context it's being used in.

Makes sense!

Should this be title_override then to clarify that you can optionally stick with whatever the library author gave as a title?

I'm inclined to not use the _override convention because the default content in the library would also be using this field (just under the content library's learning context).

Though maybe the title should be lifted out into a separate model? Maybe even one with language awareness?

Wait... maybe ItemRaw:ItemVersion is M:1?

I'll make comments along both of those lines for future followup, but won't make any immediate model changes.

kdmccormick · 2022-06-06T21:20:31Z

openedx_learning/apps/core/itemstore/models.py

+    # TODO: These pointers to the latest published version are convenient, but
+    # we don't currently have the data integrity guarantees to make sure that 
+    # multiple versions aren't active at the same time. Maybe add a ref to the
+    # Item model in LearningContextVersionItemVersion, so we can force that
+    # constraint?
+    published_version = models.ForeignKey('ItemVersion', on_delete=models.RESTRICT, null=True, related_name="+")


knowledge check: this is secondary source of information, since one could authoritatively determine this by asking the publish app what the published LearningContextVersion is, and then looking at the ItemVersions within that LCV, right?

Yes. And maybe it's not a great idea to have redundancy like this. The main motivation for this was that a lot of the time, you want to bind data against the abstract notion of a piece of content across all its versions (e.g. most student state), and I wanted a really convenient way to get to the current "live" version from there.

That does sound convenient. I guess, you're either going to end up needing to keep this field up-to-date, or you're going to end up with a lot of requests making the same query ("what's the published version?"). I don't think I have the experience to know which of these is an easier challenge to manage.

What about making published_version a [memoized] python getter that uses the publish app to return the published version? Or do you mean specifically you want to be able to use the django ORM methods on this published_version field so you can select_related and do things like filtering queries?

Or do you mean specifically you want to be able to use the django ORM methods on this published_version field so you can select_related and do things like filtering queries?

Yeah, exactly this. I want to make sure we can still do efficient filtering queries. But I think I'll actually leave this out for now and add it in later if it becomes necessary. Even if we do keep this kind of a key, it might make more sense in a separate model that specifically separates the publishing details (published_version, first_published_at, last_published_at, etc.).

kdmccormick · 2022-06-06T21:29:30Z

openedx_learning/apps/core/itemstore/models.py

+The item model hiearachy is: Item -> ItemVersion -> ItemInfo -> ItemRaw
+
+Item is the versionless thing that is guaranteed to exist for the lifetime of
+the LearningContext. An ItemVersion is a different version of that item for a
+given LearningContext, and may include policy changes (like grading). ItemInfo
+covers basic metadata that is intrinsic to the item itself, and now how it's
+used in a LearningContext. ItemRaw represents the raw byte data.
+
+TODO: Add link to ADR after it merges.


Ack, sorry, I am struggling to rebuild this hierarchy in my head. I'm going to try to rephrase it here. Let me know if I'm off:

...Assume "LC" is some LearningContext, and "LCV" some LearningContextVersion within LC.

An ItemRaw is a blob of byte data and a mimetype.

Example: an ItemRaw instance "PR", containing some OLX file

An ItemInfo associates an ItemRaw with metadata, including its LearningContext and its item type.

Example: an ItemInfo instance "PI", which tells us that PR is a problem and belongs to LC.

An Item is an atomic block of content associated with an LC. In isolation, an Item does not tell us about its content, because content is associated with versions, whereas an Item is versionless.

Example: an Item instance "P" in LC.

An ItemVersion joins together an abstract Item instance with a concrete ItemInfo, denoting a particular version of the content, policy, and metadata of an Item.

Example: an ItemVersion instance "PV", joining together P and PI.

Example: an ItemVersion instance "PV2", joining together P and PI, with some updated policy.

Example: a later ItemVersion instance "PVx", joining together P and some modified ItemInfo "PIx".

and so on

LearningContextVersionItemVersions relate LearningContextVersions to ItemVersions. This table is the connective tissue telling us which ItemVersions are in a LearningContextVersion (and, by corollary, which Items are in a LearningContext).

Example: PV is in LCV

Example: PV2 is in "LCV2", a later version of LC

Example: PV2 is also in "LCV3", a later version of LC that updates some Item(s) other than P

kdmccormick · 2022-06-06T21:29:55Z

openedx_learning/apps/core/itemstore/models.py

+    """
+    uuid = immutable_uuid_field()
+    learning_context = models.ForeignKey(LearningContext, on_delete=models.CASCADE)
+    type = models.CharField(max_length=100)


Can we always deduce type from item_raw.mime_type?

I had originally meant it as something different, but I'm not sure if that holds up in the latest iteration.

The difference I had in mind originally was something like an HTMLBlock's HTML. That it would have an ItemInfo that showed it was an XBlock with HTML handler, but the raw data had a MIME type of "text/html".

Gotcha. So I guess it depends whether we want to stuff OLX into the item_raws of xblock-typed ItemInfos, or something smarter like the constituent HTML / Loncapa XML / image bytes.

kdmccormick · 2022-06-08T16:52:45Z

openedx_learning/apps/core/itemstore/models.py

+    The other fields on ItemRaw are for data that is intrinsic to the file data
+    itself (e.g. the size). Any smart parsing of the contents into more
+    structured metadata should happen in other models that hang off of ItemInfo.


@ormsbee I am struggling a bit with the distinction between ItemInfo and ItemRaw.

What would we lose by collapsing them into a single model? Is there something bad about structured metadata models hanging off of the same model that's storing the raw bytes?

I think I originally created the idea of ItemInfo (under a different name), when I was thinking it would have more stuff in it (like a local identifier associated with the content package), but those concepts were simplified out of the design. We would have to be careful to not pull back the raw payload all the time, but that's manageable.

The only remaining use case I can think of is when you want to add a little bit of metadata about the content that is not the content itself–new information entered by a human, not something that's being derived by the code from an ItemRaw. At that point, we have to figure out where to hang that data in a way that also clearly signals that this content has changed.

As an example, let's say we have a text description of an image.

If we attach that data to a model that hangs off of ItemRaw, then there's no indication that the content has been updated. None of the versioning mechanisms above this were invoked or needed to be invoked. So we've done an unexpected mutation, and it will invalidate assumptions around versioning and caching. We can't even create a new ItemRaw to bind it to (wasteful as that would be in terms of space), because ItemRaw uses a hash of its contents for identity.

We could attach that data to a model that hangs off of ItemVersion. That's a little weird because that's not policy data that changes from course to course, but we would have default policy information that's attached at that layer and copied, so maybe it would work out...?

You're right in that other comment, it's very much like the definition/usage split in the old system. But maybe we should take that as a cautionary tale–people put many things in the settings scope that should have been put in definitions. If the distinction of when to use ItemInfo is unclear to you as an expert in this kind of system, then it will almost certainly confuse others who try to use this later. Maybe it's easier to say: "If you can completely derive it from the raw data, hang a model off of ItemRaw, and if it's any new data entered in by a user, hang it off of ItemVersion."?

I just realized I need to check an assumption about how I've been reading this: "hanging model X off of model Y" means that X would have a foreign key to Y, right?

I just realized I need to check an assumption about how I've been reading this: "hanging model X off of model Y" means that X would have a foreign key to Y, right?

Yes. I should have been clearer. 😄

ormsbee · 2022-06-09T03:23:04Z

FYI @bradenmacdonald: This is the data models PR I briefly mentioned in our meeting today.

kdmccormick · 2022-07-27T14:04:32Z

Hey @ormsbee , are you looking for another round of review on this one?

ormsbee · 2022-07-27T14:28:46Z

@kdmccormick: Almost. I've been doing bits and pieces in the odd-hours for this. I need to update the import script to make sure these ideas all hold together and then it'll be ready for review. Though I don't expect the models.py files to change very much at this point, so if you want to take another pass at that, you're more than welcome to.

ormsbee · 2022-07-27T21:11:25Z

@kdmccormick: ready for another round of review, with the caveat that I'd ask that you largely ignore load_course_data.py and the staticassets app at this point. I wanted to make sure it was functional (i.e. the data model wasn't totally broken), but the interface is completely wrong for the import script and staticassets is pretty half-baked at the moment.

kdmccormick · 2022-08-03T18:53:46Z

openedx_learning/core/itemstore/models_api.py

+
+    The idea is that if you have a model that is associated with a specific
+    version of an item, the join is going to be 1:1 with an ItemVersion, and
+    potentially M:1 with your data model.


for some instance of this, say ItemVersionData -> Data:

is Data immutable?

is ItemVersionData:Data an M:1 relationship because multiple versions of an Item may reference the same underlying Data?

is Data immutable

It should be or Bad Things will happen.

is ItemVersionData:Data an M:1 relationship because multiple versions of an Item may reference the same underlying Data?

Yes. One of the underlying assumptions here is that if you have some data that's associated with an ItemVersion, changing that data means that the Item has changed in some way, and thus forms a new ItemVersion. So if you change the retry-settings or some such for an Item, there's a new ItemVersion. But for all the other apps that didn't change their data, we can just make new entries that join the new ItemVersion with the same data that those apps had for the last ItemVersion.

@ormsbee

It should be or Bad Things will happen.

Cool, I assumed so but wanted to be sure. Is there a way to indicate that, like marking it read-only in Django admin?

Yes. ...

Makes sense 👍🏻 It would be good to include that explanation somewhere in the library before releasing v1.0. Not a blocker for this PR; I know you're still iterating.

Cool, I assumed so but wanted to be sure. Is there a way to indicate that, like marking it read-only in Django admin?

Yeah, I'll definitely do that when I start making some admin pages.

kdmccormick · 2022-08-03T19:02:16Z

openedx_learning/core/itemstore/models.py

+    # TODO: maybe we should split it up into two fields instead of doing
+    #       substring searches? 
+    mime_type = models.CharField(max_length=255, blank=False, null=False)


maybe we should split it up into two fields instead of doing substring searches?

Sounds like a win to me. Any downsides?

Another reason to split: MIME types can include a parameter field, eg text/plain;charset=utf8. If you don't want to support the parameter field, then splitting into two fields would make that more explicit.

I guess I'm just used to thinking them as a combined thing because I can't really see querying one in isolation from the other. But it is more normalized and it's easy to add, so 🤷 I'll add it.

kdmccormick · 2022-08-03T19:21:42Z

openedx_learning/core/itemstore/models_api.py

+    Minimal abstract model to let people attach data to ItemVersions.
+
+    The idea is that if you have a model that is associated with a specific
+    version of an item, the join is going to be 1:1 with an ItemVersion, and


So, I'm reading that:

each ItemVersion can only be associated with at most one of each type of "Data" model (Image, Asset, Video, etc.), but

each ItemVersion may be associated with an arbitrary number of Content objects.

This confused me at first, because the "Data" models and the Content model occupy very similar roles in my head. But I think I'm beginning to understand it... is this a good example?

Some ItemVersion represents a video.

It has multiple Content items associated with: one that points to the video file, one PNG thumbnail, and several others text files that are transcripts.

In the staticassets app, it has a single associated Video instance and Asset instance. Video metadata as well as information about the transcripts are part of (or hang off) of the Video model.

Yeah, that was my intent. A couple of extra notes about it:

Content survives after apps die.

At some point, an app becomes unsupported and disappears. At that point, all the Item/ItemVersion-related data it has dies as well. But Content is associated with the ItemVersion directly in itemstore, so that raw data will still export.

So I think this encourages a pattern where we store raw things in Content and then build smarter relational data structures out of them connected to ItemVersion. For instance, a ProblemBlock's raw OLX can live in Content while richer relational tables live in a new ProblemBlock app that give a richer, more queryable interface.

ItemVersion mapping to just one row in a "Data" model is common, but not required.

I think that most of the time apps want to associate one piece of data with a given ItemVersion (e.g. a problem weight, schedule, etc.), but that's not a requirement. A rich data model of a ProblemBlock might want to create entries for each response, for instance.

ItemVersion mapping to just one row in a "Data" model is common, but not required.

How would that look?

For example, imagine we have an ItemVersion representing a problem block, and Response data models that we want to (indirectly) hang off of it. Would it look like:

ItemVersion <--(1:1)-- ItemVersionProblemBlock --(N:N)--> Response

or:

ItemVersion <--(1:N)-- ItemVersionResponse --(N:1)--> Response

or something else?

I guess I thought it would be like:

ItemVersion <-- (N:1) -- ItemVersionProblemBlock -- (N:N) --> Response

That would still allow some central process to increment to the next version in a simple way without understanding what ItemVersionProblemBlock is. But I honestly haven't thought it through that much.

feanil

A couple of questions but nothing blocking this from landing. The re-organization generally makes sense to me based on previous conversations.

feanil · 2022-08-03T20:55:57Z

openedx_learning/core/itemstore/models.py

+
+    Each ItemVersion belongs to one and only one Item.
+
+    TODO: created_by field?


Just to confirm, I would see this as something that would be useful for debugging and analyzing but not as a thing that would bubble out to an end user.

Yeah, that was my thought as well.

feanil · 2022-08-03T20:58:25Z

openedx_learning/core/itemstore/models.py

+    # Question: Title is the only thing here that actually has human-readable
+    # text. Does it make sense to lift it out into a separate metadata model,
+    # possibly even one with language awareness?


I think maybe this is a low level enough thing that we don't need to do that. I'm thinking about how the contents if they're XBlocks for example, will probably have a specific language and so maybe it makes more sense to build language awareness at a higher level?

Yeah, I think you're right that it belongs in a higher level. In any case, I'm definitely fine with punting this for now, even if we do bake it at this layer later. I'll remove the comment.

feanil · 2022-08-03T21:10:50Z

openedx_learning/core/itemstore/models.py

+    but changing it will affect all ItemVersions.
+    """
+    uuid = immutable_uuid_field()
+    learning_context = models.ForeignKey(LearningContext, on_delete=models.CASCADE)


So an item can only be in one LearningContext(LC), but since an ItemVersion can be in any number of LearningContextVersions(LCV) do we need a constraint somewhere that ensures that all those LCVs are from the same LC? Or is that too paranoid at this stage?

I like being paranoid about the data model in general, but I'm not sure what we can do with MySQL here, or how much overhead adding CheckConstraints is going to cause. I'll see how hard it is.

feanil · 2022-08-03T21:21:35Z

openedx_learning/core/itemstore/models.py

+    # TODO: maybe we should split it up into two fields instead of doing
+    #       substring searches? 


When would we need to do a partial search? Is it just when looking at types and ignoring sub-types? or something more complex?

Yeah, that's the only situation I can really think of.

David Ormsbee added 2 commits May 25, 2022 22:34

feat: primitive import script to test OLX data with

f40bc39

feat: split Item-related models into new app

dcc51bd

ormsbee requested review from feanil and kdmccormick May 26, 2022 15:01

feanil reviewed Jun 6, 2022

View reviewed changes

openedx_learning/apps/core/itemstore/models.py Outdated Show resolved Hide resolved

kdmccormick reviewed Jun 6, 2022

View reviewed changes

kdmccormick reviewed Jun 8, 2022

View reviewed changes

David Ormsbee added 7 commits June 10, 2022 20:26

refactor: remove "apps" directory layer

dd156ad

refactor: eliminate the ItemInfo model

85603ab

remove 1:1 assumption between ItemVersion and ItemRaw

f4cda31

rename ItemRaw to Content

d443c8f

add more comments about Content relationship

a32840c

fix models and rename apps to noun form

0acab4e

more refactoring to explore model relations

381315b

fixup: make the import script work again

ec0926a

kdmccormick assigned kdmccormick and ormsbee Jul 29, 2022

kdmccormick reviewed Aug 3, 2022

View reviewed changes

feanil approved these changes Aug 3, 2022

View reviewed changes

split mimetype into two

d52ad33

ormsbee merged commit 3125cb9 into openedx:main Aug 7, 2022

kdmccormick mentioned this pull request Feb 8, 2023

Data models for Component-level Versioning Only #28

Merged

	mime_type = f'application/vnd.openedx.xblock.{block_type}+xml'
	mime_type = f'application/vnd.openedx.xblock.v1.{block_type}+xml'

		return paths_to_item_raw_ids


		def import_block_type(self, block_type, content_path, static_asset_paths_to_atom_ids, item_raw_id_cache, now):


		Each ItemVersion belongs to one and only one Item.

		TODO: created_by field?

		# TODO: maybe we should split it up into two fields instead of doing
		# substring searches?

Another pass at basic content models #13

Another pass at basic content models #13

Conversation

ormsbee commented May 26, 2022

kdmccormick 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick Jun 6, 2022 • edited Loading

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

ormsbee Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee commented Jun 9, 2022

kdmccormick commented Jul 27, 2022

ormsbee commented Jul 27, 2022

ormsbee commented Jul 27, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Content survives after apps die.

ItemVersion mapping to just one row in a "Data" model is common, but not required.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feanil 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick Jun 6, 2022 •

edited

Loading

ormsbee Jun 8, 2022 •

edited

Loading