Tox, for parallel test goodness by efischer19 · Pull Request #1023 · openedx/edx-ora2

efischer19 · 2017-07-10T19:15:23Z

EDU-768

Attempts to split each of the following into separate test contexts on travis:

django 1.8 python tests
django 1.11 python tests
quality checks
js tests

efischer19 · 2017-07-10T21:27:51Z

This has now morphed into a "modernize ORA" PR. With the goal of faster, smaller, parallelized tests, I'll be doing the following:

modify installation, there's no reason we need to specify apt-packages. Requirements were also getting silly.
remove AI grading, that eats a lot of testing cycles for a feature that isn't enabled anywhere. I'll double-check with product to ensure this is acceptable.
tests will be more direct, and not make use of "travis calls tox calls makefile calls a bash script"

efischer19 · 2017-07-13T21:15:47Z

Running into an order-dependent flaky test in test_training.py, under --hashseed 2507972091

efischer19 · 2017-07-14T13:11:29Z

+django-nose==1.4.4
+git+https://github.com/edx/django-rest-framework.git@1ceda7c086fddffd1c440cc86856441bbf0bd9cb#egg=djangorestframework==3.6.3
+dogapi==1.11.1
+jsonfield==2.0.2


@michaelroytman just saw your recent branch - I'm upgrading jsonfield here as part of https://openedx.atlassian.net/browse/EDUCATOR-768

efischer19 · 2017-07-24T14:34:14Z

@@ -0,0 +1,22 @@
+# edX Internal Requirements
+git+https://github.com/edx/XBlock.git@efischer/play_nice_with_py27#egg=XBlock==1.0.0


Relies on https://github.com/edx/XBlock/pull/365, which is currently unmerged.

Merged and updated

efischer19 · 2017-07-24T18:38:06Z

Ready for review! @cahrens has volunteered to be a reviewer (thanks again!); @edx/educator-dahlia FYI as well.

Other subtasks before I close EDUCATOR-409:

~~upload to PyPI, I'll do this work in a separate PR for easier reviewing~~ punting on this actually
get https://github.com/edx/XBlock/pull/365 merged
update Confluence docs for install/test/developing against edx-ora2

cahrens · 2017-07-24T19:04:43Z

@@ -1,7 +0,0 @@
-Log files:
-
-apps_info.log = INFO level logging for all edx-ora2 apps and OpenAssessmentBlock


Should this information be captured someplace else? It seems like it could be useful (assuming it is still true).

I don't believe this info is true any longer, edx-platform defines logging locations (which work with splunk), and this particular file is >3 years past its last update.

cahrens · 2017-07-24T19:19:59Z

-        )
-        AssessmentPart.create_from_option_points(self.assessment, criterion_scores)
-        self.mark_complete_and_save()
+# This file is empty, but we cannot delete it b/c historical migration records refer to it.


Is this still true given our discussion?

I will double check on this one - it could be in a weird intermediary state from my internal debate about dealing with migrations.

cahrens · 2017-07-24T19:32:47Z

-"""
-# pylint:disable=W0611
-from .worker.training import train_classifiers, reschedule_training_tasks
-from .worker.grading import grade_essay, reschedule_grading_tasks


Why is worker.grading removed?

Everything under worker has been removed - the idea was to "train" your AI classifiers with a given data set ahead of time, then "grade" new essays as a separate phase. We don't need any references to either of these operations.

cahrens · 2017-07-24T19:39:06Z

        try:
            temp_url = swiftclient.utils.generate_temp_url(
-                path='%s/%s/%s' % (url.path, bucket_name, key_name),
+                path='/v%s%s/%s/%s' % (SWIFT_BACKEND_VERSION, url.path, bucket_name, key_name),


Will this cause any incompatibilities for people who are already using the Swift backend?

It shouldn't, this only affects newly generated upload urls.

cahrens · 2017-07-24T19:41:39Z

-from openassessment.assessment.models import PeerWorkflow
-
-
-class Command(BaseCommand):


Why was this file deleted?

I yanked out all performance tests as well, as they also heavily blurred the line between "python library" and "suite of scripts and programs to install, some of which may be python". The tests that were here were incredibly old, I think if we need performance testing against ora we're better off rewriting it in https://github.com/edx/edx-load-tests using the modern locust setup described there.

cahrens · 2017-07-24T19:44:03Z

        # Expect that we generated a URL for the bucket
        url = cmd.history[0]['url']
-        self.assertIn("https://{}".format(self.BUCKET_NAME), url)
+        self.assertIn("ttps://s3.amazonaws.com/{}".format(self.BUCKET_NAME), url)


Why is the "h" missing from the front?

Terrible copy-paste job by the dev who put this here, what a bum that guy must be (thanks, will fix)

cahrens · 2017-07-24T19:46:28Z

        self.possible_points = 2
        peer_api.get_submission_to_assess(self.scorer_submission['uuid'], 1)
        self.assessment = self._create_assessment(self.scorer_submission['uuid'])
+        self.assertEqual(self.assessment['parts'][0]['criterion']['label'], "criterion_1")


What caused this change (and the lines below, 475 and following?

It was a flaky test issue. A particular hashseed value caused the assessment to store out-of-order values, until I made this fix. Now, this test ensures that self.assessment['parts'][0] matches up to the first criterion and appears first in the downloaded data.

cahrens · 2017-07-24T19:53:20Z


    .action--save {
-      @extend %action-button
+      @extend %action-button !optional


Why was this necessary?

It was failing quality builds, not sure why it hadn't been before. I'll double-check to ensure that's still the case.

Example failure: https://travis-ci.org/edx/edx-ora2/jobs/257446578

cahrens · 2017-07-24T19:54:21Z

+      @extend %action-button !optional
    }

    .feedback {


Is there CSS that should be deleted related to AI grading? I'm thinking of the Studio editors, as well as the LMS view.

Oh, good call. I'll do an audit of that before the next round of review

I've poked around - no files are obviously AI grading exclusive, and I can't see any obvious removals in the existing files. Judging by that and the fact that very little HTML needed to be removed, I think we're safe assuming that this feature never had a lot of dedicated front-end customization done.

cahrens · 2017-07-24T19:56:54Z

-        # If the problem already contains example-based assessment
-        # then allow the editor to display example-based assessments,
-        # which is not included in the default
-        enabled_assessments = [asmnt['name'] for asmnt in self.valid_assessments]


Have you checked that no ORA instances in prod/Edge have AI steps defined? Just wondering how they would fail.

From my database poking, I don't believe there are any. I'll verify that, and set up such a problem locally to ensure we can handle that situation correctly.

cahrens · 2017-07-24T20:06:24Z

 #!/usr/bin/env bash

-MAX_PEP8_VIOLATIONS=106
+MAX_PEP8_VIOLATIONS=62  # TODO nomerge - but also stop failing during development


Don't forget to follow up on this.

cahrens · 2017-07-24T20:06:36Z

 #!/usr/bin/env bash

-MAX_PYLINT_VIOLATIONS=438
+MAX_PYLINT_VIOLATIONS=495  # TODO nomerge - but also stop failing during development


Don't forget to follow up on this.

cahrens · 2017-07-24T20:07:54Z

-Performance Tests
-=================
-
-1. Install performance test requirements:


No more performance tests?

Correct, I've eliminated them from this repo for reasons outlined above.

cahrens · 2017-07-24T20:08:17Z

What are the .gitkeep files?

efischer19 · 2017-07-28T20:04:57Z

Alright @cahrens, I believe this is ready again, I've addressed all your comments from before. The diff of the latest commit looks scary, but a large portion of it was me running isort to standardize all our import statements (which fixed a huge number of quality errors).

I'm also running acceptance tests at https://gocd.tools.edx.org/go/pipelines/value_stream_map/build_ora2_sandbox/142.

efischer19 · 2017-07-28T20:05:42Z

 # edX Internal Requirements
-git+https://github.com/edx/XBlock.git@xblock-0.4.12#egg=XBlock==0.4.12
+# edx-submissions>=2.0.6,<3.0.0 #TODO: update after 2.0.6 release
+git+https://github.com/edx/edx-submissions.git@efischer/loosen_reqs#egg=edx-submissions==2.0.6 # TODO: and remove this line


These two lines are effectively "I'm waiting for openedx/edx-submissions#65 to merge before this"

cahrens · 2017-07-31T15:58:01Z

 # Install commands
 ##################
 install-python:
+	pip install -r requirements/django.txt


Why a separate file?

I see-- because of the tox configuration. Never mind.

cahrens · 2017-07-31T16:11:56Z

+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):


What caused this migration?

It's the same thing that's happening here - We can remove the django_extensions dependency fairly easily, but that touches a field on a model, which leads to a migration. Said migration should be a no-op, but there are some pending operational questions to sort out.

The migration should have been in the last commit; I had forgotten that it needed to be generated and ran into a "please run makemigrations" warning while fixing other things. I'll work out the details with Kevin on my other PR and update this one with the results of that conversation.

Update: we're pushing the submissions upgrade through independently, as it had an identical migration. Expectation is that it runs as a no-op, we're going to verify that as submissions goes out since it's a smaller table.

cahrens · 2017-07-31T16:14:06Z

+# edX Internal Requirements
+# edx-submissions>=2.0.6,<3.0.0 #TODO: update after 2.0.6 release
+git+https://github.com/edx/edx-submissions.git@efischer/loosen_reqs#egg=edx-submissions==2.0.6 # TODO: and remove this line
+git+https://github.com/edx/django-rest-framework.git@1ceda7c086fddffd1c440cc86856441bbf0bd9cb#egg=djangorestframework==3.6.3


Why a specific commit of django-rest-framework?

@doctoryes set this requirement to a specific commit in #1024, I'm just moving it to another file

cahrens · 2017-07-31T16:15:15Z

+known_third_party = ddt,mock
+known_django = django
+known_djangoapp = jsonfield,model_utils
+known_edx =


Yeah, it has something to do with isort configuration. I copied this over from https://github.com/edx/edx-celeryutils/blob/master/setup.cfg, I think we can remove the known_edx line

cahrens · 2017-07-31T17:27:29Z

@@ -93,6 +92,12 @@
    'django.template.loaders.filesystem.Loader',


These are the default values, so TEMPLATE_LOADERS can be removed.

https://docs.djangoproject.com/en/1.11/ref/templates/upgrading/

cahrens · 2017-07-31T17:30:28Z

-from celery import Celery

 DEBUG = True
 TEMPLATE_DEBUG = DEBUG


I believe you should remove TEMPLATE_DEBUG. You can include 'debug' in the TEMPLATES options.

For example,

TEMPLATES = [ { 'BACKEND': 'django.template.backends.django.DjangoTemplates', 'DIRS': [ os_path.join(PROJECT_PATH, 'templates'), ], 'APP_DIRS': True, 'OPTIONS': { 'context_processors': [ "django.contrib.auth.context_processors.auth", "django.template.context_processors.debug", "django.template.context_processors.i18n", "django.template.context_processors.media", "django.template.context_processors.static", "django.template.context_processors.request", "django.template.context_processors.tz", "django.contrib.messages.context_processors.messages", "sekizai.context_processors.sekizai", ], 'debug': DEBUG, }, }, ]

Also, STATICFILES_DIRS and STATICFILES_FINDERS can be removed.

https://docs.djangoproject.com/en/1.11/ref/settings/#std:setting-STATICFILES_FINDERS

cahrens · 2017-07-31T17:35:03Z

This seems very odd:

Sorry... didn't mean to close, just comment!

cahrens · 2017-07-31T17:40:41Z

@efischer19 done re-reviewing-- just a few more comments/questions.

efischer19 · 2017-07-31T21:15:48Z

Thanks for the review @cahrens! I think I've answered all your questions, will get this updated witch code changes and ready for merge in the next day or so.

This work was done in the context of a larger PR, I'd be shocked if this commit can be successfully reverted by itself. If you're trying to restore AI grading though, it's a good place to start.

efischer19 · 2017-08-02T18:29:11Z

We're back - latest commit has:

updated location of developer wiki links (I'm going to be editing those soon to explain docker devstack setup FYI)
finalized edx-submissions requirement to 2.0.6, since that merged
lowered pylint error limit, since I was well below it
cleaned up settings/base.py as suggested
preemptively fixed a flaky test I noticed earlier this morning

Once tests have passed and everyone is happy, I'll get this squashed, merged, and deployed

cahrens · 2017-08-02T19:30:36Z

👍

Contains installation and testing changes for the django 1.11 upgrade preparedness initiative. Splits tests out into seperate shards on travis.

Code changes needed to work with the upgraded dependencies from the previous commit.

efischer19 · 2017-08-02T21:20:08Z

🎆

Thanks for the help here @cahrens!

I'm going to get this deployed tomorrow

cahrens · 2017-08-03T13:27:04Z

🚀

efischer19 force-pushed the efischer/tox branch 4 times, most recently from 979751f to 01cb48b Compare July 13, 2017 20:02

efischer19 commented Jul 14, 2017

View reviewed changes

efischer19 force-pushed the efischer/tox branch 2 times, most recently from f591f51 to c7138bc Compare July 14, 2017 20:04

efischer19 mentioned this pull request Jul 20, 2017

Update ORA2 and submissions to include field error printing. openedx/openedx-platform#15626

Merged

efischer19 force-pushed the efischer/tox branch 4 times, most recently from d54f479 to 5d59212 Compare July 21, 2017 20:50

efischer19 commented Jul 24, 2017

View reviewed changes

cahrens reviewed Jul 24, 2017

View reviewed changes

efischer19 commented Jul 28, 2017

View reviewed changes

cahrens reviewed Jul 31, 2017

View reviewed changes

cahrens closed this Jul 31, 2017

cahrens reopened this Jul 31, 2017

efischer19 mentioned this pull request Aug 2, 2017

Loosen requirements openedx/edx-submissions#65

Merged

Remove AI grading

5a07f1f

This work was done in the context of a larger PR, I'd be shocked if this commit can be successfully reverted by itself. If you're trying to restore AI grading though, it's a good place to start.

efischer19 force-pushed the efischer/tox branch from fc3da33 to 7064f90 Compare August 2, 2017 18:26

efischer19 force-pushed the efischer/tox branch from 7064f90 to 63413e9 Compare August 2, 2017 19:02

Eric Fischer added 2 commits August 2, 2017 15:49

Modernize ORA, commit 1 of 2

0060509

Contains installation and testing changes for the django 1.11 upgrade preparedness initiative. Splits tests out into seperate shards on travis.

Modernize ORA, commit 2 of 2

3e13d27

Code changes needed to work with the upgraded dependencies from the previous commit.

efischer19 force-pushed the efischer/tox branch from 63413e9 to 3e13d27 Compare August 2, 2017 19:51

efischer19 merged commit 60f6ea3 into master Aug 2, 2017

efischer19 deleted the efischer/tox branch August 2, 2017 21:19

This was referenced Aug 3, 2017

Loosen celery version pin #972

Closed

Updated requirements to ranges #946

Closed

Unpin dependencies #937

Closed

Upgrade numpy to it's latest version #907

Closed

		@@ -0,0 +1,22 @@
		# edX Internal Requirements
		git+https://github.com/edx/XBlock.git@efischer/play_nice_with_py27#egg=XBlock==1.0.0

		@@ -1,7 +0,0 @@
		Log files:

		apps_info.log = INFO level logging for all edx-ora2 apps and OpenAssessmentBlock

		from openassessment.assessment.models import PeerWorkflow


		class Command(BaseCommand):

		from django.db import migrations, models


		class Migration(migrations.Migration):

		@@ -93,6 +92,12 @@
		'django.template.loaders.filesystem.Loader',

Uh oh!

Conversation

efischer19 commented Jul 10, 2017

Uh oh!

efischer19 commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efischer19 commented Jul 13, 2017

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!

efischer19 commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Jul 24, 2017

Uh oh!

efischer19 commented Jul 28, 2017

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!

efischer19 commented Jul 10, 2017 •

edited

Loading

efischer19 commented Jul 24, 2017 •

edited

Loading

cahrens commented Jul 31, 2017 •

edited

Loading