feat: run mypy as part of testing the codebase #27575

regisb · 2021-05-10T09:40:04Z

Description

The edx-platform codebase already includes quite a few type annotations, but
they were not regularly checked. This may cause problems, when the annotations
themselves include errors (as we found out in some of the learning_sequences
annotations). So here, we add mypy as a dev requirement and introduce a make
command to run mypy regularly. Mypy runs on a very small portion of the total
edx-platform, as configured in mypy.ini. Our hope is that developers will add
more and more modules to this configuration file, until we can eventually run
mypy on the full code base.

Supporting information

See discussion: https://discuss.openedx.org/t/dev-notes-running-mypy-on-edx-platform/4860

Testing instructions

Run the following:

pip install mypy
make test-types

Deadline

None.

Other information

This PR does not add type-testing to Github pull requests because I could not find the proper CI file. Can someone point me in the right direction?

cc @kdmccormick @ormsbee @jmbowman

openedx-webhooks · 2021-05-10T09:40:09Z

Thanks for the pull request, @regisb! I've created OSPR-5772 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

ormsbee

Thanks @regisb! A couple of questions, as I'm still really unfamiliar with this stuff.

ormsbee · 2021-05-10T13:24:17Z

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


    def num_sequences(self, course_context):
        return self._publish_report_attr(course_context, 'num_sequences')
-    num_sequences.short_description = "Sequences"
+    setattr(num_sequences, "short_description", "Sequences")


Question: How did the admin.py bits here get wrapped up in mypy related annotation changes? Nothing here is annotated, is it?

Well, mypy runs here on the entire learning_sequences/ directory, including this admin.py module. Without these changes, mypy reports the following errors:

openedx/core/djangoapps/content/learning_sequences/admin.py:87: error: "Callable[[CourseSectionSequenceInline, Any], Any]" has no attribute "short_description"

@ormsbee When you run mypy on a module or directory, it's not just checking that all the annotations are correct--it's stricter than that. mypy is checking that your code can be deduced as properly typed given (1) what it can infer about your code on its own and (2) what it is being told by that annotations that you add. Code without any annotations will generally fail type checking, since mypy can only infer so much without help.

Specifically, these attributes-assigned-on-functions seem like one of those instances where the dynamicism of Python and Django butts up against the formality of type checking. I assume @regisb made this change because mypy was saying something to the effect of "I have no reason to believe that num_sequences, which is a function, should have an attribute short_description." Using setattr tends to fix errors like that, since mypy throws its hands up in the air when it sees dynamic attribute assignment/retrieval and just assumes you know what you're doing.

If we don't like the setattr solution, a couple alternative fixes could be throwing # type: ignore on these (I think that would work, but I haven't tried) or using a type decorator, as others in this same situation have. I encourage us to think carefully about this, since whatever we go with, we're setting a pattern for every admin.py in the repository to follow :)

Merde. setattr(num_sequences, "key", "value") triggers a pylint warning... https://build.testeng.edx.org/job/edx-platform-quality-pipeline-pr/28019/pylint/new/folder.1177700281/source.4b0778cb-34d0-42ff-8a47-99b2d29d2194/#87

FWIW, I have no strong feelings on ignoring pylint vs. ignoring mypy for this section of code, and I'd be fine with rearranging so all of these are grouped at the end of the class definition if it makes it easier.

Actually, I guess given that we have to ignore one or the other, I have a mild preference for ignoring mypy for these, since that would bring us to the idiomatic usage people would see in Django docs. But it's a mild preference. :-P

@regisb could you either follow the ActionWithAttributes example (could go in openedx/core/types.py so that other apps could use it) or just mypy-ignore these lines?

The setattr approach feels to me like an instance of making the code clunkier in order to make the tooling happy.

I went with another approach, based on Protocol, referenced in this same thread. Let me know what you think.

@regisb Your approach looks great. Nice and straightforward, but also enforces the types of the method attributes, which is cool. I think the class and decorator should live in openedx/core/types.py so that other apps' admin pages can use it; do you agree?

Fwiw, Django 3.2 will introduce a new way of customizing admin columns:

@admin.display(short_description="Record Created") def created(self): return ...

which probably won't bother mypy. So, we could drop your workaround in the medium-term future if we wanted to.

This is good to know. Thus I decided to replace admin_method with a future-proof admin_display decorator. I moved it to a admin.py file in the types package.

ormsbee · 2021-05-10T13:26:20Z

openedx/core/djangoapps/content/learning_sequences/api/outlines.py

@@ -255,7 +256,7 @@ def get_content_errors(course_key: CourseKey) -> List[ContentErrorData]:

 @function_trace('learning_sequences.api.get_user_course_outline')
 def get_user_course_outline(course_key: CourseKey,
-                            user: User,
+                            user: Model,


If Django magic is making it so that at we can't put User here, can we use a plugin like django-stubs to make mypy more Django-aware instead?

I don't believe so. That's because a static-check time, we have absolutely no idea what is the actual interface of the "User" class, which could be pretty much anything. All we know is that it's a Django model, as reported by the signature of the get_user_model function.

@regisb My experience with the django-stubs mypy plugin is that it handles the Django-model-magic surprisingly well, allowing for model-specific type annotations. I recommend using it--we will only get so far with mypy in edx-platform if it doesn't understand models and model fields.

Whoops, @regisb I realize now you're talking specifically about the limitations of get_user_model. Yeah, you're right, that's a special weird case.

@ormsbee For such an edx-platform specific app like learning_sequences, would we lose much by just directly using django.contrib.auth.models.User instead of get_user_model?

Can I suggest that we create a types.py somewhere in a common module? This types.py module would include things like:

import django.contrib.auth.models.User User = django.contrib.auth.models.User

Then developers would write:

from openedx.core import types def my_func(user: types.User): ...

Can we hardcode it to from django.contrib.auth import get_user_model for this module only for now? I've honestly been contemplating removing the User object from this anyhow (and replacing it with another attrs class with fewer attributes).

@ormsbee

Can we hardcode it to from django.contrib.auth import get_user_model for this module only for now?

Can you elaborate? Using get_user_model in learning_sequences would require us to annotate the user objects as type Model -- do you want to do that?

kdmccormick · 2021-05-10T14:18:34Z

@regisb

This PR does not add type-testing to Github pull requests because I could not find the proper CI file. Can someone point me in the right direction?

Right, so the edx-platform CI suite is currently run on a Jenkins instance using Jenkins job DSL defined here. This has allowed us to tweak and optimize our existing test suite so that it runs in a reasonable amount of time, but that approach is more complex and quite specific to edX.org's infrastructure, so I think we want to avoid putting new checks there unless we absolutely need to.

Instead, I recommend writing a GitHub Action to run the check, much like course-discovery does for its tests and quality checks. It should be simpler to do, and also would make the action more accessible to fork maintainers. This would be the first GHA to be merged into edx-platform.

@jmbowman , please correct me if you disagree with any of this ^

regisb · 2021-05-10T14:39:04Z

I updated my PR with a new proposal, which includes a dedicated types.py module with shared type definitions.

I propose that we first agree on the right way to integrate mypy, and then, if we do agree, I'll update this PR with a GitHub Actions manifest -- I expect it's going to take some effort to produce a working template.

natabene · 2021-05-11T15:58:45Z

@regisb Thank you for your contribution.

kdmccormick · 2021-05-20T18:03:57Z

openedx/core/types.py

@@ -0,0 +1,3 @@
+import django.contrib.auth.models


Cool! Works for me.

Name suggestion: openedx/core/typing.py, to parallel the built-in typing.py module. It would also imply a bit more strongly that this module is specifically for typing utilities, and not just any old class.

On the other hand... this wouldn't just be for type checking, as we'd expect from openedx.core.types import User to become the standard way of referencing the User model, even for apps that aren't using mypy yet.

Eh, whichever you prefer is fine by me.

Makefile

natabene · 2021-05-26T13:34:48Z

@regisb Is this good for another review?

regisb · 2021-05-27T09:27:11Z

@natabene Yes, this is ready for review. I would have to squash the commits before merging, though.

The edx-platform codebase already includes quite a few type annotations, but they were not regularly checked. This may cause problems, when the annotations themselves include errors (as we found out in some of the learning_sequences annotations). So here, we add mypy as a dev requirement and introduce a make command to run mypy regularly. Mypy runs on a very small portion of the total edx-platform, as configured in mypy.ini. Our hope is that developers will add more and more modules to this configuration file, until we can eventually run mypy on the full code base. See discussion: https://discuss.openedx.org/t/dev-notes-running-mypy-on-edx-platform/4860

edx-status-bot · 2021-05-27T16:52:22Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2021-05-27T17:54:39Z

@regisb 🎉 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 · 2021-05-27T19:03:37Z

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

edx-pipeline-bot · 2021-05-27T19:34:25Z

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

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels May 10, 2021

regisb force-pushed the regisb/mypy branch 2 times, most recently from f59a82f to 8f9aec1 Compare May 10, 2021 11:15

ormsbee reviewed May 10, 2021

View reviewed changes

regisb force-pushed the regisb/mypy branch from 8f9aec1 to 25c27c5 Compare May 10, 2021 14:35

regisb force-pushed the regisb/mypy branch from 25c27c5 to 10d8075 Compare May 10, 2021 14:43

kdmccormick reviewed May 20, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 20, 2021

regisb force-pushed the regisb/mypy branch from e2839a6 to 64e47ff Compare May 21, 2021 14:15

openedx-webhooks added changes requested and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 24, 2021

ormsbee approved these changes May 27, 2021

View reviewed changes

regisb force-pushed the regisb/mypy branch from 2a6403b to 88a2d28 Compare May 27, 2021 16:18

ormsbee merged commit a131d63 into openedx:master May 27, 2021

openedx-webhooks added merged and removed changes requested labels May 27, 2021

regisb mentioned this pull request May 31, 2021

build: run github actions on pull requests #27786

Merged

bradenmacdonald mentioned this pull request Jan 9, 2025

Upgrade django-stubs again, unpin mypy, and add more annotations #36072

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run mypy as part of testing the codebase #27575

feat: run mypy as part of testing the codebase #27575

regisb commented May 10, 2021

openedx-webhooks commented May 10, 2021

ormsbee left a comment

ormsbee May 10, 2021

regisb May 10, 2021

kdmccormick May 10, 2021

regisb May 10, 2021

ormsbee May 10, 2021

ormsbee May 10, 2021

kdmccormick May 20, 2021

regisb May 21, 2021

kdmccormick May 21, 2021

regisb May 25, 2021

ormsbee May 10, 2021 •

edited

Loading

regisb May 10, 2021

kdmccormick May 10, 2021

kdmccormick May 10, 2021

regisb May 10, 2021

ormsbee May 10, 2021

kdmccormick May 20, 2021

kdmccormick commented May 10, 2021 •

edited

Loading

regisb commented May 10, 2021

natabene commented May 11, 2021

kdmccormick May 20, 2021

kdmccormick May 20, 2021 •

edited

Loading

natabene commented May 26, 2021

regisb commented May 27, 2021

edx-status-bot commented May 27, 2021

openedx-webhooks commented May 27, 2021

edx-pipeline-bot commented May 27, 2021

edx-pipeline-bot commented May 27, 2021

feat: run mypy as part of testing the codebase #27575

feat: run mypy as part of testing the codebase #27575

Conversation

regisb commented May 10, 2021

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented May 10, 2021

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented May 10, 2021 • edited Loading

regisb commented May 10, 2021

natabene commented May 11, 2021

Choose a reason for hiding this comment

kdmccormick May 20, 2021 • edited Loading

Choose a reason for hiding this comment

natabene commented May 26, 2021

regisb commented May 27, 2021

edx-status-bot commented May 27, 2021

openedx-webhooks commented May 27, 2021

edx-pipeline-bot commented May 27, 2021

edx-pipeline-bot commented May 27, 2021

ormsbee May 10, 2021 •

edited

Loading

kdmccormick commented May 10, 2021 •

edited

Loading

kdmccormick May 20, 2021 •

edited

Loading