feat: reuse runtime services and wrappers between XBlocks [FC-0026] #32420

Agrendalath · 2023-06-12T11:50:14Z

Description

The goal of FC-0026 is to remove the XBlock-specific handling from the prepare_runtime_for_user function. This part handles the final step, which is introducing runtime service reusability.

In a single request, each XBlocks use the same runtime instance. Therefore, when the services are initialized for the user in the course, these services are overridden¹, so all XBlock-specific data is lost. In previous FC-0026 PRs, we have removed the block-specific code from the prepare_runtime_for_user function. This way, it no longer matters which XBlock bound the runtime to the user. This PR ensures that the runtime binding happens only once for each user².

Supporting information

Private-ref: BB-7448

Testing instructions

Ensure that XBlocks work correctly in the LMS, Preview, and Studio. Check that importing and exporting the course works.

Other notes

Please ignore the first two commits, as they come from #32357.

You can see this comment, which explains the fragile behavior related to the runtime initialization order. ↩
There are two use cases for this:
1. While rebinding the user in the LTI XBlock.
2. In the Instructor Dashboard tasks - when a Problem Block is rescored for multiple users.
↩

openedx-webhooks · 2023-06-12T11:50:19Z

Thanks for the pull request, @Agrendalath!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

edx-pipeline-bot · 2023-06-13T13:04:30Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-06-13T13:26:28Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-pipeline-bot · 2023-06-13T13:43:09Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-06-13T14:02:42Z

EdX Release Notice: This PR has been deployed to the production environment.

Agrendalath · 2023-06-22T13:01:47Z

lms/djangoapps/courseware/tests/test_block_render.py

This has increased because we renamed get_block_for_descriptor_internal to get_block_for_descriptor.

Agrendalath · 2023-06-22T13:17:20Z

lms/djangoapps/courseware/block_render.py

I moved this code to get_block_for_descriptor. I find this change a bit controversial - allowing None values in positional arguments is inelegant.

However, I had to untangle the dependencies between get_block_for_descriptor and get_block_for_descriptor_internal whenever I read this code. The latter exists primarily because we can use it in the inner_get_block and Instructor Dashboard tasks without the request object. Unifying these two functions makes this code (subjectively) easier to understand. We won't need to jump between four functions (get_block_for_descriptor, get_block_for_descriptor_internal, prepare_runtime_for_user, inner_get_block) to understand why they are calling each other.

Agrendalath · 2023-06-22T13:21:09Z

xmodule/x_module.py

We don't need runtime._runtime_services anymore. Within a single request, each XBlock uses the same instance of the runtime, so we can safely rely on the XBlock library to check the service declaration:
https://github.com/openedx/XBlock/blob/7b8bdeafbabbbc96f11dfbceaf53c89bd416ea6f/xblock/runtime.py#L1120-L1126.

Agrendalath · 2023-06-22T13:24:11Z

lms/djangoapps/courseware/block_render.py

This. Each XBlock in a single request uses the same runtime instance, so we don't need any cache. This was why the services were overriding each other with each XBlock initialization.

Agrendalath · 2023-06-22T13:25:14Z

lms/djangoapps/courseware/block_render.py

Fixed by dc3fd30. This conversation explains why disable_staff_debug_info was added here. The render_xblock view is used in two places:

In the Learning MFE (as an iframe).

In the LTI context where Open edX is the LTI Provider.

It looks like the "staff debug info" button has been working correctly in the MFE by a pure accident - this parameter was overridden when the view initialized parent XBlocks.

Agrendalath · 2023-06-22T13:48:24Z

lms/djangoapps/courseware/block_render.py

Added because of the get_block_for_descriptor_internal -> get_block_for_descriptor merge.

Agrendalath · 2023-06-22T13:49:22Z

lms/djangoapps/courseware/block_render.py

We are not checking services because some of them are initialized by the Runtime class from the XBlock library.

Agrendalath · 2023-06-22T14:40:03Z

xmodule/services.py

I don't know why we passed the function to this service - the local import is working fine.

My guesses:

We were trying to reduce the number of direct dependencies xmodule had on lms.*.

We used to pass different methods for LMS/Studio/Preview here?

@ormsbee, neither of these. We added this in #30320. I might have forgotten to suggest changing it there.

Agrendalath · 2023-06-22T14:42:07Z

lms/djangoapps/courseware/block_render.py

We removed all XBlock-specific code from this function, so passing the runtime directly (instead of the block) is better.

Agrendalath · 2023-06-22T14:43:25Z

@0x29a, @bradenmacdonald, this is ready for review.

bradenmacdonald

👏🏻 wow, I am so happy to see this.

Again, I'll let @0x29a do the regression testing etc. I've just read through the code and I have to say, I really like it. Much cleaner and easier to understand. Hopefully more efficient and less buggy too :)

bradenmacdonald · 2023-06-23T16:45:30Z

lms/djangoapps/courseware/block_render.py

What happens here if the user is masquerading? Is user.id and/or user_service._django_user.id the "real" user ID or the masquerading user ID?

When a user is masquerading, the "real" user is under the user.real_user:
https://github.com/openedx/edx-platform/blob/c80fba689a36aa35489ded8018eb997c4ef4e8a1/lms/djangoapps/courseware/masquerade.py#L205-L244

The user service contains our masquerading target:
https://github.com/openedx/edx-platform/blob/27969b3e1e82edf35ac92a2eeae99ad15d6bf949/lms/djangoapps/courseware/block_render.py#L603-L613

Therefore, masquerading would require rebinding the runtime only if the runtime initialization happened from the "real" user (though I'm unsure if this ever happens). Once the runtime is bound to our masquerading target, the user is detected correctly, as the user service is initialized with the user object.

bradenmacdonald · 2023-06-23T16:46:28Z

lms/djangoapps/courseware/block_render.py

What is this position parameter? I'm not familiar with it.

As far as I understand, this is used by the SequenceBlock/subsection to determine which tab should be active:
https://github.com/openedx/edx-platform/blob/0b455e0336fa54199fd992c2ce83a81c07dd07c2/xmodule/seq_block.py#L335-L363

This is no longer used in the learning MFE, but the Preview still uses the old experience. I tried removing this, but it caused some errors. It should be safe to remove when we completely remove the legacy experience. For now, I would leave it as-is.

I see. Could you perhaps add that information as a comment into the function?

Sure, I added it in d7b898d. I added this information to the get_block function since it contains the description of these parameters. Let me know if you had another function in mind.

bradenmacdonald · 2023-06-23T16:59:49Z

xmodule/services.py

RebindUserService is only used in the LMS right? I almost wonder if we should move this code out of xmodule and into lms. But no change needed.

Yes, we only use it in LMS. However, we also instantiate it in the XBlockRuntime, which is in the openedx.core.djangoapps.xblock. I don't have enough context about this runtime to determine if we need it there:

https://github.com/openedx/edx-platform/blob/27969b3e1e82edf35ac92a2eeae99ad15d6bf949/openedx/core/djangoapps/xblock/runtime/runtime.py#L265-L274

That runtime is currently used for "v2" content libraries and was also used for LabXchange where learners could "learn" (LMS mode) directly from components in a content library. I don't think it's used for LabXchange anymore. So the rebind service there is probably not used at the moment but will be in the future.

0x29a

👍

Note: this error appears when deleting a block from a unit in Studio. I verified that it happens without the changes from this PR and for courses created earlier, but anyway decided to share, just in case.

2023-06-26 11:42:45,652 INFO 1058 [tracking] [user 3] [ip 172.21.0.1] logger.py:41 - {"name": "/xblock/block-v1:Test+Test+Test+type@openassessment+block@878d7bca725b405ea5ec19ec28059df6", "context": {"user_id": 3, "path": "/xblock/block-v1:Test+Test+Test+type@openassessment+block@878d7bca725b405ea5ec19ec28059df6", "course_id": "", "org_id": "", "enterprise_uuid": ""}, "username": "edx", "session": "74b02955cff3654d45e4205f713fe972", "ip": "172.21.0.1", "agent": "Mozilla/5.0 (Windows NT 10.0; rv:114.0) Gecko/20100101 Firefox/114.0", "host": "localhost:18010", "re
ferer": "http://localhost:18010/container/block-v1:Test+Test+Test+type@vertical+block@552e0a3f2484401192e53b16cda50d9a?action=new", "accept_language": "en-US,en;q=0.5", "event": "{\"GET\": {}, \"POST\": {}}", "time": "2023-06-26T11:42:45.651868+00:00", "event_type": "/xblock/block-v1:Test+Test+Test+type@openassessment+block@878d7bca725b405ea5ec19ec28059df6", "event_source": "server", "page": null}
Error calling handle_item_deleted in Signal.send_robust() (BlockKey(type='openassessment', id='878d7bca725b405ea5ec19ec28059df6'))                                                                                                                                                          Traceback (most recent call last):
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 212, in send_robust                                                                                                                                                                       response = receiver(signal=self, sender=sender, **named)
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/signals/handlers.py", line 254, in handle_item_deleted                                                                                                                                                                         deleted_block = modulestore().get_item(usage_key)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/mixed.py", line 84, in inner                                                                                                                                                                                                           retval = func(field_decorator=strip_key_collection, *args, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/mixed.py", line 250, in get_item                                                                                                                                                                                                   
    return store.get_item(usage_key, depth, **kwargs)                                                                                                                                                                                                                                         File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split_draft.py", line 283, in get_item
    return super().get_item(usage_key, depth=depth, **kwargs)                                                                                                                                                                                                                               
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 1182, in get_item                                                                                                                                                                                          items = self._load_items(course, [BlockKey.from_usage_key(usage_key)], depth, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 779, in _load_items                                                                                                                                                                                    
    return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]                                                                                                                                                                                               
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 779, in <listcomp>                                                                                                                                                                                     
    return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]                                                                                                                                                                                               
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 122, in _load_item              
    block_data = self.get_module_data(block_key, course_key)                                                                                                                                                                                                                                  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 149, in get_module_data                                                                                                                                                                raise ItemNotFoundError(block_key)                                                                                                                                                                                                                                                      xmodule.modulestore.exceptions.ItemNotFoundError: BlockKey(type='openassessment', id='878d7bca725b405ea5ec19ec28059df6')        
2023-06-26 11:42:45,661 ERROR 1058 [django.dispatch] [user 3] [ip 172.21.0.1] dispatcher.py:214 - Error calling handle_item_deleted in Signal.send_robust() (BlockKey(type='openassessment', id='878d7bca725b405ea5ec19ec28059df6'))
Traceback (most recent call last):                                                                                                                                                                                                                                                            File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 212, in send_robust                                                                                                                                                                       response = receiver(signal=self, sender=sender, **named)                                                                                                                                                                                                                                
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/signals/handlers.py", line 254, in handle_item_deleted
    deleted_block = modulestore().get_item(usage_key)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/mixed.py", line 84, in inner
    retval = func(field_decorator=strip_key_collection, *args, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/mixed.py", line 250, in get_item
    return store.get_item(usage_key, depth, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split_draft.py", line 283, in get_item
    return super().get_item(usage_key, depth=depth, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 1182, in get_item
    items = self._load_items(course, [BlockKey.from_usage_key(usage_key)], depth, **kwargs)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 779, in _load_items
    return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/split.py", line 779, in <listcomp>
    return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 122, in _load_item
    block_data = self.get_module_data(block_key, course_key)
  File "/edx/app/edxapp/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 149, in get_module_data
    raise ItemNotFoundError(block_key)
xmodule.modulestore.exceptions.ItemNotFoundError: BlockKey(type='openassessment', id='878d7bca725b405ea5ec19ec28059df6')

I did plenty of regression tests.
I read through the changes.
Includes comments where needed.

Agrendalath · 2023-06-26T13:35:54Z

@ormsbee, would you like to take a look at this (and #32357) before we schedule the merge?

ormsbee

This PR brings me so much joy. 😄 Great stuff!

ormsbee · 2023-06-27T19:18:02Z

lms/djangoapps/courseware/block_render.py

Why are these conditional imports?

@ormsbee, typing.TYPE_CHECKING (defined in PEP-484) is False at runtime. It should only be True during static analysis (usually in an IDE or type checker). Thanks to this, and the fact we don't evaluate annotations in function definitions (PEP-563), we:

Reduce the number of imports. These imports are not heavy, but we're not instantiating these classes, so we don't need them at runtime.

Avoid potential circular imports at runtime.

…ation This part was added in 3d7246e. Then, 0dd4978 refactored the approach, but did not remove these lines.

openedx-webhooks · 2023-06-30T13:29:57Z

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-06-30T14:33:42Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-06-30T14:53:38Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-06-30T15:13:51Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-06-30T15:38:43Z

2U Release Notice: This PR has been deployed to the edX production environment.

Agrendalath self-assigned this Jun 12, 2023

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core committer labels Jun 12, 2023

Agrendalath force-pushed the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch from 60b1588 to b09cbd2 Compare June 13, 2023 10:13

Agrendalath force-pushed the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch 2 times, most recently from 5c742be to 3736c15 Compare June 15, 2023 13:43

Agrendalath mentioned this pull request Jun 16, 2023

feat!: remove block handling from runtime initialization of ReplaceURLService [FC-0026] #32368

Merged

Agrendalath force-pushed the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch 4 times, most recently from c242a53 to 0e3dc9f Compare June 22, 2023 10:16

Agrendalath commented Jun 22, 2023

View reviewed changes

Agrendalath marked this pull request as ready for review June 22, 2023 14:43

Agrendalath requested a review from bradenmacdonald June 22, 2023 14:43

Agrendalath changed the title ~~feat: add request cache for runtime services and wrappers [FC-0026]~~ feat: reuse runtime services and wrappers between XBlocks [FC-0026] Jun 22, 2023

bradenmacdonald approved these changes Jun 23, 2023

View reviewed changes

0x29a approved these changes Jun 26, 2023

View reviewed changes

ormsbee approved these changes Jun 26, 2023

View reviewed changes

Agrendalath force-pushed the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch 2 times, most recently from 54ad729 to 03a661e Compare June 26, 2023 17:22

ormsbee reviewed Jun 27, 2023

View reviewed changes

refactor: reuse services and wrappers between XBlocks

36cc415

refactor: remove redundant masquerade handling from runtime initializ…

6a8e695

…ation This part was added in 3d7246e. Then, 0dd4978 refactored the approach, but did not remove these lines.

Agrendalath force-pushed the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch from 7e19462 to 6a8e695 Compare June 30, 2023 13:06

Agrendalath enabled auto-merge June 30, 2023 13:07

Agrendalath merged commit 5c424b4 into openedx:master Jun 30, 2023

Agrendalath deleted the agrendalath/fc-0026-cache_runtime_services_and_wrappers branch June 30, 2023 13:29

This was referenced Jul 13, 2023

Revert "refactor: reuse services and wrappers between XBlocks" [FC-0026] #32730

Merged

Revert "Revert "feat: remove field-data binding from the runtime [FC-0026]" (openedx#32740)" #32737

Merged

zacharis278 mentioned this pull request Jul 13, 2023

fix: removed get_block_for_descriptor_internal call openedx/xblock-lti-consumer#395

Merged

Agrendalath mentioned this pull request Jul 18, 2023

refactor: reuse services and wrappers between XBlocks (fixed) [FC-0026] #32779

Merged

feat: reuse runtime services and wrappers between XBlocks [FC-0026] #32420

feat: reuse runtime services and wrappers between XBlocks [FC-0026] #32420

Uh oh!

Conversation

Agrendalath commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other notes

Footnotes

Uh oh!

openedx-webhooks commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edx-pipeline-bot commented Jun 13, 2023

Uh oh!

edx-pipeline-bot commented Jun 13, 2023

Uh oh!

edx-pipeline-bot commented Jun 13, 2023

Uh oh!

edx-pipeline-bot commented Jun 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Jun 22, 2023

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x29a left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Jun 26, 2023

Agrendalath commented Jun 12, 2023 •

edited

Loading

openedx-webhooks commented Jun 12, 2023 •

edited

Loading

Agrendalath Jun 22, 2023 •

edited

Loading

Agrendalath Jun 22, 2023 •

edited

Loading

Agrendalath Jun 23, 2023 •

edited

Loading

Agrendalath Jun 23, 2023 •

edited

Loading