-
Notifications
You must be signed in to change notification settings - Fork 227
Plugin extensible xblock services #927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
felipemontoya
wants to merge
3
commits into
openedx:master
Choose a base branch
from
eduNEXT:fmo/extensible-services
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| 0001 Plugin-provided runtime services via entry points | ||
| ###################################################### | ||
|
|
||
| Status | ||
| ****** | ||
|
|
||
| Proposed | ||
|
|
||
| Context | ||
| ******* | ||
|
|
||
| XBlocks consume capabilities from their environment through *runtime | ||
| services*: a block declares ``@XBlock.needs("name")`` or | ||
| ``@XBlock.wants("name")`` and calls ``self.runtime.service(self, "name")``. | ||
| The base ``Runtime.service()`` resolves the name against the ``_services`` | ||
| dict that the runtime application populated at construction time. | ||
|
|
||
| This makes the *consumption* side of services fully generic, but the | ||
| *provision* side closed: only the application that instantiates the runtime | ||
| can decide which services exist. In Open edX — by far the largest user of | ||
| this library — service wiring is hardcoded in several places | ||
| (``ModuleStoreRuntime`` service dicts for LMS/Studio/preview, and the | ||
| ``if/elif`` chain in the newer ``XBlockRuntime``), and there is no supported | ||
| way for a separately installed package to offer a new service. | ||
|
|
||
| The need is real and recurring. The motivating case is an AI-extensions | ||
| plugin that wants to offer an ``"ai_extensions"`` service so that blocks like | ||
| ORA can call LLM workflows without pinning provider SDKs or importing plugin | ||
| internals (see the community thread in the References). But the same gap | ||
| applies to any optional capability a pip-installed package might offer to | ||
| blocks: translation backends, proctoring integrations, institution-specific | ||
| storage, and so on. | ||
|
|
||
| Two facts about the existing design make this library the right place to | ||
| close the gap: | ||
|
|
||
| 1. **Every runtime already funnels through ``Runtime.service()``.** Open edX | ||
| runtimes either populate ``_services`` and delegate to the base method | ||
| (``ModuleStoreRuntime``), or run their own chain and fall back to the base | ||
| method (``XBlockRuntime``). The xblock-sdk workbench uses the base | ||
| behavior directly. A fallback added here is therefore reached by every | ||
| known runtime without any changes to host applications. | ||
|
|
||
| 2. **The library already has the discovery machinery and the stated intent.** | ||
| ``xblock/plugin.py`` loads XBlocks (``xblock.v1``) and asides | ||
| (``xblock_asides.v1``) from entry points, with caching, ambiguity | ||
| detection, and an ``.overrides`` group. The reference ``Service`` class in | ||
| ``xblock/reference/plugins.py`` has documented the goal for years: services | ||
| should *"be able to load through Stevedore, and have a plug-in mechanism | ||
| similar to XBlock."* | ||
|
|
||
| Decision | ||
| ******** | ||
|
|
||
| Add a third entry-point group to the XBlock framework, ``xblock.service.v1``, | ||
| and a fallback in ``Runtime.service()`` — the ``_load_service_from_entry_point`` | ||
| method — that consults it. | ||
|
|
||
| A package provides a service by declaring:: | ||
|
|
||
| entry_points={ | ||
| "xblock.service.v1": [ | ||
| "my_service = my_package.services:MyService", | ||
| ], | ||
| } | ||
|
|
||
| where the entry-point name is the service name blocks declare with | ||
| ``needs``/``wants``. Resolution order in ``Runtime.service()`` becomes: | ||
|
|
||
| 1. Reject undeclared requests (unchanged): a block that never declared the | ||
| service still gets ``NoSuchServiceError``. | ||
| 2. Return the runtime-provided service from ``_services`` if present | ||
| (unchanged). | ||
| 3. **New:** if the runtime has nothing, try | ||
| ``_load_service_from_entry_point(block, service_name)``, which loads the | ||
| provider class from the ``xblock.service.v1`` group and instantiates it as | ||
| ``provider_class(runtime=self, xblock=block)``. | ||
| 4. Apply ``need``/``want`` semantics to the result (unchanged): ``None`` for | ||
| a wanted-but-absent service, ``NoSuchServiceError`` for a needed one. | ||
|
|
||
| Reasoning behind the specific choices | ||
| ===================================== | ||
|
|
||
| **Why a fallback in the base class rather than a hook in each runtime.** | ||
| Placing the lookup after the ``_services`` miss, inside the one method every | ||
| runtime inherits, gives complete coverage (all Open edX runtimes, the | ||
| workbench, third-party runtimes that don't override ``service()``) for a | ||
| single small change, and gives a hard guarantee: *runtime-provided services | ||
| always shadow plugin-provided ones*. A pip package cannot replace or | ||
| intercept ``user``, ``field-data``, ``i18n``, or any other service the host | ||
| application provides deliberately. "Provided" is decided by key presence in | ||
| ``_services``, not truthiness: runtimes use an explicit ``None`` to mean | ||
| "this service exists but is disabled here" — the Open edX LMS maps | ||
| ``completion`` to ``None`` for anonymous users, and this library's own test | ||
| suite passes ``services={'i18n': None}`` — and a plugin must not resurrect a | ||
| service the runtime switched off. Runtimes that override ``service()`` | ||
| entirely keep that freedom — the fallback only exists in the default path | ||
| they opt into by calling ``super().service()``. | ||
|
|
||
| **Why entry points rather than configuration.** Entry points are how this | ||
| library already discovers XBlocks and asides, so providers and operators deal | ||
| with one consistent model: installing a package is the act that makes its | ||
| plugins available, and the trust decision is the install decision — exactly | ||
| as it is for XBlocks themselves. A settings-based registry would be | ||
| runtime-application-specific (this library is not Django-bound) and would put | ||
| the burden of wiring on every operator instead of on the providing package. | ||
|
|
||
| **Why the existing ``Plugin`` loader.** Reusing ``Plugin.load_class`` buys, | ||
| for free: per-process caching of hits *and misses* (steady-state cost of the | ||
| fallback is one dict lookup); loud ``AmbiguousPluginError`` when two installed | ||
| packages claim the same service name, instead of last-write-wins — the exact | ||
| failure mode that makes monkey-patching unacceptable; a sanctioned override | ||
| path (``xblock.service.v1.overrides``) when replacing a default implementation | ||
| is intentional; and ``register_temp_plugin`` for tests. | ||
|
|
||
| **Why ``provider_class(runtime=…, xblock=…)``.** This mirrors the | ||
| constructor of the reference ``Service`` class, gives the provider the two | ||
| context objects almost every service needs (and from which the rest — user, | ||
| usage key, learning context — is reachable), and keeps the contract so small | ||
| that providers do not need to import ``xblock`` at all. Note that the | ||
| fallback returns an *instance*, never a class: some runtimes | ||
| (``ModuleStoreRuntime``) call callable services with ``(block)``, and a | ||
| class-valued service would be invoked accidentally. Instantiation is | ||
| per-request for now; providers with expensive set-up are expected to cache it | ||
| themselves (module- or class-level), consistent with the long-standing | ||
| "don't over-initialize" guidance in ``reference/plugins.py``. Memoizing per | ||
| ``(runtime, service_name)`` in the base class is a possible follow-up once | ||
| real-world usage shows it is needed. | ||
|
|
||
| **Why the ``needs``/``wants`` gate stays in front.** The declaration check | ||
| runs before any entry-point lookup, so a plugin-provided service is only ever | ||
| handed to blocks that explicitly asked for it. ``wants`` gives blocks a | ||
| portable soft-dependency: the same block works on installs with and without | ||
| the providing package, enabling features conditionally. | ||
|
|
||
| Rejected alternatives | ||
| ===================== | ||
|
|
||
| * **Wiring extension points into each host-application runtime** (new | ||
| ``openedx.*`` entry-point group, an ``XBLOCK_EXTRA_SERVICES`` Django | ||
| setting, or an openedx-filters filter at resolution time) — all viable, but | ||
| each covers only the call sites it patches, must be replicated for every | ||
| current and future runtime, and lives in repositories whose architectural | ||
| direction is to *shrink* their XBlock-runtime surface, not grow it. These | ||
| were prototyped and documented by the openedx-ai-extensions project (see | ||
| References) before converging here. | ||
|
|
||
| Consequences | ||
| ************ | ||
|
|
||
| * Installed packages can provide named runtime services to consenting blocks | ||
| on any runtime that uses the default resolution path; no host-application | ||
| changes are required. | ||
| * The service namespace becomes shared between runtime applications and | ||
| installed packages. Runtimes always win, and duplicate provider claims | ||
| fail loudly, but a future registry of well-known service names would help | ||
| providers avoid accidental collisions. | ||
| * Operators implicitly accept a package's service registrations by installing | ||
| it, as with XBlocks. If field experience shows a need for finer control, a | ||
| block-list mechanism can be layered on without changing the provider | ||
| contract. | ||
| * The behavior of every existing runtime and block is unchanged unless a | ||
| package registering ``xblock.service.v1`` entry points is installed. | ||
|
|
||
| References | ||
| ********** | ||
|
|
||
| * Community discussion: https://discuss.openedx.org/t/plugin-provided-xblock-runtime-services/18682 | ||
| * Prior analysis and prototypes of the platform-side alternatives: | ||
| ADR-0005 and ADR-0011 in https://github.com/openedx/openedx-ai-extensions | ||
| * Original pluggability intent: ``xblock/reference/plugins.py`` (``Service`` | ||
| docstring) | ||
| * Discovery machinery reused: ``xblock/plugin.py`` | ||
| * Open edX platform ADR *Role of XBlocks* (scope reduction of the platform | ||
| runtime): ``docs/decisions/0006-role-of-xblock.rst`` in edx-platform | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| """ | ||
| Tests for runtime services provided by installed packages via the | ||
| ``xblock.service.v1`` entry-point group. | ||
| """ | ||
| import pytest | ||
|
|
||
| from xblock.core import XBlock | ||
| from xblock.exceptions import NoSuchServiceError | ||
| from xblock.fields import ScopeIds | ||
| from xblock.runtime import ServiceProvider | ||
| from xblock.test.tools import TestRuntime | ||
|
|
||
|
|
||
| class DummyAIService: | ||
| """A service provider class, as a plugin package would define it.""" | ||
|
|
||
| def __init__(self, **kwargs): | ||
| self.runtime = kwargs.get('runtime') | ||
| self.xblock = kwargs.get('xblock') | ||
|
|
||
| def run_profile(self, profile_id, user_input): | ||
| """A representative service method.""" | ||
| return f"ran {profile_id} with {user_input!r}" | ||
|
|
||
|
|
||
| @XBlock.wants('ai_extensions') | ||
| class WantsAIBlock(XBlock): | ||
| """An XBlock that can optionally use the ai_extensions service.""" | ||
|
|
||
|
|
||
| @XBlock.needs('ai_extensions') | ||
| class NeedsAIBlock(XBlock): | ||
| """An XBlock that requires the ai_extensions service.""" | ||
|
|
||
|
|
||
| def make_block(block_class, runtime=None): | ||
| """Construct a block of `block_class` in a fresh TestRuntime.""" | ||
| runtime = runtime or TestRuntime() | ||
| return runtime.construct_xblock_from_class( | ||
| block_class, ScopeIds('user', 'test', 'def_id', 'usage_id'), | ||
| ) | ||
|
|
||
|
|
||
| @ServiceProvider.register_temp_plugin( | ||
| DummyAIService, identifier='ai_extensions', group='xblock.service.v1', | ||
| ) | ||
| def test_plugin_service_loaded_from_entry_point(): | ||
| block = make_block(WantsAIBlock) | ||
| service = block.runtime.service(block, 'ai_extensions') | ||
| assert isinstance(service, DummyAIService) | ||
| assert service.runtime is block.runtime | ||
| assert service.xblock is block | ||
| assert service.run_profile('profile-1', 'hi') == "ran profile-1 with 'hi'" | ||
|
|
||
|
|
||
| @ServiceProvider.register_temp_plugin( | ||
| DummyAIService, identifier='ai_extensions', group='xblock.service.v1', | ||
| ) | ||
| def test_runtime_service_shadows_plugin_service(): | ||
| sentinel = object() | ||
| runtime = TestRuntime(services={'ai_extensions': sentinel}) | ||
| block = make_block(WantsAIBlock, runtime=runtime) | ||
| assert block.runtime.service(block, 'ai_extensions') is sentinel | ||
|
|
||
|
|
||
| @ServiceProvider.register_temp_plugin( | ||
| DummyAIService, identifier='ai_extensions', group='xblock.service.v1', | ||
| ) | ||
| def test_runtime_none_service_disables_plugin_service(): | ||
| wants_block = make_block( | ||
| WantsAIBlock, runtime=TestRuntime(services={'ai_extensions': None}), | ||
| ) | ||
| assert wants_block.runtime.service(wants_block, 'ai_extensions') is None | ||
|
|
||
| needs_block = make_block( | ||
| NeedsAIBlock, runtime=TestRuntime(services={'ai_extensions': None}), | ||
| ) | ||
| with pytest.raises(NoSuchServiceError): | ||
| needs_block.runtime.service(needs_block, 'ai_extensions') | ||
|
|
||
|
|
||
| def test_missing_plugin_service_wanted_returns_none(): | ||
| block = make_block(WantsAIBlock) | ||
| assert block.runtime.service(block, 'ai_extensions') is None | ||
|
|
||
|
|
||
| def test_missing_plugin_service_needed_raises(): | ||
| block = make_block(NeedsAIBlock) | ||
| with pytest.raises(NoSuchServiceError): | ||
| block.runtime.service(block, 'ai_extensions') | ||
|
|
||
|
|
||
| @ServiceProvider.register_temp_plugin( | ||
| DummyAIService, identifier='ai_extensions', group='xblock.service.v1', | ||
| ) | ||
| def test_undeclared_plugin_service_still_raises(): | ||
| block = make_block(XBlock) # declares neither needs nor wants | ||
| with pytest.raises(NoSuchServiceError): | ||
| block.runtime.service(block, 'ai_extensions') |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand why we have a
needsdeclaration (to show an appropriate error when a required service is not available). But I never understood the point of declaringwants. It just seems like pointless boilerplate."We don't allow XBlocks to access a service unless they declare it with needs/wants, because if they don't declare it, ___" What? It would still work totally fine if every XBlock implicitly
wantsevery service, without having to declare them.