Skip to content

Update pytz to 2015.2 - #6712

Merged
singingwolfboy merged 2 commits into
masterfrom
update-pytz
Apr 29, 2015
Merged

Update pytz to 2015.2#6712
singingwolfboy merged 2 commits into
masterfrom
update-pytz

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

pytz changed their versioning scheme from using letters to using
numbers. The old scheme is screwing up new versions of pip.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

This is going to be a tricky upgrade, because we also specify pytz==2012h in ora2 and ease. I've created openedx/edx-ora2#663 and openedx-unsupported/ease#62 for those repos -- when we decide to do the upgrade, we should merge all three pull requests simultaneously (or as close to simultaneously as possible).

@e0d, can you or someone else on the devops team review this, and provide some perspective on it? @benpatterson, can you or someone else on the testeng team take a look from a testing perspective? I can create an issue in JIRA to track this, as well.

@benpatterson

Copy link
Copy Markdown
Contributor

Bump. Could we sync up on this in the next week or so? I'd like to scope it for next sprint if possible, and move this along. @singingwolfboy not sure what you have in mind for the testing perspective? Have you looked at where pytz is called in code and if it's covered by tests (vs being mocked)?

@benpatterson

Copy link
Copy Markdown
Contributor

Oh also, I'm wondering if there's a particular driver for the upgrade (or if it's perhaps upgrade-treadmill/hygiene...)

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@benpatterson I just rebased the pull request. In terms of why I want to do this upgrade, I mentioned that in the description of the PR: "The old [versioning] scheme is screwing up new versions of pip." See pypa/pip#1144 for further context.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

jenkins, test this please

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @singingwolfboy! I've created OSPR-476 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

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

  • supporting documentation
  • edx-code email 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 still be done via the Github pull request interface. As a reminder, our process documentation is here.

Comment thread requirements/edx/github.txt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we switching to using your fork of ora2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to do that in order to get the automated tests to pass. Because edx-platform and edx-ora2 share the same virtualenv, the dependency versions must be kept in sync with each other. (Same deal with ease.) Once the edx fork of edx-ora2 has the same version of pytz as the edx fork of edx-platform, then we can make them depend on each other again. Does that make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@singingwolfboy I'm not sure I understand. Is there an edx-ora2 PR that I don't know about? Why not merge the changes into edx-ora2 repo first, then bubble up the changes to edx-platform with a new release of the edx/edx-ora2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could certainly do that. However, once we merge the changes into edx-ora2, we won't be able to do another release of edx-ora2 until either edx-platform upgrades, or we rollback the upgrade in edx-ora2. So it's still not something that we can do lightly.

@benpatterson

Copy link
Copy Markdown
Contributor

@singingwolfboy could you see where this lib is used and see how well (or not-well) we are covered? I'd like to figure out what kind of testing, if any, might be needed...

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@benpatterson: pytz is used a lot in our codebase: searching for the string "pytz" resulted in about 250 matches. The good news is that many of those matches are in our tests, as well. Based on skimming through the results, it seems like we're pretty well covered.

@feanil

feanil commented Apr 16, 2015

Copy link
Copy Markdown
Contributor

I think doing a release of ora2 is pretty reasonable, it doesn't get updated very often. If it came down to it, we can always revert the change, but this would change the amount of work that would be left once we got all the new changes merged.

@benpatterson

Copy link
Copy Markdown
Contributor

@singingwolfboy Thanks for doing the analysis. Can you give a better idea of the pieces that are not covered? I guess what I mean is...if we use pytz.foo in tons of places covered by tests, but we use pytz.bar in one place that's not covered, then I'd want at least manual verification of pytz.bar. If, OTOH, we use pytz.foo in tons of places, some covered by tests, some not covered, then I'd feel ok about the change, since we're hitting that method. Know what I mean? Any additional info would be great.

In terms of the ora2 conversation, whether or not ora2 is upgraded first, I'd feel better if the version that platform references is on the edx repo, rather than your fork. Can we make that kind of adjustment?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@benpatterson here's what I found for pytz usages in our codebase, and whether they're covered by automated tests:

cms/djangoapps/contentstore/signals: covered
cms/djangoapps/contentstore/tasks: covered
cms/djangoapps/contentstore/utils: covered
cms/djangoapps/contentstore/views/item: covered
common/djangoapps/course_modes/models: partial
common/djangoapps/course_modes/admin: covered
common/djangoapps/reverification/models: covered
common/djangoapps/student/helpers: MISSING
common/djangoapps/student/models: covered
common/djangoapps/student/views: partial
common/djangoapps/student/management/commands/assigngroups: MISSING
common/djangoapps/track/utils: covered
common/djangoapps/util/date_utils: covered
common/djangoapps/util/file: covered
common/lib/capa/capa/capa_problem: covered
common/lib/capa/capa/responsetypes: MISSING
common/lib/xmodule/xmodule/fields: partial
common/lib/xmodule/xmodule/assetstore: covered
common/lib/xmodule/xmodule/modulestore: covered
common/lib/xmodule/xmodule/modulestore/inheritance: covered
common/lib/xmodule/xmodule/modulestore/mongo/base: covered
common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection: covered
common/lib/xmodule/xmodule/modulestore/split_mongo/split: covered
common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module: covered
common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild: covered
lms/djangoapps/ccx/views: covered
lms/djangoapps/certificates/management/commands/ungenerated_certs: MISSING
lms/djangoapps/courseware/access: covered
lms/djangoapps/django_comment_client/utils: covered
lms/djangoapps/instructor/views/coupons: covered
lms/djangoapps/instructor/views/instructor_dashboard: covered
lms/djangoapps/instructor_task/tasks_helper: covered
lms/djangoapps/lms_migration/management/commands/create_user: MISSING
lms/djangoapps/psychometrics/psychoanalyze: MISSING
lms/djangoapps/shoppingcart/models: covered
lms/djangoapps/shoppingcart/views: covered
lms/djangoapps/verify_student/views: covered
openedx/core/djangoapps/user_api/accounts/api: covered

In terms of what parts of pytz are used, it seems to be almost exclusively used for the UTC object it provides. There are also one or two places that use pytz.timezone.

@benpatterson

Copy link
Copy Markdown
Contributor

@singingwolfboy the pieces that are missing in your previous comment. Can those be manually tested?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@benpatterson: Several of the uncovered usages are Django management commands, which could be manually tested quite easily just by running the Django management command. However, I don't even know if those commands are working in our codebase today, so you'd need to check that first. For the others, I guess it would be a matter of running the platform and visiting pages that exercise the codepaths.

@benpatterson

Copy link
Copy Markdown
Contributor

@singingwolfboy Thanks for all the analysis. Since they're all used the same (1 of 2) ways, I say we are GTG. 👍

Comment thread requirements/edx/github.txt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should not be using your fork of edx-ora2. Let's get the fix into edx-ora2 instead, since you already have PR on that repo already.

@feanil

feanil commented Apr 28, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy for the management commands that would need to be manually tested, do you expect to test those on stage? @benpatterson do you think testing them in a sandbox is sufficient?

@benpatterson

Copy link
Copy Markdown
Contributor

@feanil testing on a sandbox sounds fine to me. I do echo @singingwolfboy's skepticism though. I'm not convinced all of those commands will be functional. They are a black eye in our codebase as far as coverage goes and I wouldn't be shocked if some were broken. But as part of that, I also see the value in testing them, so doing a round would be valuable.

And yeah...sandbox sounds good.

@feanil

feanil commented Apr 29, 2015

Copy link
Copy Markdown
Contributor

The ungenerated_certs command specifically should be tested thoroughly since we use it all the time to generate certs for students.

pytz changed their versioning scheme from using letters to using
numbers. The old scheme is screwing up new versions of pip.
singingwolfboy added a commit that referenced this pull request Apr 29, 2015
@singingwolfboy
singingwolfboy merged commit 6eaeddc into master Apr 29, 2015
@singingwolfboy
singingwolfboy deleted the update-pytz branch April 29, 2015 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants