Skip to content

Loosen requirements #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
efischer19 merged 1 commit into from
Aug 2, 2017
Merged

Loosen requirements #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
dogapi==1.2.1
django-extensions==1.5.9
django-model-utils==2.3.1
dogapi>=1.2.1,<2.0.0
django-model-utils>=2.3.1,<3.0.0
# Use the same DRF version as edx-platform
git+https://github.com/edx/django-rest-framework.git@1ceda7c086fddffd1c440cc86856441bbf0bd9cb#egg=djangorestframework==3.6.3
jsonfield==1.0.3
jsonfield>=1.0.3,<2.0.0
pytz
3 changes: 0 additions & 3 deletions settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@
'django.contrib.admin',
'django.contrib.admindocs',

# Third party
'django_extensions',

@doctoryes doctoryes Jul 31, 2017

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.

Good to see dependencies removed!

# Test
'django_nose',

Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def load_requirements(*requirements_paths):

setup(
name='edx-submissions',
version='2.0.5',
version='2.0.6',
author='edX',
description='An API for creating submissions and scores.',
url='https://github.com/edx/edx-submissions.git',
Expand Down
10 changes: 7 additions & 3 deletions submissions/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import logging
import operator
import json
from uuid import UUID

from django.conf import settings
from django.core.cache import cache
Expand Down Expand Up @@ -222,9 +223,12 @@ def get_submission(submission_uuid, read_replica=False):

"""
if not isinstance(submission_uuid, basestring):
raise SubmissionRequestError(
msg="submission_uuid ({!r}) must be a string type".format(submission_uuid)
)
if isinstance(submission_uuid, UUID):
submission_uuid = unicode(submission_uuid)
else:
raise SubmissionRequestError(
msg="submission_uuid ({!r}) must be serializable".format(submission_uuid)
)

cache_key = Submission.get_cache_key(submission_uuid)
try:
Expand Down
3 changes: 1 addition & 2 deletions submissions/migrations/0001_initial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from django.db import migrations, models
import jsonfield.fields
import django.utils.timezone
import django_extensions.db.fields


class Migration(migrations.Migration):
Expand Down Expand Up @@ -48,7 +47,7 @@ class Migration(migrations.Migration):
name='Submission',
fields=[
('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
('uuid', django_extensions.db.fields.UUIDField(db_index=True, version=1, editable=False, blank=True)),
('uuid', models.UUIDField(db_index=True, editable=False, blank=True)),
('attempt_number', models.PositiveIntegerField()),
('submitted_at', models.DateTimeField(default=django.utils.timezone.now, db_index=True)),
('created_at', models.DateTimeField(default=django.utils.timezone.now, editable=False, db_index=True)),
Expand Down
20 changes: 20 additions & 0 deletions submissions/migrations/0004_remove_django_extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models
import uuid


class Migration(migrations.Migration):

dependencies = [
('submissions', '0003_submission_status'),
]

operations = [
migrations.AlterField(
model_name='submission',
name='uuid',
field=models.UUIDField(default=uuid.uuid4, db_index=True),
),
]

@doctoryes doctoryes Jul 31, 2017

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.

What's the deployment implications of this migration? How big is the submissions_submission table on production?

@efischer19 efischer19 Jul 31, 2017

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.

This is a good question - the only change is a semantic one around the uuid field (it has no practical effect on normal usage), but the output of manage.py sqlmigrate is not something I'd expect:

BEGIN;
ALTER TABLE `submissions_submission` ALTER COLUMN `uuid` SET DEFAULT '240af22377d44f82817ee79b1a5fd61d';
ALTER TABLE `submissions_submission` ALTER COLUMN `uuid` DROP DEFAULT;

COMMIT;

The size of the table is 1.7M rows in prod. However, I would argue that the above is effectively a no-op, and we ought to just skip applying the migration entirely.

@jibsheet, any suggestions here? I'm attempting to remove the django_extensions requirement on this project, and it's used in a model field. That change necessitated a migration, which when poked seems to do nothing, effectively. Can we declare this "safe to skip" and include a --fake in the GoCD rollout of this code change? Is that even possible?

If this turns out to be too complex, I'm fine with reverting the django_extensions removal out.

@jibsheet jibsheet Jul 31, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Last time I ran across this, it's a django stupidity but MySQL Innodb treats it as a metadata change: https://github.com/edx/edx-platform/pull/14830#discussion_r110185023

You can probably make a million row table on a sandbox to test, but when I did something similar for Christina, it was fine. I'll mention that the behavior might differ on nullable fields, but I'd be shocked if a uuid field was NULLable.

@efischer19 efischer19 Jul 31, 2017

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.

Oh awesome, thanks for the links @jibsheet. I'll test this out on a sandbox tomorrow and get back to you.

@efischer19 efischer19 Aug 2, 2017

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.

Running against a sandbox with 200K rows on the table in question:

edxapp@ip-192-168-0-15:~/edx-platform$ time ./manage.py lms --settings=aws migrate submissions 0004
<lots of django warnings>
2017-08-02 09:56:21,582 INFO 12833 [dd.dogapi] dog_stats_api.py:66 - Initializing dog api to use statsd: localhost, 8125
Operations to perform:
  Target specific migration: 0004_remove_django_extensions, from submissions
Running migrations:
  Rendering model states... DONE
  Applying submissions.0004_remove_django_extensions... OK

real	0m6.412s
user	0m5.656s
sys	0m0.676s

From my perspective, the Applying submissions.0004_remove_django_extensions portion was nearly instantaneous, the 5 seconds were spent in django warmups and model state rendering.

@jibsheet jibsheet Aug 2, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yep, that sounds reasonable

6 changes: 3 additions & 3 deletions submissions/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@

"""
import logging
from uuid import uuid4

from django.db import models, DatabaseError
from django.db.models.signals import post_save
from django.dispatch import receiver, Signal
from django.utils.timezone import now
from django_extensions.db.fields import UUIDField
from jsonfield import JSONField


Expand Down Expand Up @@ -101,7 +101,7 @@ class Submission(models.Model):
"""
MAXSIZE = 1024*100 # 100KB

uuid = UUIDField(version=1, db_index=True)
uuid = models.UUIDField(db_index=True, default=uuid4)

student_item = models.ForeignKey(StudentItem)

Expand Down Expand Up @@ -196,7 +196,7 @@ def submission_uuid(self):

"""
if self.submission is not None:
return self.submission.uuid
return unicode(self.submission.uuid)
else:
return None

Expand Down
13 changes: 8 additions & 5 deletions submissions/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def setUp(self):
"""
Clear the cache.
"""
super(TestSubmissionsApi, self).setUp()
cache.clear()

@ddt.data(ANSWER_ONE, ANSWER_DICT)
Expand All @@ -67,10 +68,14 @@ def test_get_submission_and_student(self):
retrieved = api.get_submission_and_student(submission['uuid'])
self.assertItemsEqual(submission, retrieved)

# Should raise an exception if the student item does not exist
with self.assertRaises(api.SubmissionNotFoundError):
# Should raise an exception if uuid is malformed
with self.assertRaises(api.SubmissionInternalError):
api.get_submission_and_student(u'no such uuid')

# Should raise a different exception if the student item does not exist
with self.assertRaises(api.SubmissionNotFoundError):
api.get_submission_and_student(u'deadbeef-1234-5678-9100-1234deadbeef')

def test_get_submissions(self):
api.create_submission(STUDENT_ITEM, ANSWER_ONE)
api.create_submission(STUDENT_ITEM, ANSWER_TWO)
Expand Down Expand Up @@ -161,9 +166,7 @@ def test_get_submission(self):

# Test not found
with self.assertRaises(api.SubmissionNotFoundError):
api.get_submission("notarealuuid")
with self.assertRaises(api.SubmissionNotFoundError):
api.get_submission("0" * 50) # This is bigger than our field size
api.get_submission("deadbeef-1234-5678-9100-1234deadbeef")

@patch.object(Submission.objects, 'get')
@raises(api.SubmissionInternalError)
Expand Down