Python upgrade from 3.9.x to 3.12.x #3089

mudassir-hafeez · 2024-08-06T13:00:59Z

IMPORTANT: This PR (Python upgrade) depends on the Webpack upgrade PR. Please review and merge the Webpack upgrade PR first. After it is merged, this PR will be rebased accordingly. Please follow this comment https://github.com/mitodl/hq/issues/5077#issuecomment-2276436824 to get further information.

What are the relevant tickets?

#4965

Description (What does it do?)

This PR upgrades Python to the latest supported version 3.12.4 along with some other dependencies that need to be upgraded to support it.

Screenshots (if appropriate):

Desktop screenshots
Mobile width screenshots

How can this be tested?

Before getting started, rebuild your containers with docker-compose build web celery to ensure you are working with containers built with the new versions. The entire site should be smoke tested. Here is a basic list, but please add anything else as necessary:

Additional Context

To start reviewing this PR, please see the recommendation mentioned here: https://github.com/mitodl/hq/issues/5077#issuecomment-2276436824

arslanashraf7

I requested a couple of changes and some questions. Some change requests would need to be applied overall as I didn't add a separate comment for each of them.

Also, My perception was that we will be doing Webpack upgrade as part of https://github.com/mitodl/hq/issues/5077 separately but this PR contains the code for that as well.

arslanashraf7 · 2024-08-12T10:48:38Z

authentication/middleware.py

@@ -30,7 +30,7 @@ def process_exception(self, request, exception):
            url = self.get_redirect_uri(request, exception)

            if url:  # noqa: RET503
-                url += ("?" in url and "&" or "?") + "message={0}&backend={1}".format(  # noqa: UP030, UP032
+                url += ("?" in url and "&" or "?") + "message={}&backend={}".format(  # noqa: UP032


We can even replace it with f-string

The reason pyupgrade did not convert this into an f-string is likely due to its design choice to prioritize readability. According to the pyupgrade documentation, it intentionally avoids converting expressions into f-strings if doing so would make the expression longer or if the substitution parameters are sufficiently complicated, as this can decrease readability.

In this case, the expression might have been considered sufficiently complex, or the transformation into an f-string might have made it less readable, which is why pyupgrade retained the .format() syntax.

However, if you believe an f-string would improve clarity in this case, I'm open to making the change manually. What do you think?

This holds true for similar comments you mentioned regarding f-string where pyupgrade intentionally avoids converting expressions into f-strings.

arslanashraf7 · 2024-08-13T08:20:58Z

courses/api.py

@@ -57,7 +57,7 @@ def get_user_enrollments(user):
            for program_enrollment in program_enrollments
        )
    )
-    program_course_ids = set(course.id for course in program_courses)  # noqa: C401


I think the set might have been intentional here for unique values.

arslanashraf7 · 2024-08-13T10:29:41Z

compliance/test_utils.py

@@ -33,14 +33,14 @@ def mock_cybersource_wsdl(mocked_responses, settings, service_version=SERVICE_VE
    Mocks the responses to achieve a functional WSDL
    """
    # in order for zeep to load the wsdl, it will load the wsdl and the accompanying xsd definitions
-    with open(f"{DATA_DIR}/CyberSourceTransaction_{service_version}.wsdl", "r") as wsdl:  # noqa: PTH123, UP015
+    with open(f"{DATA_DIR}/CyberSourceTransaction_{service_version}.wsdl") as wsdl:  # noqa: PTH123


The file open is readable by default but explicitly adding "r" adds more readability for all code readers. Did Python upgrade complain about it? Same goes for the line below.

It does not complain but the open() function in Python defaults to read mode ("r"), which pyupgrade makes explicitly specifying "r" to redundant-open-mode/unnecessary and removes it. So we can go any of the following options:

Keep the Explicit "r"

We can keep the explicit "r" for readability if that's preferred. Although pyupgrade removed it because it's not necessary, adding it back can make the intent clearer to all code readers, especially those who may not be aware of the default behavior.

Adopt Python's Default Behavior

We can follow Python's default behavior to avoid redundant open mode. It resolves redundant-open-modes (UP015 - derived from the pyupgrade) which recommends removing unnecessary open modes to reduce confusion.

Note: The first option will be applied consistently to all instances where this change is present and/or explicitly mentioned.

Thanks for the details you answered my question but maybe inverted, below is why.

It does not complain but the open() function in Python defaults to read mode ("r"), which pyupgrade makes explicitly specifying "r" to redundant-open-mode/unnecessary and removes it. So we can go any of the following options:

I think if pyupgrade complains about it that should mean that the python upgrade does complain. In, which case maybe we can remove the r and add a code comment above this line saying that the file open is read-only by default.

arslanashraf7 · 2024-08-13T10:32:14Z

courses/serializers_test.py

@@ -6,6 +6,7 @@

 import factory
 import pytest
+import pytz


We intentionally moved away from pytz as part of #2867. Official docs on https://docs.djangoproject.com/en/5.0/releases/4.0/#what-s-new-in-django-4-0

arslanashraf7 · 2024-08-13T10:33:04Z

courses/views_test.py

@@ -283,13 +283,11 @@ def test_course_view(  # noqa: PLR0913
            class_name = "enrolled"

    assert (
-        f'<a class="enroll-button {class_name}" href="{url}">'.encode("utf-8")  # noqa: UP012
-        in resp.content
+        f'<a class="enroll-button {class_name}" href="{url}">'.encode() in resp.content


Same thing here, Does python upgrade complain about it? explicitly adding utf-8 makes it more readable. Same for the places below.

This change is also made by pyupgrade to encode string to bytes literal as UTF-8 is the default encoding in Python 3. This resolves unnecessary-encode-utf8 (UP012))

My answer to this would be similar to #3089 (comment)

arslanashraf7 · 2024-08-13T11:05:55Z

mitxpro/templates/base.html

@@ -32,8 +32,6 @@
      {% endif %}
    </script>
    {% render_bundle 'style' %}
-    {% render_bundle 'common' %}


Is this needed? Or maybe this is part of Webpack upgrade, If so, we should remove it from this PR.

The changes you mentioned here, along with others related to Webpack are part of the Webpack upgrade. We should first review this PR before proceeding with the Python upgrade, as it depends on the Webpack upgrade and will need to be rebased afterward. For more details, refer to this comment #3089 (comment).

arslanashraf7 · 2024-08-13T11:06:22Z

package.json

I think all these changes are Webpack related which we should remove

arslanashraf7 · 2024-08-13T11:08:51Z

sheets/coupon_assign_api.py

@@ -823,7 +823,7 @@ def get_desired_coupon_assignments(cls, assignment_rows):
                "codes listed in the Sheet. There may be an invalid coupon code in the Sheet."
            )
        product_coupon_dict = dict(product_coupon_tuples)
-        return set((row.email, product_coupon_dict[row.code]) for row in valid_rows)  # noqa: C401
+        return {(row.email, product_coupon_dict[row.code]) for row in valid_rows}


I suppose it would maintain the uniqueness of the items in this set?

Yes, it will maintain the uniqueness of the items in the set. Set comprehensions, like {(row.email, product_coupon_dict[row.code]) for row in valid_rows}, automatically ensure that the items in the set are unique, just as set() does.
For example:

In [22]: valid_rows = [ ...: {"email": "[email protected]", "code": "code1"}, ...: {"email": "[email protected]", "code": "code2"}, ...: {"email": "[email protected]", "code": "code1"}, # Duplicate ...: ] In [23]: product_coupon_dict = { ...: "code1": "coupon1", ...: "code2": "coupon2", ...: } In [24]: # Using set comprehension ...: result_set_comprehension = {(row["email"], product_coupon_dict[row["code"]]) for row in valid_rows} In [25]: result_set_comprehension {('[email protected]', 'coupon2'), ('[email protected]', 'coupon1')}

This change was made by pyupgrade and is supported by Python’s style guidelines for clarity and performance. It also resolves the unnecessary-generator-set (C401) issue.

This holds true for similar set comprehensions you mentioned regarding uniqueness.

Yes, it will maintain the uniqueness of the items in the set.

Thanks. This answers my question.

arslanashraf7 · 2024-08-13T11:09:35Z

sheets/coupon_request_api.py

@@ -444,7 +444,7 @@ def process_row(self, row_index, row_data):
                row_db_record=coupon_gen_request,
                row_object=None,
                result_type=ResultType.FAILED,
-                message="Parsing failure: {}".format(str(exc)),  # noqa: UP032
+                message=f"Parsing failure: {exc!s}",


I think str would be called by default

I think str would be called by default

Yes, in many cases, calling str() within an f-string is unnecessary and can be removed entirely, as the value will be converted to a string automatically. But, pyupgrade keeps explicit type conversions like f"Parsing failure: {str(exc)}", which pre-commit further resolves to f"Parsing failure: {exc!s}" for rules like RUF010.

This comment is valid for the overall comment you mentioned regarding the str.

arslanashraf7 · 2024-08-13T11:09:52Z

sheets/deferral_request_api.py

@@ -134,7 +134,7 @@ def process_row(  # noqa: C901, PLR0911
                row_db_record=deferral_request,
                row_object=None,
                result_type=ResultType.FAILED,
-                message="Parsing failure: {}".format(str(exc)),  # noqa: UP032
+                message=f"Parsing failure: {exc!s}",


Some similar comments about this above

mudassir-hafeez · 2024-08-13T19:56:14Z

I requested a couple of changes and some questions. Some change requests would need to be applied overall as I didn't add a separate comment for each of them.

I’ve left comments on specific questions and will continue to add more as needed. Some of these comments address similar requests or questions.

Also, My perception was that we will be doing Webpack upgrade as part of https://github.com/mitodl/hq/issues/5077 separately but this PR contains the code for that as well.

I’ve mentioned the review order for these PRs in the ticket here and referred in this PR description . We should first review the Webpack upgrade PR before proceeding with this PR (Python upgrade), as it depends on the Webpack upgrade and will need to be rebased afterward. I will apply a "on hold" label to this PR until the Webpack upgrade PR is reviewed and merged.

arslanashraf7 · 2024-08-15T06:56:45Z

I requested a couple of changes and some questions. Some change requests would need to be applied overall as I didn't add a separate comment for each of them.

I’ve left comments on specific questions and will continue to add more as needed. Some of these comments address similar requests or questions.

I responded to them.

Also, My perception was that we will be doing Webpack upgrade as part of mitodl/hq#5077 separately but this PR contains the code for that as well.

I’ve mentioned the review order for these PRs in the ticket here and referred in this PR description . We should first review the Webpack upgrade PR before proceeding with this PR (Python upgrade), as it depends on the Webpack upgrade and will need to be rebased afterward. I will apply a "on hold" label to this PR until the Webpack upgrade PR is reviewed and merged.

Sorry, I didn't read the additional context because I didn't feel like this might have one. Thanks for following up on the call and adding the On Hold label. But this PR is more like blocked

Ideally in cases like you mentioned, we can follow the approach I'm mentioning below because that adds clarity:

If you are looking for PR#A to be reviewed before PR#B you should not mark PR#B as Needs Review you should mark it Blocked with a comment about why it is blocked
Critical information, such as the order of PR review, can be mentioned at the top of the PR description so that the reviewer would not wander off track.

arslanashraf7 · 2024-08-27T09:57:10Z

@mudassir-hafeez It is not blocked anymore. correct? Could you rebase the PR? There are conflicts.

mudassir-hafeez · 2024-08-27T12:53:57Z

@mudassir-hafeez It is not blocked anymore. correct? Could you rebase the PR? There are conflicts.

Yes, it's no longer blocked. I've rebased the PR, resolved the conflicts, and tested it. It should be good to proceed now.

arslanashraf7

👍

odlbot temporarily deployed to xpro-ci-pr-3089 August 6, 2024 13:04 Inactive

odlbot temporarily deployed to xpro-ci-pr-3089 August 6, 2024 13:05 Inactive

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from 4450d42 to 39b5a3d Compare August 6, 2024 17:07

odlbot temporarily deployed to xpro-ci-pr-3089 August 6, 2024 17:07 Inactive

odlbot temporarily deployed to xpro-ci-pr-3089 August 6, 2024 17:52 Inactive

odlbot temporarily deployed to xpro-ci-pr-3089 August 6, 2024 18:09 Inactive

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from 5d5b638 to 1bef9fa Compare August 8, 2024 16:31

odlbot temporarily deployed to xpro-ci-pr-3089 August 8, 2024 16:31 Inactive

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from 1bef9fa to 1f40793 Compare August 8, 2024 16:45

odlbot deployed to xpro-ci-pr-3089 August 8, 2024 16:45 View deployment

mudassir-hafeez marked this pull request as ready for review August 8, 2024 18:04

mudassir-hafeez added the Needs Review label Aug 8, 2024

feoh approved these changes Aug 9, 2024

View reviewed changes

arslanashraf7 self-assigned this Aug 12, 2024

arslanashraf7 requested changes Aug 13, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 13, 2024

mudassir-hafeez added the On Hold label Aug 13, 2024

mudassir-hafeez added Blocked and removed On Hold labels Aug 16, 2024

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from f3c04e9 to 9577311 Compare August 27, 2024 10:00

mudassir-hafeez added 6 commits August 27, 2024 15:04

build: update dependencies for webpack 4 to 5

358a3a9

test: fix tests for python upgrade and update python version for CI

28a60a5

refactor: upgrade python syntax to support newer version

aa4a660

fix: precommit fixes

314d076

fix: Resolve more failing pre-commit check

334bf84

fix: more precommit fixes

deff5a9

fix: build to run js test

b447d54

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from 850ea97 to f2895b6 Compare August 27, 2024 10:05

docs: few comments to hightlight and a test to refactor

e3dffde

mudassir-hafeez force-pushed the mudassir/python-upgrade-3.12 branch from f2895b6 to e3dffde Compare August 27, 2024 10:42

mudassir-hafeez added Needs Review and removed Blocked Waiting on Author labels Aug 27, 2024

arslanashraf7 approved these changes Aug 27, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 27, 2024

mudassir-hafeez merged commit c0ea374 into master Aug 28, 2024
7 checks passed

mudassir-hafeez deleted the mudassir/python-upgrade-3.12 branch August 28, 2024 06:27

This was referenced Aug 29, 2024

Release 0.159.0 #3124

Closed

Release 0.159.0 #3128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python upgrade from 3.9.x to 3.12.x #3089

Python upgrade from 3.9.x to 3.12.x #3089

mudassir-hafeez commented Aug 6, 2024 •

edited

Loading

arslanashraf7 left a comment •

edited

Loading

arslanashraf7 Aug 12, 2024

mudassir-hafeez Aug 16, 2024

arslanashraf7 Aug 13, 2024

arslanashraf7 Aug 13, 2024

mudassir-hafeez Aug 13, 2024

arslanashraf7 Aug 15, 2024

arslanashraf7 Aug 13, 2024

arslanashraf7 Aug 13, 2024

mudassir-hafeez Aug 13, 2024

arslanashraf7 Aug 15, 2024

arslanashraf7 Aug 13, 2024

mudassir-hafeez Aug 13, 2024

arslanashraf7 Aug 13, 2024

arslanashraf7 Aug 13, 2024

mudassir-hafeez Aug 13, 2024

arslanashraf7 Aug 15, 2024 •

edited

Loading

arslanashraf7 Aug 13, 2024

mudassir-hafeez Aug 16, 2024

arslanashraf7 Aug 13, 2024

mudassir-hafeez commented Aug 13, 2024

arslanashraf7 commented Aug 15, 2024

arslanashraf7 commented Aug 27, 2024 •

edited

Loading

mudassir-hafeez commented Aug 27, 2024

arslanashraf7 left a comment

Python upgrade from 3.9.x to 3.12.x #3089

Python upgrade from 3.9.x to 3.12.x #3089

Conversation

mudassir-hafeez commented Aug 6, 2024 • edited Loading

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Additional Context

arslanashraf7 left a comment • 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

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

arslanashraf7 Aug 15, 2024 • 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

mudassir-hafeez commented Aug 13, 2024

arslanashraf7 commented Aug 15, 2024

arslanashraf7 commented Aug 27, 2024 • edited Loading

mudassir-hafeez commented Aug 27, 2024

arslanashraf7 left a comment

Choose a reason for hiding this comment

mudassir-hafeez commented Aug 6, 2024 •

edited

Loading

arslanashraf7 left a comment •

edited

Loading

arslanashraf7 Aug 15, 2024 •

edited

Loading

arslanashraf7 commented Aug 27, 2024 •

edited

Loading