docs: add ADR for cross-instance taxonomy identity#656
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mgwozdz-unicon! 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. |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
I'm not totally opposed to using UUIDs here, but I don't think it's necessary and I do think you can achieve everything that you want by making an effort to make the default value of export_id "more unique" than it currently is.
| No stable, cross-instance identity exists for a ``Taxonomy`` or ``Tag`` today. ``Taxonomy.export_id`` | ||
| and ``Tag.external_id`` are both editable, instance-scoped identifiers designed for import-file | ||
| bookkeeping (see :ref:`openedx-tagging-adr-0006`), not for answering "does the target already have |
There was a problem hiding this comment.
Hmm, I guess it's not documented clearly but the goal of export_id is actually to identify taxonomies across different instances, exactly what you are requesting with this ADR. It is meant for answering "does the target already have this taxonomy."
See openedx/modular-learning#183
"As a content author, I want a consistent way to identify taxonomies, especially across instances".
| Add an immutable ``uuid`` field to ``Taxonomy``, generated at creation and preserved through | ||
| export/import. ``export_id`` cannot serve this purpose on its own: it is free text, chosen and | ||
| editable by whoever administers a taxonomy, so nothing guarantees that the same ``export_id`` on | ||
| two different instances refers to the same taxonomy, or that two different taxonomies on two | ||
| instances never happen to share one. A ``uuid`` is generated once, never touched by a person, and | ||
| so cannot collide or drift the way a human-chosen identifier can. ``export_id`` keeps its existing | ||
| role as the human-meaningful identifier; ``uuid`` adds the machine identity it was never designed | ||
| to provide. This follows the existing convention in this codebase of using ``uuid`` for a stable | ||
| external reference (see ``PublishableEntity.uuid``, :ref:`openedx-content-adr-0003`), applied here | ||
| to a model that currently lacks it. |
There was a problem hiding this comment.
I don't think a UUID is what we want.
If I set up the Lightcast Open Skills taxonomy on instance A, and someone else sets up the Lightcast Open Skills taxononomy on instance B, there should be some way to indicate that they're the same, and future exports/imports across the instances will handle that. But if they each get an immutable UUID at the time of original import, then there is no way to achieve that. Whereas with export ID, it's easy, you just make sure they both have the same export ID like io.lightcast.open-skills.
| ``Taxonomy``/``Tag`` could gain version history (e.g. via ``django-simple-history``, matching | ||
| ``CompetencyCriteria``), which would let drift between source and target be tracked precisely | ||
| rather than detected only as additive or not. Likely needed eventually, but not required for this | ||
| use case, since the additions-only reconciliation policy already satisfies it without history. |
There was a problem hiding this comment.
Please link to #455 which is our tentative plan for making taxonomies versioned.
| Two institutions may independently author what they each consider the same taxonomy on their own | ||
| instances, without ever having copied content between each other. Since a ``uuid`` establishes | ||
| identity only once two taxonomies have actually shared a copy operation, importing between them | ||
| today would create a second, unrelated taxonomy on the receiving side, not recognize them as the | ||
| same one. Automatic matching cannot safely resolve this: falling back to matching by name or | ||
| content similarity would reintroduce the false-positive risk ``uuid`` was introduced to avoid, two | ||
| instances with genuinely different taxonomies that happen to look similar would be silently | ||
| merged. The eventual resolution is a deliberate, human-initiated action: an administrator manually | ||
| reassigns one instance's taxonomy to adopt the other's ``uuid``, retroactively establishing shared | ||
| identity going forward. Not designed here, since it is a distinct, rare operation, orthogonal to | ||
| the copy-time behavior this decision covers. |
There was a problem hiding this comment.
This is what I was saying above, but it can be done more easily via export_id. It's awkward to compare and edit UUIDs.
Reuse the existing Taxonomy.export_id field for cross-instance identity instead of adding a new uuid field, per review feedback.
|
Thanks @bradenmacdonald , this makes sense, went with You're right that Additionally:
Let me know if I've missed anything. |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
I agree with @bradenmacdonald's review, and using export_id. It's similar to what we already do for things like courses and libraries, where we rely on meaningful keys.
One thing I want flag is that courses and libraries are implicitly namespaced to an org, which is something that we're not currently doing with taxonomies. We don't really have system vs. org-level libraries or courses. If there is ever a situation in which multiple orgs want to independently tweak or update the same logical taxonomy, we may want to consider some kind of namespacing conventions.
I added a note about this to the "Excluded from this decision" section to flag it for the future. |
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Some clarifications needed :)
| Context | ||
| ------- | ||
|
|
||
| Competency Based Education (CBE) will bring about the use case where, when course content is |
There was a problem hiding this comment.
Before this ADR uses "competency criteria," can we get one plain sentence defining it? Something like: "A competency criterion asserts that a piece of course content demonstrates a specific competency, which it identifies by pointing at a tag in a taxonomy." Right now CompetencyCriteria/CompetencyCriteriaGroup show up in parentheses on line 17 with zero setup, and this ADR lives in openedx_tagging, which explicitly isn't supposed to know about competencies. A reader coming from the tagging side needs to know why a criterion touches a taxonomy at all before the rest of the Context makes sense.
| it. Competency criteria (``CompetencyCriteria``, ``CompetencyCriteriaGroup``) are versioned via | ||
| ``django-simple-history`` per :ref:`openedx-learning-adr-0003`, but the taxonomies and tags they | ||
| reference are deliberately non-evaluative, unversioned display metadata. Copying a criterion | ||
| therefore also means resolving the taxonomy and tags it points to on the target. |
There was a problem hiding this comment.
| it. Competency criteria (``CompetencyCriteria``, ``CompetencyCriteriaGroup``) are versioned via | |
| ``django-simple-history`` per :ref:`openedx-learning-adr-0003`, but the taxonomies and tags they | |
| reference are deliberately non-evaluative, unversioned display metadata. Copying a criterion | |
| therefore also means resolving the taxonomy and tags it points to on the target. | |
| it. Each competency criterion (CompetencyCriteria) identifies the competency it evaluates by referencing a specific Tag in a Taxonomy. Criteria themselves are versioned via django-simple-history (see :ref:openedx-learning-adr-0003), but the taxonomy and tags they reference are deliberately not: they're non-evaluative display metadata that can change independently without creating a new version of the criteria pointing at them. Copying a criterion therefore also means resolving the taxonomy and tags it points to on the target, since the copy has to land somewhere with a matching tag for it to reference. |
| Copy semantics | ||
| ~~~~~~~~~~~~~~ | ||
|
|
||
| Competency criteria are copied **by reference**: the target's criteria are bound to a taxonomy |
There was a problem hiding this comment.
Does this actually work?
- A
CompetencyCriteriais indeed bound to a tag application that is bound to an object and a taxonomy, where the object is an object in e.g. the course that's being copied. Does a course copy copy the taxonomy, and do the tags resolve so that the object tag ids actually apply to the new taxonomy_id that has the same export_id? - A
CompetencyCriteriais also bound to aCompetencyCriteriaGroup. How is thisCompetencyCriteriaGroupcopied to the new course or content? - Do I understand by reference to mean a reference of the criteria to the export_id of a taxonomy? Or by reference to the competency criteria? In that case I'm confused who references them and how the reference could be resolved if we have a different environment with a different database.
- A
CompetencyCriteriaGroupis optionally bound to acourse_id. I assume that is a course run. I assume it actually has to have the course_id to apply to a course. Since aCompetencyCriteriabelongs to aCompetencyCriteriaGroupthat belongs to a course, wouldn't copying it by reference mean that it does not apply to the new course now?
There was a problem hiding this comment.
To clarify my point 1:
An object_tag encodes a course run in the object_id. So if we do a course re-run, for example, the object_tags reference the previous course run, which means that the new course has no tags. When tags are copied to the new course, the copy of the tags somehow needs to be resolved to the new object_ids of the course content, not the same as previously. I assume this is handled via this tags file in course imports - and we will now apply the same logic to re-runs and such - but can you confirm this assumption is true?
There was a problem hiding this comment.
I am not sure if this is handled at all in course re-runs today. (But it should be.) Some testing will be required.
|
Claude flagged this: |
Distinguishes CompetencyCriteria/CompetencyCriteriaGroup/ObjectTag recreation (copied by value, re-keyed to new object/course ids) from Taxonomy/Tag identity (never duplicated, resolved by reference via export_id). Also defines competency criterion in plain terms with a reference to ADR 0002, and ties "course" to CourseRun (openedx_catalog), reconciling with ADR 0002's course_id.
|
@jesperhodge, addressed your comments:
On the course_id inconsistency you flagged separately: checked both ADRs against the actual @bradenmacdonald your suspicion on the rerun/ObjectTag question was right, confirmed above. |
|
@mgwozdz-unicon @jesperhodge Note that there is code to duplicate tags when an XBlock is duplicated but reruns don't involve the We probably need to add a similar handler for the |
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Looks very good! No reservations from my side, this looks ready.
Feel free to reject all the suggestions! They are completely optional.
As we have been looking at making ADRs relatively concise and skip implementation detail, I've just gone through this and looked for places that could be shortened or where we can skip implementation details as such.
This is not meant as "please improve this", but instead just as inspiration for you on how we can make this ADR more concise, if desired. Shortening technical documents is hard, so I thought I just see what suggestions may be helpful to you.
| Use the existing ``Taxonomy.export_id`` field as the cross-instance identity, rather than adding a | ||
| new field. ``export_id`` is already required, unique, and format-validated | ||
| (``^[\w\-.]+$``) at the model level, and the REST API already accepts a caller-supplied value on | ||
| taxonomy creation. Two institutions that each set up the same third-party taxonomy (for example, | ||
| Lightcast Open Skills) can establish that they're the same taxonomy simply by using the same | ||
| ``export_id`` (for example, a reverse-DNS-style value like ``io.lightcast.open-skills``), something | ||
| an immutable, randomly-generated identifier could never let them do, since two independent imports | ||
| would always get two different random values with no way to reconcile them afterward. |
There was a problem hiding this comment.
| Use the existing ``Taxonomy.export_id`` field as the cross-instance identity, rather than adding a | |
| new field. ``export_id`` is already required, unique, and format-validated | |
| (``^[\w\-.]+$``) at the model level, and the REST API already accepts a caller-supplied value on | |
| taxonomy creation. Two institutions that each set up the same third-party taxonomy (for example, | |
| Lightcast Open Skills) can establish that they're the same taxonomy simply by using the same | |
| ``export_id`` (for example, a reverse-DNS-style value like ``io.lightcast.open-skills``), something | |
| an immutable, randomly-generated identifier could never let them do, since two independent imports | |
| would always get two different random values with no way to reconcile them afterward. | |
| Use the existing ``Taxonomy.export_id`` field as the cross-instance identity across multiple institutions and external systems, rather than adding a | |
| new field. |
| ``Tag`` does not need a new identifier: ``Tag.external_id`` (already used by the tag import/export | ||
| plan-building logic, see :ref:`openedx-tagging-adr-0006`) is sufficient for within-taxonomy | ||
| matching. Free-text taxonomies have no ``Tag`` rows and travel as literal strings unconditionally; | ||
| no reconciliation applies to them. |
There was a problem hiding this comment.
| ``Tag`` does not need a new identifier: ``Tag.external_id`` (already used by the tag import/export | |
| plan-building logic, see :ref:`openedx-tagging-adr-0006`) is sufficient for within-taxonomy | |
| matching. Free-text taxonomies have no ``Tag`` rows and travel as literal strings unconditionally; | |
| no reconciliation applies to them. | |
| The existing ``Tag.external_id`` is sufficient for within-taxonomy matching. |
| ``export_id``'s existing mutability (it can be edited after creation) is also a feature here rather | ||
| than a gap: it is how the deferred manual-merge case below would actually be performed, by editing | ||
| one instance's ``export_id`` to match the other's. |
There was a problem hiding this comment.
| ``export_id``'s existing mutability (it can be edited after creation) is also a feature here rather | |
| than a gap: it is how the deferred manual-merge case below would actually be performed, by editing | |
| one instance's ``export_id`` to match the other's. | |
| ``export_id``'s remains mutable and supports the manual-merge case below by editing | |
| one instance's ``export_id`` to match the other's. |
| The ``Taxonomy``/``Tag`` rows a recreated ``ObjectTag`` points to are handled differently: they are | ||
| never duplicated, only referenced, and it's this taxonomy/tag relationship the rest of this | ||
| decision means by **by reference**. For a same-instance mechanism (new course run, library copy), | ||
| source and target already share the identical taxonomy row, so no resolution is needed at all. For | ||
| a cross-instance mechanism (course export/import), the reference is resolved to whichever taxonomy | ||
| on the target shares the source's ``export_id``, per Resolution on import, below. This is a larger | ||
| commitment than a by-value taxonomy copy, but a by-value copy would leave the target's competency | ||
| evaluation permanently disconnected from the taxonomy it depends on, undermining the goal of this | ||
| use case. |
There was a problem hiding this comment.
| The ``Taxonomy``/``Tag`` rows a recreated ``ObjectTag`` points to are handled differently: they are | |
| never duplicated, only referenced, and it's this taxonomy/tag relationship the rest of this | |
| decision means by **by reference**. For a same-instance mechanism (new course run, library copy), | |
| source and target already share the identical taxonomy row, so no resolution is needed at all. For | |
| a cross-instance mechanism (course export/import), the reference is resolved to whichever taxonomy | |
| on the target shares the source's ``export_id``, per Resolution on import, below. This is a larger | |
| commitment than a by-value taxonomy copy, but a by-value copy would leave the target's competency | |
| evaluation permanently disconnected from the taxonomy it depends on, undermining the goal of this | |
| use case. | |
| The ``Taxonomy``/``Tag`` rows a recreated ``ObjectTag`` points to are handled differently: they are | |
| never duplicated, only referenced, and it's this taxonomy/tag relationship the rest of this | |
| decision means by **by reference**. |
Since resolution on import is already outlined below, we don't need it here again.
| evaluation permanently disconnected from the taxonomy it depends on, undermining the goal of this | ||
| use case. | ||
|
|
||
| Resolution on import |
There was a problem hiding this comment.
| Resolution on import | |
| Resolution on course export/import |
| This section, and Reconciliation on repeat import, below, apply specifically to course | ||
| export/import: it is the only mechanism where a matching taxonomy might not already exist on the | ||
| target. New course run and library copy need no taxonomy-side resolution at all, per Copy | ||
| semantics, above. | ||
|
|
There was a problem hiding this comment.
| This section, and Reconciliation on repeat import, below, apply specifically to course | |
| export/import: it is the only mechanism where a matching taxonomy might not already exist on the | |
| target. New course run and library copy need no taxonomy-side resolution at all, per Copy | |
| semantics, above. |
No need to list everything that is not affected.
| target. New course run and library copy need no taxonomy-side resolution at all, per Copy | ||
| semantics, above. | ||
|
|
||
| On import, whether the source and target are the same deployment or two different organizations' |
There was a problem hiding this comment.
| On import, whether the source and target are the same deployment or two different organizations' | |
| On course import, whether the source and target are the same deployment or two different organizations' |
| A single uniform rule was chosen over branching by deployment relationship because the | ||
| reconciliation policy below already guards against silent corruption in both cases; adding a | ||
| second behavior for the cross-organization case would add complexity without removing risk. |
There was a problem hiding this comment.
If necessary, this can explained in the "rejected alternatives" section, or it can just be dropped.
| A single uniform rule was chosen over branching by deployment relationship because the | |
| reconciliation policy below already guards against silent corruption in both cases; adding a | |
| second behavior for the cross-organization case would add complexity without removing risk. |
| The actor triggering a course copy has no standing to mutate a taxonomy shared with other, | ||
| unrelated content on the target instance. In a manual tag re-import (the existing use of this | ||
| plan-building logic), the file represents the deliberate intent of whoever owns that taxonomy. A | ||
| copied course's snapshot only reflects what the source looked like at export time: a tag missing | ||
| from it doesn't mean the source deleted it, it may be something the target added independently. | ||
| Auto-applying deletes or renames on that basis risks silently corrupting taxonomy state that | ||
| unrelated courses on the target depend on. Additions carry no such risk: they only ever add new | ||
| identities, never touch existing ones. |
There was a problem hiding this comment.
| The actor triggering a course copy has no standing to mutate a taxonomy shared with other, | |
| unrelated content on the target instance. In a manual tag re-import (the existing use of this | |
| plan-building logic), the file represents the deliberate intent of whoever owns that taxonomy. A | |
| copied course's snapshot only reflects what the source looked like at export time: a tag missing | |
| from it doesn't mean the source deleted it, it may be something the target added independently. | |
| Auto-applying deletes or renames on that basis risks silently corrupting taxonomy state that | |
| unrelated courses on the target depend on. Additions carry no such risk: they only ever add new | |
| identities, never touch existing ones. |
Probably not needed.
| A refusal is modeled as an ordinary import task failure, using the existing | ||
| ``UserTaskStatus.fail(message)`` / ``Error`` artifact / ``import_status_handler`` mechanism in | ||
| ``contentstore``. The Studio Authoring MFE's import flow already reads this same ``Message`` field | ||
| (``CourseImportContext.tsx``), so this requires no new frontend or backend surface, and no | ||
| distinction needs to be drawn between a Platform Administrator and a Course Author watching the | ||
| same import: whoever is watching sees the existing failure message. |
There was a problem hiding this comment.
| A refusal is modeled as an ordinary import task failure, using the existing | |
| ``UserTaskStatus.fail(message)`` / ``Error`` artifact / ``import_status_handler`` mechanism in | |
| ``contentstore``. The Studio Authoring MFE's import flow already reads this same ``Message`` field | |
| (``CourseImportContext.tsx``), so this requires no new frontend or backend surface, and no | |
| distinction needs to be drawn between a Platform Administrator and a Course Author watching the | |
| same import: whoever is watching sees the existing failure message. | |
| A refusal is modeled as an ordinary import task failure. |
We can probably drop the rest as implementation details, especially since those mechanics already exist on edx-platform.
6 participants
uuidfield onTaxonomyso that competencycriteria copied along with course content (new course run, course export/import, course/library
copy) can be bound by reference to the correct taxonomy on the target instance, rather than an
export_id match that isn't reliable across instances.
only, apply it; if it includes any rename, reparent, or delete, refuse the import rather than
risk silently mutating taxonomy state that unrelated content on the target depends on.
copy, the newer
openedx_contentComponent/Container model, and full taxonomy/tag versioningare explicitly out of scope, noted as excluded or deferred with reasoning.