BOM-2245 : Unpin python-dateutil by mumarkhan999 · Pull Request #30255 · openedx/openedx-platform

mumarkhan999 · 2022-04-15T06:52:30Z

Last time we tried to bump the version of python-dateutil (PR#27196) we got this error

Line 585 in method get_course_assignments was causing the error at that time

https://github.com/openedx/edx-platform/blob/072b6b8875e2a6323b6666998a3bf5c5e6444039/lms/djangoapps/courseware/courses.py#L585

The reason of the error was that start datetime object which we were getting from BlockStructureBlockData wasn't updated as per the latest version of python-dateutil
https://github.com/openedx/edx-platform/blob/072b6b8875e2a6323b6666998a3bf5c5e6444039/lms/djangoapps/courseware/courses.py#L584

https://github.com/openedx/edx-platform/blob/072b6b8875e2a6323b6666998a3bf5c5e6444039/openedx/core/djangoapps/content/block_structure/block_structure.py#L396

The tzlocal object assigned to tzinfo variable of start date was like this

But according to the latest version of python-dateutil it should have been like this

So now as we know that the problem is with the outdated datetime objects. We should update them and this is what we are doing in this PR. We have added a patch where we are creating updated datetime objects using the outdated ones and using them for calculations and comparisons.

How did I test?

I used follwoing 3 branches to test this solution.

bom-2245-attempt-3 (current branch)
- Has updated version of python-dateutil
- Has patch where we are creating updated datetime objects from the outdated ones
test-bom-2245-attempt-3
- Has updated version of python-dateutil
- Has no patch, we're using outdated datetime objects
master
- Has outdated version of python-dateutil
- Has no patch

I also created a code snippet to replicate the behavior of get_course_assingments method, so that I can trigger that behavior from lms-shell

from xmodule.modulestore.django import modulestore
from opaque_keys.edx.keys import CourseKey
from lms.djangoapps.course_blocks.api import get_course_blocks
from django.contrib.auth.models import User
from datetime import datetime
import pytz


course_key_string = 'course-v1:edX+DemoX+Demo_Course'
course_key = CourseKey.from_string(course_key_string)  
store = modulestore()
course_usage_key = store.make_course_usage_key(course_key)

user = User.objects.get(username='admin')
block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True)

dates = []

for section_key in block_data.get_children(course_usage_key):
    for subsection_key in block_data.get_children(section_key):
        due = block_data.get_xblock_field(subsection_key, 'due')
        graded = block_data.get_xblock_field(subsection_key, 'graded', False)
        if due and graded:
            start = block_data.get_xblock_field(subsection_key, 'start')
            dates.append(start)



print(dates)
vars(dates[0].tzinfo)

now = datetime.now(pytz.UTC)
print(now)
print(now < dates[0])

So I executed the above code snippet on the mentioned branches and I got the following outputs:

For `master`

The datetime objects are outdated
The date comparisons are working fine with the outdated version of python-dateutil

When I switched from `master` to `bom-2245-attempt-3` (`current branch`)

The datetime objects are updated due to the patch we have applied
The date comparsons are working fine with the updated version of python-dateutil

When I switched from `bom-2245-attempt-3` to `test-bom-2245-attempt-3`

The datetime objects are not updated due to the absense of patch
The date comparisons are not working as datetime objects are outdated

mumarkhan999 · 2022-05-11T12:43:35Z

Hi @regisb , can you please have a look at this PR. It's related to python-dateutil version update.
You have worked on python-dateutil related issues so If you can review this one as well it would be great.

regisb

Thanks for resuming work on this issue. The fix matches my analysis from here and it looks good to me!

jmbowman · 2022-05-17T18:09:56Z

openedx/core/djangoapps/content/block_structure/block_structure.py

+                return datetime(
+                    year=xblock_field.year, month=xblock_field.month, day=xblock_field.day,
+                    hour=xblock_field.hour, minute=xblock_field.minute, second=xblock_field.second,
+                    tzinfo=tzlocal()


I don't see anywhere else we use tzlocal in edx-platform, and the Django docs indicate that UTC is generally used to store in MySQL when USE_TZ = True.

If we check the course_outline on sandbox where USE_TZ = True, we will find that tzlocal is being used multiple times

Code snippet to check the course outline:

from django.contrib.auth.models import User from django.test import RequestFactory from openedx.features.course_experience.utils import get_course_outline_block_tree user = User.objects.get(username='admin') request = RequestFactory().get(u'/') request.user = user course_key_string = 'course-v1:edX+DemoX+Demo_Course' course_outline = get_course_outline_block_tree(request, course_key_string, request.user) print(course_outline)

Ah, I forgot that this is coming from a cache instead of the database. Yes, in that sense it makes sense to just replicate the time zone they had before being cached.

jmbowman · 2022-05-17T18:12:39Z

lms/djangoapps/instructor/tests/test_api.py

 import pytest
 import pytz
 from boto.exception import BotoServerError
+from dateutil.zoneinfo import gettz


Why not the original dateutil.tz? According to the python-dateutil docs, the method you have here is deprecated.

Updated.
Two tests were failing when I first created this PR that's why I tried to use this method.
But now tests are passing even after removing this method. So now using the original dateutil.tz
as you suggested.

jmbowman · 2022-05-17T18:12:47Z

lms/djangoapps/instructor/views/api.py

 import dateutil
 import pytz
 import edx_api_doc_tools as apidocs
+from dateutil.zoneinfo import gettz


Same question here.

jmbowman · 2022-05-17T18:19:21Z

requirements/constraints.txt

-networkx<2.6
+# numpy>=1.17 is required for matplotlib>=3.5.1
+# and numpy>=1.17.0 caused failures in importing numpy in code-jail environment.
+matplotlib==3.4.0


This looks like it would upgrade matplotlib from 3.3.4 to 3.4.0, but I don't see a corresponding change in py38.txt. And I'd rather not risk upgrading that in codejail at the same time as we attempt the python-dateutil upgrade, so can we shift this back to matplotlib<3.4.0?

Done. Will be upgraded in separate PR if required.

jmbowman

Thanks for helping unravel this! I still think we'll want to stop using pickle on datetimes altogether in the future to avoid further upgrade hassles, but hopefully this will get us through this upgrade at least.

Are there still any cache purges that we'll need to coordinate with SRE during deployment, or do you think this should cover it?

jmbowman · 2022-05-18T12:52:11Z

openedx/core/djangoapps/content/block_structure/block_structure.py

+                return datetime(
+                    year=xblock_field.year, month=xblock_field.month, day=xblock_field.day,
+                    hour=xblock_field.hour, minute=xblock_field.minute, second=xblock_field.second,
+                    tzinfo=tzlocal()


Ah, I forgot that this is coming from a cache instead of the database. Yes, in that sense it makes sense to just replicate the time zone they had before being cached.

mumarkhan999 · 2022-05-18T15:57:20Z

Thanks for helping unravel this! I still think we'll want to stop using pickle on datetimes altogether in the future to avoid further upgrade hassles, but hopefully this will get us through this upgrade at least.

Are there still any cache purges that we'll need to coordinate with SRE during deployment, or do you think this should cover it?

I think that we will not need any cache purges. But if somethings goes wrong, we can rely on that as a backup plan.

edx-pipeline-bot · 2022-05-31T14:38:50Z

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

edx-pipeline-bot · 2022-05-31T15:56:16Z

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

edx-pipeline-bot · 2022-05-31T16:05:23Z

EdX Release Notice: This PR has been rolled back from the production environment.

edx-pipeline-bot · 2022-05-31T19:54:28Z

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

edx-pipeline-bot · 2022-05-31T19:54:28Z

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

mumarkhan999 force-pushed the bom-2245-attempt-3 branch 5 times, most recently from f792247 to 2b961fb Compare April 15, 2022 19:43

mumarkhan999 self-assigned this Apr 15, 2022

mumarkhan999 force-pushed the bom-2245-attempt-3 branch 7 times, most recently from 7c95ff6 to 23db538 Compare April 18, 2022 10:50

awais786 requested a review from jmbowman May 6, 2022 15:03

mumarkhan999 requested a review from regisb May 11, 2022 12:43

regisb approved these changes May 12, 2022

View reviewed changes

mumarkhan999 requested a review from ormsbee May 12, 2022 07:27

mumarkhan999 force-pushed the bom-2245-attempt-3 branch from f82855f to 29e9413 Compare May 12, 2022 09:23

jmbowman reviewed May 17, 2022

View reviewed changes

mumarkhan999 force-pushed the bom-2245-attempt-3 branch 2 times, most recently from a5ef5b3 to f302d2a Compare May 18, 2022 08:34

jmbowman approved these changes May 18, 2022

View reviewed changes

mumarkhan999 force-pushed the bom-2245-attempt-3 branch from cd772d7 to 95d756e Compare May 23, 2022 12:27

fix: python-dateutil version issue

3492bed

mumarkhan999 force-pushed the bom-2245-attempt-3 branch from 95d756e to 3492bed Compare May 31, 2022 12:51

mumarkhan999 merged commit a631324 into master May 31, 2022

mumarkhan999 deleted the bom-2245-attempt-3 branch May 31, 2022 13:14

UsamaSadiq mentioned this pull request May 31, 2022

Revert "BOM-2245 : Unpin python-dateutil" #30515

Merged

mumarkhan999 mentioned this pull request Jun 2, 2022

fix: python-dateutil version issue #30530

Merged

regisb mentioned this pull request Jun 6, 2022

Fix "last submission time" parsing issue #25818

Closed

leangseu-edx mentioned this pull request Aug 11, 2022

fix: section.due datetime version issue #30848

Merged

Conversation

mumarkhan999 commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How did I test?

For master

When I switched from master to bom-2245-attempt-3 (current branch)

When I switched from bom-2245-attempt-3 to test-bom-2245-attempt-3

Uh oh!

mumarkhan999 commented May 11, 2022

Uh oh!

regisb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumarkhan999 May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmbowman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumarkhan999 commented May 18, 2022

Uh oh!

edx-pipeline-bot commented May 31, 2022

Uh oh!

edx-pipeline-bot commented May 31, 2022

Uh oh!

edx-pipeline-bot commented May 31, 2022

Uh oh!

edx-pipeline-bot commented May 31, 2022

Uh oh!

edx-pipeline-bot commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mumarkhan999 commented Apr 15, 2022 •

edited

Loading

For `master`

When I switched from `master` to `bom-2245-attempt-3` (`current branch`)

When I switched from `bom-2245-attempt-3` to `test-bom-2245-attempt-3`

mumarkhan999 May 18, 2022 •

edited

Loading