search

openedx / XBlock

Framework for building custom learning components that run in the Open edX LMS!
https://docs.openedx.org/projects/xblock/en/latest/xblock-tutorial/index.html
Apache License 2.0
451 stars 216 forks source link

[DEPR]: Support for XBlock Runtimes with raw string scope IDs #784

Open kdmccormick opened 2 weeks ago

kdmccormick commented 2 weeks ago

In other words: Starting with XBlock 6.0.0, we will assume that XBlock Scope IDs are instances of OpaqueKey.

(Most if not all people can ignore this DEPR. Only operators with entirely custom XBlock Runtime implementations need to pay attention. We're actually not aware of any such custom Runtime implementations currently, so this DEPR is most likely just a formality.)

Proposal Date

Proposed 2024-08-27 Communicated 2024-09-02

Target Ticket Acceptance Date

2024-09-11

Earliest Open edX Named Release Without This Functionality

Sumac (master: Late Sept 2024)

Rationale

History

edx-platform was created circa 20211 and the XBlock framework was created circa 2012. Originally, those systems used carefully-formatted strings (str instances) to identify XBlocks in various contexts:

Circa 2014, in order to deal with the fragility and complexity of those strings in the light of a major overhaul of edx-platform's MongoDB schema (from "old" ModuleStore to "split" ModuleStore), we created the opaque-keys package. An OpaqueKey is a value object which identifies an XBlock scope and has a well-defined string representation; each subclass of OpaqueKey identifies a different kind of scope. Specifically, in this new system:

edx-platform was completely migrated over from string IDs to OpaqueKey IDs and has exclusively used opaque-keys for the past decade.

However, this migration was never represented in the XBlock framework. In theory, any object can be used as an XBlock scope ID. XBlock tests still use string IDs, as does the xblock workbench. In practice, though, edx-platform is the only production XBlock runtime that any of us are aware of, so in the real world XBlocks are all running with OpaqueKey scope IDs rather than string scope IDs.

Rationale for now assuming that scope IDs are OpaqueKeys

As an XBlock developer, it is confusing that the XBlock documentation makes no mention of OpaqueKeys, and it is strange and unhelpful that the XBlock Workbench identifies blocks in a way that is inconsistent from edx-platform.

Furthermore, we are adding type annotations to the XBlock package. The type annotations would be significantly less potent and instructive if they had to support string IDs: they would all be typed as object or Any, so edx-platform would need to disable mypy with # type: ignore wherever it treated an ID is an OpaqueKey. However, once we assume that scope IDs are all OpaqueKeys, we can annotate various XBlock API signatures with LearningContextKey, UsageKey, DefinitionKey, etc.. This will allow for stronger correctness checking in CI, better documentation for core and plugin developers, and more accurate code intelligence for developers using IDEs.

Removal

Beginning in XBlock 6.0.0:

Replacement

N/A

Deprecation

In the interest of making it easy to update unit tests, we will make the XBlock API raise obvious assertion errors wherever the new scope-IDs-are-OpaqueKeys assumption is violated.

We do not plan to raise warnings ahead of time. We believe that only the XBlock Workbench (xblock-sdk) is in violation of this assumption, and Axim will take care of fixing it.

Migration

If any production XBlock Runtimes exist using string scope IDs, those IDs can be substituted with OpaqueKey instances whose __str__ methods generate identical scope IDs. We do not expect any such Runtimes to exist, though. If you know of one and need help understanding the migration, please reach out.

Additional Info

None

Task List

edx-platform repo

Constrain XBlock to < 6:

# Date: 2024-mm-dd
# Description: XBlock>=6 drops support for string scope IDs. We expect that this will
# not break any edx-platform app code, but we should smoke-test that expectation, and
# we may need to fix some unit tests that use string scope IDs.
# Ticket: https://github.com/openedx/XBlock/issues/784
XBlock[django]<6

XBlock repo

xblock-sdk repo

back to the edx-platform repo

kdmccormick commented 2 weeks ago

@bradenmacdonald we talked a few months ago about making this change. Here's a formal DEPR for it.

@ormsbee , my understanding and my research indicates that edx-platform will be completely unaffected by this. Let me know if you disagree.

@robrap , just a heads up that I plan to bring this up at the edx-platform maintenance WG meeting on Thursday. I'm going to propose that the 6-month waiting period be waived since this shouldn't impact 2U, or any other known site operator for that matter.

robrap commented 2 weeks ago

Thanks @kdmccormick. Thoughts:

  1. I'd drop the "if necessary", and simply constrain XBlock to <6. You can do that as early as you wish, and if you use the updated style of constraint comment , it would be the best way to communicate this work without having to be concerned with timing between the library publish and upgrade work.
  2. I agree that the 6-month waiting period could be waived, unless you learn about something that neither of us considered.
  3. From an edx-platform perspective, hopefully your assumptions prove true and this is a smooth refactor.
ormsbee commented 2 weeks ago

@kdmccormick: I had totally forgotten this still existed. 😱 No objections.

kdmccormick commented 2 weeks ago

I'd drop the "if necessary", and simply constrain XBlock to <6. You can do that as early as you wish, and if you use the https://github.com/openedx/edx-platform/issues/35055, it would be the best way to communicate this work without having to be concerned with timing between the library publish and upgrade work.

@robrap Sounds good. Task list updated 👍🏻

kdmccormick commented 6 days ago

This is now Accepted. I will work through the removal steps listed above, hopefully soon, definitely before the Sumac cutoff.