Expand mypy static type checking #32591

bradenmacdonald · 2023-06-27T22:45:06Z

Description

The edx-platform codebase has mypy set up, but only a single django app (learning_sequences) is currently type checked.

This PR expands the type checking to include several django apps that I've developed:

openedx/core/djangoapps/content_staging
openedx/core/djangoapps/content_libraries
openedx/core/djangoapps/xblock

The type checking found a couple actual bugs in the code, which I have now fixed. It also required a bunch of additional tweaks here are there to get the type checks passing.

Testing instructions

MyPy tests should be run by CI, so just confirm that they ran.

Deadline

None

Other information

Private ref MNG-3784

openedx-webhooks · 2023-06-27T22:45:10Z

Thanks for the pull request, @bradenmacdonald!

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.

bradenmacdonald · 2023-06-27T23:01:41Z

openedx/core/djangoapps/content_libraries/api.py

-    ADMIN_LEVEL = ContentLibraryPermission.ADMIN_LEVEL
-    AUTHOR_LEVEL = ContentLibraryPermission.AUTHOR_LEVEL
-    READ_LEVEL = ContentLibraryPermission.READ_LEVEL
-    NO_ACCESS = None


Turns out this was defined twice in this file. The pylint warning was suppressed by an automatic lint amnesty line.

bradenmacdonald · 2023-06-27T23:07:20Z

openedx/core/djangoapps/content/learning_sequences/data.py

@@ -213,7 +213,7 @@ def not_deprecated(self, _attribute, value):
    course_visibility: CourseVisibility = attr.ib(validator=attr.validators.in_(CourseVisibility))

    # Entrance Exam ID
-    entrance_exam_id = attr.ib(type=str)
+    entrance_exam_id = attr.ib(type=Optional[str])


It was unclear if this should be changed to allow None or if None values should be converted to "" when stored in this field, but there were clear examples if it being explicitly set to None in the tests, so I went with this.

edx-platform/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py

Line 55 in 478c301

entrance_exam_id=None,

bradenmacdonald · 2023-06-27T23:08:32Z

openedx/core/djangoapps/content/learning_sequences/views.py

@@ -201,7 +201,7 @@ def _determine_user(self, request, course_key: CourseKey) -> types.User:
            # Just like in masquerading, set real_user so that the
            # SafeSessions middleware can see that the user didn't
            # change unexpectedly.
-            target_user.real_user = request.user
+            target_user.real_user = request.user  # type: ignore


Until python typing supports intersections there is no way to tell the type system that we want to add a real_user attribute onto the User class, so I just ignored this line.

bradenmacdonald · 2023-06-27T23:09:29Z

openedx/core/djangoapps/content_staging/api.py

@@ -88,7 +88,7 @@ def get_staged_content_static_files(staged_content_id: int) -> list[StagedConten
    sc = _StagedContent.objects.get(pk=staged_content_id)

    def str_to_key(source_key_str: str):
-        if not str:
+        if not source_key_str:


This was a clear bug found by the type check. 🐛 str here was the global string type :/

bradenmacdonald · 2023-06-27T23:10:00Z

openedx/core/djangoapps/content_staging/filters.py

@@ -49,7 +49,7 @@ def remove_large_data(fd: StagedContentFileData):
            """ Remove 'data' from the immutable StagedContentFileData object, if it's above the size limit """
            if fd.data and len(fd.data) > limit:
                # these data objects are immutable so make a copy with data=None:
-                return StagedContentFileData(**{**asdict(fd), "data": None})
+                return StagedContentFileData(**{**asdict(fd), "data": None})  # type: ignore


Not sure why it didn't like this line, but I'm planning to remove this filter implementation soon anyways. So just ignored it for now.

bradenmacdonald · 2023-06-27T23:12:08Z

openedx/core/djangoapps/content_staging/views.py

@@ -182,16 +182,16 @@ def _save_static_assets_to_clipboard(
        Helper method for "post to clipboard" API endpoint. This deals with copying static files into the clipboard.
        """
        files_to_save: list[StagedContentFileData] = []
-        for f in static_files:
+        for sf in static_files:


I had to rename this variable because the f used as an iterator in this for loop is considered the same as the f used as the iterator in the second for loop in this function. I don't like how python does scoping.

Agrendalath

👍

I tested this: checked that the tests are passing
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

openedx/core/djangoapps/content_staging/models.py

openedx/core/djangoapps/xblock/runtime/runtime.py

Agrendalath · 2023-07-06T16:25:03Z

@bradenmacdonald, I left a few suggestions, but none are blockers. Thanks for implementing this - I really like that we're adding more typing to Open edX!

Btw. did you think about using pyright? We're running mypy only on a few packages in Open edX, so it could be nice to investigate it as an alternative. After doing this review, I ran it, and it detects more errors than mypy. E.g., it detects that this code can potentially raise AttributeErrors when user is None.

pyright openedx/core/djangoapps/xblock/runtime/runtime.py
/edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:113:38 - error: Cannot access member "id" for type "User"
    Member "id" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:143:49 - error: Cannot access member "get_site_root_url" for type "AppConfig"
    Member "get_site_root_url" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:177:13 - error: Argument of type "int | str" cannot be assigned to parameter "__value" of type "str" in funct
ion "__setitem__"
    Type "int | str" cannot be assigned to type "str"
      "int" is incompatible with "str" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:178:9 - error: Argument of type "dict[Any, Any]" cannot be assigned to parameter "__value" of type "str" in f
unction "__setitem__"
    "dict[Any, Any]" is incompatible with "str" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:187:26 - error: "is_anonymous" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:244:21 - error: Argument of type "None" cannot be assigned to parameter "__value" of type "FieldData" in func
tion "__setitem__"
    Type "None" cannot be assigned to type "FieldData" (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:250:26 - error: "is_anonymous" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:259:41 - error: "is_staff" is not a known member of "None" (reportOptionalMemberAccess)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:366:64 - error: Cannot access member "get_site_root_url" for type "AppConfig"
    Member "get_site_root_url" is unknown (reportGeneralTypeIssues)
  /edx/app/edxapp/edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py:436:16 - error: Object of type "XBlockRuntime" is not callable (reportGeneralTypeIssues)
10 errors, 0 warnings, 0 informations

bradenmacdonald · 2023-07-06T21:45:46Z

@Agrendalath I thought about pyright and I tried it out, because it seems nicer and faster. But it doesn't have a plugin framework, so it doesn't support attrs which is important for most of these modules in edx-platform. Didn't you run into this issue? When I tried pyright, it gave a ton of errors related to the attrs data classes.

Agrendalath · 2023-07-07T16:37:50Z

@bradenmacdonald, good point - I didn't think about attrs. According to the attrs docs, we could use @attr.define instead of @attr.s, and then use explicit type annotations. Given that we have only 6 import attrs usages (excluding tests), it could be possible to change it.

Actually, the speed of both mypy and pyright seems to be comparable, but pyright is much more sensitive. I ran both on the whole edx-platform repo

mypy -> time: 17.051; Found 460 errors in 207 files (checked 2469 source files)
pyright --skipunannotated -> time: 17.654s; 789 errors, 33 warnings, 11784 informations
pyright -> time: 33.477s; 8198 errors, 37 warnings, 0 informations

For mypy, I only changed the following in the mypy.ini:

files =
    lms,
    openedx,
    cms,
    xmodule

For pyright, I created the following pyproject.toml:

[tool.pyright]
exclude = ['**/migrations', '**/__pycache__', '**/tests']

bradenmacdonald · 2023-07-07T17:00:08Z

@Agrendalath Thanks, that seems doable. Another issue is Django though - while mypy has a plugin that works automatically together with django-stubs, for pyright you still have to add a lot of manual type annotations. https://github.com/sbdchd/django-types https://news.ycombinator.com/item?id=34223647 . Presumably same for DRF.

I suspect that many of the issues pyright is flagging are due to it not understanding django as fully?

Agrendalath · 2023-07-07T19:03:43Z

@bradenmacdonald, you're right - mypy with plugins works much better with Django. pyright has more false positives, and using django-types with it requires plenty of hacks to work properly.

We could try using mypy with check_untyped_defs = true to catch more errors (though still less than pyright).

openedx-webhooks · 2023-07-19T16:58:24Z

@bradenmacdonald 🎉 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-07-19T17:56:27Z

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

edx-pipeline-bot · 2023-07-19T18:17:52Z

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

edx-pipeline-bot · 2023-07-19T18:17:52Z

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

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

bradenmacdonald force-pushed the braden/mypy-checks branch from 63f73da to fb1c5f6 Compare June 27, 2023 22:58

bradenmacdonald commented Jun 27, 2023

View reviewed changes

bradenmacdonald force-pushed the braden/mypy-checks branch from fb1c5f6 to c1a889f Compare June 27, 2023 23:05

bradenmacdonald commented Jun 27, 2023

View reviewed changes

bradenmacdonald force-pushed the braden/mypy-checks branch from 80bc094 to 576f4d4 Compare June 27, 2023 23:24

bradenmacdonald force-pushed the braden/mypy-checks branch 2 times, most recently from 3761bf2 to 86cb4dd Compare July 5, 2023 20:19

Agrendalath approved these changes Jul 6, 2023

View reviewed changes

bradenmacdonald added 5 commits July 18, 2023 11:58

chore: typing + fixes for content_staging

55c9e47

chore: typing + fixes for learning_sequences

d33a145

chore: typing + fixes for content_libraries

30b54c0

chore: typing + fixes for new XBlock runtime

3555e28

feat: type hinting more code with mypy

f485794

bradenmacdonald force-pushed the braden/mypy-checks branch from 86cb4dd to f485794 Compare July 18, 2023 19:20

bradenmacdonald merged commit 57420ed into openedx:master Jul 19, 2023
42 checks passed

bradenmacdonald deleted the braden/mypy-checks branch July 19, 2023 16:58

bradenmacdonald mentioned this pull request Jul 27, 2023

feat: Add type hints openedx/opaque-keys#256

Merged

1 task

Agrendalath mentioned this pull request Aug 3, 2023

feat: create cea for invite only courses before checkout openedx/edx-enterprise#1813

Draft

9 tasks

kenclary mentioned this pull request Aug 4, 2023

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

Merged

bradenmacdonald mentioned this pull request Aug 25, 2023

fix: incorrect type hints in a few places #33104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand mypy static type checking #32591

Expand mypy static type checking #32591

bradenmacdonald commented Jun 27, 2023 •

edited

Loading

openedx-webhooks commented Jun 27, 2023

bradenmacdonald Jun 27, 2023

bradenmacdonald Jun 27, 2023

bradenmacdonald Jun 27, 2023

bradenmacdonald Jun 27, 2023

bradenmacdonald Jun 27, 2023

bradenmacdonald Jun 27, 2023

Agrendalath left a comment

Agrendalath commented Jul 6, 2023

bradenmacdonald commented Jul 6, 2023

Agrendalath commented Jul 7, 2023

bradenmacdonald commented Jul 7, 2023

Agrendalath commented Jul 7, 2023

openedx-webhooks commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

Expand mypy static type checking #32591

Expand mypy static type checking #32591

Conversation

bradenmacdonald commented Jun 27, 2023 • edited Loading

Description

Testing instructions

Deadline

Other information

openedx-webhooks commented Jun 27, 2023

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

bradenmacdonald Jun 27, 2023

Choose a reason for hiding this comment

Agrendalath left a comment

Choose a reason for hiding this comment

Agrendalath commented Jul 6, 2023

bradenmacdonald commented Jul 6, 2023

Agrendalath commented Jul 7, 2023

bradenmacdonald commented Jul 7, 2023

Agrendalath commented Jul 7, 2023

openedx-webhooks commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

edx-pipeline-bot commented Jul 19, 2023

bradenmacdonald commented Jun 27, 2023 •

edited

Loading