Skip to content
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

Drop py3.8 support | Replace pkg_resources lib with importlib.resources #716

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Feb 6, 2024

  • Drop Py3.8 support
  • Replace pkg_resources lib with importlib.resources

Ticket: #676

Followed migration guide:
https://importlib-resources.readthedocs.io/en/latest/migration.html

Testing:
I have tested the changes with following steps:

  1. Use PR of xblock-sdk

  2. Install xblock into xblock-sdk

  3. Install feedback-xblock from PR into xblock-sdk

  4. Test feedback xblock functionality

@farhan farhan linked an issue Feb 10, 2024 that may be closed by this pull request
@farhan farhan force-pushed the farhan/replace_pkg_resources branch 2 times, most recently from cbf6745 to 79817df Compare February 12, 2024 13:26
@farhan farhan marked this pull request as ready for review February 13, 2024 05:03
@kdmccormick
Copy link
Member

kdmccormick commented Feb 13, 2024

@farhan per the conversation on #676, could we convert this to a draft and pause it until after the Python 3.11/3.12 upgrade? That would allow us to avoid adding importlib_resources as a dependency, because the new importlib.resources API becomes part of the standard library in 3.11.

@farhan farhan marked this pull request as draft February 14, 2024 08:20
@kdmccormick kdmccormick removed their request for review May 8, 2024 13:13
@farhan farhan force-pushed the farhan/replace_pkg_resources branch 5 times, most recently from 6a467a8 to 7d95fd3 Compare May 20, 2024 12:42
@farhan farhan closed this May 20, 2024
@farhan farhan reopened this May 20, 2024
@farhan farhan force-pushed the farhan/replace_pkg_resources branch 5 times, most recently from 044638d to ec37198 Compare May 20, 2024 15:42
@farhan farhan marked this pull request as ready for review May 22, 2024 06:01
@farhan farhan force-pushed the farhan/replace_pkg_resources branch from fa5df5d to 5f63a7d Compare May 23, 2024 07:34
@farhan farhan requested review from kdmccormick, feanil, irtazaakram and a team May 23, 2024 07:38
@farhan farhan force-pushed the farhan/replace_pkg_resources branch 3 times, most recently from ad54839 to b29b815 Compare May 30, 2024 07:11
@feanil
Copy link
Contributor

feanil commented Jun 7, 2024

This looks good, but we're currently blocked on merging and releasing this because we have not yet dropped 3.8 support in edx-platform. So let's hold off on merging for now.

@farhan
Copy link
Contributor Author

farhan commented Jun 10, 2024

PR is blocked as we are waiting for drop of Python v3.8 support from edx-platform

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Just a couple change requests. Glad to hear you tested this with xblock-sdk. I'm going to test this with edx-platform today or tomorrow, I'll let you know if I run into any issues.

docs/xblock-tutorial/getting_started/prereqs.rst Outdated Show resolved Hide resolved
xblock/core.py Outdated Show resolved Hide resolved
@farhan farhan force-pushed the farhan/replace_pkg_resources branch from d980494 to d55a025 Compare June 20, 2024 08:23
@farhan
Copy link
Contributor Author

farhan commented Jun 20, 2024

@kdmccormick
Requested changes addressed.
Do share the results of testing on edx-platform

@kdmccormick
Copy link
Member

I tried this branch of XBlock in Tutor Nightly with the latest edx-platform master and the lti_consumer advanced component, and got an error when rendering the block (both in LMS and CMS):

cms-1  | 2024-06-20 16:57:35,863 WARNING 97 [cms.djangoapps.contentstore.views.preview] [user 4] [ip 172.24.0.1] preview.py:349 - Unable to render author_view for <VerticalBlockWithMixins @4C72 name=None, parent=BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'sequential', '725212bbf01049549da23d4ceee10065'), tags=[], copied_from_block=None, display_name='Unit', course_edit_method='Studio', days_early_for_beta=None, due=None, edxnotes=False, edxnotes_visibility=True, giturl=None, graceperiod=None, graded=False, group_access={}, hide_from_toc=False, in_entrance_exam=False, matlab_api_key=None, max_attempts=None, relative_weeks_due=None, rerandomize='never', self_paced=False, show_correctness='always', show_reset_button=False, showanswer='finished', start=datetime.datetime(2030, 1, 1, 0, 0, tzinfo=tzlocal()), static_asset_path='', use_latex_compiler=False, user_partitions=[], video_auto_advance=False, video_bumper={}, video_speed_optimizations=True, visible_to_staff_only=False, xqa_key=None, chrome=None, default_tab=None, format=None, source_file=None, children=[BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'html', '5bd3ee44d9c146c3885f2a0fba77a6b5'), BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'problem', 'problem2'), BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'problem', '87692450f28041c2935d5bdef1b441f6'), BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'html', '77655940986a45ee866cd506b36af1c8'), BlockUsageLocator(CourseLocator('Kyle', '1', '1', None, None), 'lti_consumer', '63872f432944418c8c9fa49997453336')], discussion_enabled=True, hide_after_due=False, is_entrance_exam=False, position=None, xml_attributes={'filename': ['vertical/ab86ca95a1764838a6f11578005df673.xml', 'vertical/ab86ca95a1764838a6f11578005df673.xml']}>
cms-1  | Traceback (most recent call last):
cms-1  |   File "/openedx/edx-platform/cms/djangoapps/contentstore/views/preview.py", line 347, in get_preview_fragment
cms-1  |     fragment = block.render(preview_view, context)
cms-1  |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/core.py", line 815, in render
cms-1  |     return self.runtime.render(self, view, context)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/edx-platform/xmodule/x_module.py", line 994, in render
cms-1  |     return super().render(block, view_name, context=context)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/runtime.py", line 823, in render
cms-1  |     frag = view_fn(context)
cms-1  |            ^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/edx-platform/xmodule/vertical_block.py", line 223, in author_view
cms-1  |     self.render_children(context, fragment, can_reorder=True, can_add=True)
cms-1  |   File "/openedx/edx-platform/xmodule/studio_editable.py", line 28, in render_children
cms-1  |     rendered_child = child.render(StudioEditableBlock.get_preview_view_name(child), context)
cms-1  |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/core.py", line 815, in render
cms-1  |     return self.runtime.render(self, view, context)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/edx-platform/xmodule/x_module.py", line 994, in render
cms-1  |     return super().render(block, view_name, context=context)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/runtime.py", line 823, in render
cms-1  |     frag = view_fn(context)
cms-1  |            ^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/venv/lib/python3.11/site-packages/lti_consumer/lti_xblock.py", line 1168, in author_view
cms-1  |     return self.student_view(context)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/venv/lib/python3.11/site-packages/lti_consumer/lti_xblock.py", line 1219, in student_view
cms-1  |     fragment.add_content(loader.render_mako_template('/templates/html/student.html', context))
cms-1  |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/utils/resources.py", line 58, in render_mako_template
cms-1  |     template_str = self.load_unicode(template_path)
cms-1  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/mnt/xblock/xblock/utils/resources.py", line 25, in load_unicode
cms-1  |     return importlib.resources.files(package_name).joinpath(resource_path).read_text()
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/opt/pyenv/versions/3.11.8/lib/python3.11/pathlib.py", line 1058, in read_text
cms-1  |     with self.open(mode='r', encoding=encoding, errors=errors) as f:
cms-1  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/opt/pyenv/versions/3.11.8/lib/python3.11/pathlib.py", line 1044, in open
cms-1  |     return io.open(self, mode, buffering, encoding, errors, newline)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  | FileNotFoundError: [Errno 2] No such file or directory: '/templates/html/student.html'

The resource is added to the LTI block on this line.

@farhan farhan force-pushed the farhan/replace_pkg_resources branch from d55a025 to 20de95c Compare June 24, 2024 08:11
@farhan
Copy link
Contributor Author

farhan commented Jun 24, 2024

@kdmccormick Thanks for the testing of this important PR.

I have fixed the issues in this comment and tested on edx-platform

PR is ready for next pass.

Can you share some example course having different XBlocks on the latest edx-platform?

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments, but this is getting close. The Python packaging APIs drive me nuts... I appreciate you guys digging into this.

xblock/plugin.py Outdated Show resolved Hide resolved
xblock/core.py Outdated Show resolved Hide resolved
xblock/utils/resources.py Outdated Show resolved Hide resolved
xblock/utils/resources.py Outdated Show resolved Hide resolved
xblock/utils/resources.py Outdated Show resolved Hide resolved
xblock/utils/resources.py Outdated Show resolved Hide resolved
@farhan
Copy link
Contributor Author

farhan commented Jul 19, 2024

@kdmccormick Thanks for such a quality review. Let me spend more time on it in depth

@farhan farhan force-pushed the farhan/replace_pkg_resources branch 2 times, most recently from 44698d0 to 20149cf Compare July 19, 2024 15:39
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small comments, but if you like these, then this looks good to me! Thanks for your thorough work here.

I tested with the ProblemBlock, drag-and-drop-v2, and lti_consumer blocks. (Since you asked, the best course I know of for testing different XBlocks is the openedx-test-course. It's not great, but it's better than nothing :)

xblock/plugin.py Outdated Show resolved Hide resolved
xblock/utils/resources.py Outdated Show resolved Hide resolved
@farhan farhan force-pushed the farhan/replace_pkg_resources branch from 05cee82 to 08627b0 Compare July 21, 2024 09:13
@farhan farhan force-pushed the farhan/replace_pkg_resources branch from 08627b0 to 8885b94 Compare July 25, 2024 06:42
@farhan farhan merged commit a63390b into master Jul 25, 2024
9 of 10 checks passed
@farhan farhan deleted the farhan/replace_pkg_resources branch July 25, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for Python 3.8 Switch off of deprecated pkg_resources library
3 participants