[1/6] refactor: define resource paths (not contents) on XModule classes #32286

kdmccormick · 2023-05-23T14:53:06Z

Supporting information

This is a part of a series of PRs:

Stop dynamically generating XModule SCSS #32292

The previous series of PR is:

Decouple XModule styles from LMS/Studio styles (attempt 3) #32237

The next PR is:

[2/6] build: remove prefixes from XModule resource copies #32287

Description

For the XBlocks types that use legacy XModule-style assets (specifically, those that
inherit from HTMLSnippet), this is small refactor that brings them a bit closer to being like
standard XBlocks.

Given these class attributes:

class SomeXModuleLikeBlock(..., HTMLSnippet, ...):
    ...
    studio_view_css = { ... }
    preview_view_css = { ... }
    studio_view_js = { ... }
    preview_view_js = { ... }
    ...

we make it so their values are paths to the resources
rather than the actual content of the resources.
This is a no-op change, but it'll enable future XModule
asset refactorings which require us to operate on asset
paths rather than contents.

Other information

Reviewers: see #32286 (review)

Testing Instructions

Build the Tutor openedx image with this branch
Browse the demo course in the Studio outline editor
Browse the demo course in Learning MFE

Deadline

Medium-high urgency, as this is in the critical path to a long line of DevX improvement PRs.

For the XBlocks types that use legacy XModule-style assets, this is small refactor that brings them a bit closer to being like XBlocks. Given class attributes on those block types in the form: SomeXModuleLikeBlock.(studio|preview)_view_(js|css)['(js|scss|css|xmodule_js)'] we make it so their value is the *path to the resource* rather than *the actual content of the resource*. This will make future refactorings simpler. Part of: openedx#31624

kdmccormick

Notes for reviewers 👀

kdmccormick · 2023-05-23T14:56:29Z

xmodule/annotatable_block.py

-        'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
+        'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),


resource_string loaded the contents of the resource. We change it to resource_filename, which just returns the absolute path to the resource.

We make this change uniformly across all XModule-like XBlocks.

Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the setuptools community what the timeline for this deprecation is so we can plan accordingly.

kdmccormick · 2023-05-23T15:01:03Z

xmodule/static_content.py

-            for idx, fragment in enumerate(class_css.get(filetype, [])):
+            for idx, fragment_path in enumerate(class_css.get(filetype, [])):
+                with open(fragment_path, 'rb') as fragment_file:
+                    fragment = fragment_file.read()


When processing the SCSS, we now must load the files rather than just looking at the contents.

This creates a very minor build-time inefficiency: any SCSS file that is used across multiple XModules will be loaded multiple times. On the other hand, it achieves a minor run-time optimization: these SCSS files are no longer loaded along with XModule classes.

The inefficiency will be gone soon, because follow-up refactorings aim to delete this processing step entirely.

kdmccormick · 2023-05-23T15:01:49Z

xmodule/static_content.py

+        with open(module_js.get('xmodule_js'), 'rb') as xmodule_js_file:
+            xmodule_js_fragment = xmodule_js_file.read()
        # It will enforce 000 prefix for xmodule.js.
-        fragment_owners[(0, 'js', module_js.get('xmodule_js'))].append(getattr(class_, js_attribute + '_bundle_name')())
+        fragment_owners[(0, 'js', xmodule_js_fragment)].append(getattr(class_, js_attribute + '_bundle_name')())
        for filetype in ('coffee', 'js'):
-            for idx, fragment in enumerate(module_js.get(filetype, [])):
+            for idx, fragment_path in enumerate(module_js.get(filetype, [])):
+                with open(fragment_path, 'rb') as fragment_file:
+                    fragment = fragment_file.read()


(this is just the JS version of the SCSS change above; same considerations)

feanil

Seems reasonable to me, I tested locally by bringing up tutor and looking at the video block and seeing it rendered correctly in studio and the LMS.

feanil · 2023-05-24T14:50:31Z

xmodule/annotatable_block.py

-        'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
+        'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),


Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the setuptools community what the timeline for this deprecation is so we can plan accordingly.

kdmccormick · 2023-05-24T15:24:49Z

Non-blocking note: It looks like pkg_resources is deprecated and we should be using importlib.resources.path(package, resource). But annoyingly even that new way will be deprecated in Python 3.11 and replaced with a different interface.

It may be fine to leave as is for now, I've asked the pypa/setuptools#3934 community what the timeline for this deprecation is so we can plan accordingly.

@feanil Good point, and thanks for opening that issue.

The direction that I hope we go for edx-platform XBlocks is to stop using pkg_resources and instead use the standard XBlock resource-loading framework (SharedBlockBase.open_local_resource, et al). Unfortunately, the XBlock framework itself uses pkg_resources. So:

open an issue in edx-platform to switch us over to standard XBlock resource management
open an issue in XBlock to switch over to importlib and/or the Python 3.11 replacement

kdmccormick · 2023-05-24T15:26:09Z

Sorry, I wasn't trying to ask you to open those issues, I meant to write:

So, I will:

open ...`

kdmccormick · 2023-05-25T16:53:49Z

Okay, it's a little more complicated than I thought. XBlock does not provide a resource abstraction, but xblock-utils does. If everyone just used xblock-utils' abstraction, then switching off of pkg_resources would be easy. Issues:

edx-pipeline-bot · 2023-05-25T18:43:10Z

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

edx-pipeline-bot · 2023-05-25T19:03:59Z

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

kdmccormick commented May 23, 2023

View reviewed changes

This was referenced May 23, 2023

[2/6] build: remove prefixes from XModule resource copies #32287

Merged

Remove special XModule asset handling #31624

Closed

kdmccormick changed the title ~~refactor: define resource paths (not contents) on XModule classes~~ [1] refactor: define resource paths (not contents) on XModule classes May 23, 2023

kdmccormick changed the title ~~[1] refactor: define resource paths (not contents) on XModule classes~~ [1/6] refactor: define resource paths (not contents) on XModule classes May 23, 2023

This was referenced May 23, 2023

Radically simplify XModule SCSS #32283

Closed

Stop dynamically generating XModule SCSS #32292

Closed

feanil approved these changes May 24, 2023

View reviewed changes

kdmccormick merged commit 0f847df into openedx:master May 25, 2023

kdmccormick deleted the kdmccormick/xmodule-resource-filename branch May 25, 2023 22:55

kenclary mentioned this pull request Jun 29, 2023

feat: basic get/post endpoint for v2 xblocks. TNL-10873 #32600

Merged

kenclary mentioned this pull request Aug 4, 2023

fix: fix mako runtime bug with studio_view. TNL-10912 #32912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/6] refactor: define resource paths (not contents) on XModule classes #32286

[1/6] refactor: define resource paths (not contents) on XModule classes #32286

kdmccormick commented May 23, 2023 •

edited

Loading

kdmccormick left a comment

kdmccormick May 23, 2023

feanil May 24, 2023

kdmccormick May 23, 2023 •

edited

Loading

kdmccormick May 23, 2023

feanil left a comment

feanil May 24, 2023

kdmccormick commented May 24, 2023

kdmccormick commented May 24, 2023

kdmccormick commented May 25, 2023

edx-pipeline-bot commented May 25, 2023

edx-pipeline-bot commented May 25, 2023

		'xmodule_js': resource_string(__name__, 'js/src/xmodule.js'),
		'xmodule_js': resource_filename(__name__, 'js/src/xmodule.js'),

[1/6] refactor: define resource paths (not contents) on XModule classes #32286

[1/6] refactor: define resource paths (not contents) on XModule classes #32286

Conversation

kdmccormick commented May 23, 2023 • edited Loading

Supporting information

Description

Other information

Testing Instructions

Deadline

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick May 23, 2023

Choose a reason for hiding this comment

feanil May 24, 2023

Choose a reason for hiding this comment

kdmccormick May 23, 2023 • edited Loading

Choose a reason for hiding this comment

kdmccormick May 23, 2023

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

feanil May 24, 2023

Choose a reason for hiding this comment

kdmccormick commented May 24, 2023

kdmccormick commented May 24, 2023

kdmccormick commented May 25, 2023

edx-pipeline-bot commented May 25, 2023

edx-pipeline-bot commented May 25, 2023

kdmccormick commented May 23, 2023 •

edited

Loading

kdmccormick May 23, 2023 •

edited

Loading