Fix #3797: Deprecate old classification framework #4653

akumar1503 · 2018-02-04T16:00:56Z

[Explain what your PR does here]

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.

i have removed LDAStringClassifier, domain/base_classifier.py, domain/classifier_registry.py.
and updated some frontend specs.

codecov-io · 2018-02-04T16:45:45Z

Codecov Report

Merging #4653 into develop will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4653      +/-   ##
===========================================
- Coverage    44.92%   44.86%   -0.06%     
===========================================
  Files          383      384       +1     
  Lines        23266    23303      +37     
  Branches      3750     3757       +7     
===========================================
+ Hits         10452    10455       +3     
- Misses       12814    12848      +34

Impacted Files	Coverage Δ
...d/components/SolutionExplanationEditorDirective.js	`2.77% <0%> (-0.93%)`	⬇️
...mplates/dev/head/components/HintEditorDirective.js	`2.38% <0%> (-0.75%)`	⬇️
...ractions/FractionInput/directives/FractionInput.js	`34.02% <0%> (-0.36%)`	⬇️
...ates/dev/head/components/OutcomeEditorDirective.js	`1.03% <0%> (-0.12%)`	⬇️
...exploration_editor/statistics_tab/StatisticsTab.js	`1.62% <0%> (-0.05%)`	⬇️
...on_editor/MarkAllAudioAsNeedingUpdateController.js	`16.66% <0%> (ø)`
...ploration_player/HintsAndSolutionManagerService.js	`77.88% <0%> (+0.21%)`	⬆️
...n_editor/editor_tab/StateContentEditorDirective.js	`74.46% <0%> (+6.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca46a4a...3cb1fdd. Read the comment docs.

prasanna08

@GanitGenius you have removed few parts of the new framework here. I have added relevant comments.

I guess the confusion arose because we are using LDAStringClassifier in unit tests of the new framework, as well. Actually, it should have been replaced by TextClassifier when that was implemented but now I realize that there are still some loose ends regarding the transition from LDA to TextClassifier.

prasanna08 · 2018-02-06T02:55:51Z

core/storage/classifier/gae_models_test.py


 (classifier_models,) = models.Registry.import_models(
    [models.NAMES.classifier])


-class ClassifierTrainingJobModelUnitTests(test_utils.GenericTestBase):


Keep this. It is part of new framework.

prasanna08 · 2018-02-06T03:00:47Z

feconf.py

@@ -78,7 +78,7 @@
 # A mapping of interaction ids to classifier properties.
 INTERACTION_CLASSIFIER_MAPPING = {
    'TextInput': {
-        'algorithm_id': 'LDAStringClassifier',
+        'algorithm_id': 'TextClassifier',


Why wasn't this modified before @anmolshkl? (when you implemented TextClassifier on Oppia side)

Also have you done the end to end tests (manually) of running TextClassifier i.e. start Oppia-ml and Oppia, change all the constants and enable classification framework, create or load any exploration that uses text classifier, and then check logs of Oppia-ml to see if classifier is trained, check logs of Oppia and see if trained model has been stored, also playtest exploration once the trained model has been stored and see if it predicts correct values?

prasanna08 · 2018-02-06T03:01:39Z

core/storage/classifier/gae_models_test.py

-    def test_create_and_get_new_training_job_runs_successfully(self):
-        next_scheduled_check_time = datetime.datetime.utcnow()
-        job_id = classifier_models.ClassifierTrainingJobModel.create(
-            'LDAStringClassifier', 'TextInput', 'exp_id1', 1,


You will have to change LDAStringClassifier to TextClassifier as you did in feconf.

prasanna08 · 2018-02-06T03:02:28Z

core/domain/classifier_domain_test.py

 from core.domain import classifier_domain
 from core.tests import test_utils
 import utils


-class ClassifierTrainingJobDomainTests(test_utils.GenericTestBase):


Keep this. It is part of the new framework.

prasanna08 · 2018-02-06T03:03:14Z

core/controllers/reader_test.py

@@ -199,55 +148,6 @@ def test_give_feedback_handler(self):
        self.logout()


-class ExplorationStateClassifierMappingTests(test_utils.GenericTestBase):


Keep this. It is part of the new framework.

akumar1503 · 2018-02-06T17:57:41Z

@prasanna08 i have added them now. please review.

pranavsid98

LGTM @GanitGenius . Just this one small nit.
Thanks!

pranavsid98 · 2018-02-07T05:45:08Z

core/storage/activity/gae_models_test.py

@@ -24,6 +24,7 @@
 class ActivityListModelTest(test_utils.GenericTestBase):
    """Tests the ActivityListModel class."""

+


Remove this new line

thanks @pranavsid98 it tried to add and it gives lint error for one line gap.

Thats weird. @seanlip Do you have any idea?

@apb7 should be able to advice. @GanitGenius it would be helpful if you could provide the exact text of the lint error here. (In general, always do that whenever you are reporting an error, otherwise it's pretty much impossible to debug.)

ok, will do.

ckd@PC:~/Desktop/dev/oppia/oppia$ git push origin deprecate_old_classification_framework ref_list: [GitRef(local_ref='refs/heads/deprecate_old_classification_framework', local_sha1='fe09b03b3a5cb90c5f729f79e363ec7b8a067580', remote_ref='refs/heads/deprecate_old_classification_framework', remote_sha1='783961291ecc1b26bbcc71f1fc0692624774bec1')] Modified files in deprecate_old_classification_framework: [FileDiff(status='M', name='core/storage/classifier/gae_models_test.py')] Files to lint in deprecate_old_classification_framework: ['core/storage/classifier/gae_models_test.py'] Starting def-spacing checks ---------------------------------------- core/storage/classifier/gae_models_test.py:27:1: E302 expected 2 blank lines, found 1 1 E302 expected 2 blank lines, found 1 ---------------------------------------- FAILED Def spacing checks failed

i think this might be due to this where tox.ini contains E302 check. waiting for apurv to confirm.

@seanlip I tried this locally (with a single line gap) but did not get any lint error here. Also, initially there was a single line gap only between the class and the method and no linting error was encountered back then.
@GanitGenius: I'm not exactly sure at the moment how this error springs up.

posting the full log:

ckd@PC:~/Desktop/dev/oppia/oppia$ git push origin deprecate_old_classification_framework ref_list: [GitRef(local_ref='refs/heads/deprecate_old_classification_framework', local_sha1='fe09b03b3a5cb90c5f729f79e363ec7b8a067580', remote_ref='refs/heads/deprecate_old_classification_framework', remote_sha1='783961291ecc1b26bbcc71f1fc0692624774bec1')] Modified files in deprecate_old_classification_framework: [FileDiff(status='M', name='core/storage/classifier/gae_models_test.py')] Files to lint in deprecate_old_classification_framework: ['core/storage/classifier/gae_models_test.py'] Starting def-spacing checks ---------------------------------------- core/storage/classifier/gae_models_test.py:27:1: E302 expected 2 blank lines, found 1 1 E302 expected 2 blank lines, found 1 ---------------------------------------- FAILED Def spacing checks failed Starting import-order checks ---------------------------------------- ---------------------------------------- Starting newline-at-EOF checks ---------------------------------------- ---------------------------------------- (1 files checked, 0 errors found) SUCCESS Newline character checks passed Starting linter... Starting Javascript and Python Linting ---------------------------------------- Linting 1 Python files Linting Python files 1 to 1... There are no JavaScript files to lint. -------------------------------------------------------------------- Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00) Python linting finished. ---------------------------------------- SUCCESS 1 Python files linted (1.3 secs) Starting Pattern Checks ---------------------------------------- ---------------------------------------- (1 files checked, 0 errors found) SUCCESS Pattern checks passed Push failed, please correct the linting issues above error: failed to push some refs to 'git@github.com:GanitGenius/oppia.git'

Commit changes:

ckd@PC:~/Desktop/dev/oppia/oppia$ git show fe09b03b3 commit fe09b03b3a5cb90c5f729f79e363ec7b8a067580 (HEAD -> deprecate_old_classification_framework) Author: GanitGenius <anonims.ak@gmail.com> Date: Wed Feb 7 12:55:01 2018 +0530 addressed review comment diff --git a/core/storage/classifier/gae_models_test.py b/core/storage/classifier/gae_models_test.py index e57267fa2..e50921f8b 100644 --- a/core/storage/classifier/gae_models_test.py +++ b/core/storage/classifier/gae_models_test.py @@ -24,7 +24,6 @@ import feconf (classifier_models,) = models.Registry.import_models( [models.NAMES.classifier]) - class ClassifierTrainingJobModelUnitTests(test_utils.GenericTestBase): """Test the ClassifierTrainingJobModel class."""

so earlier there was 2 line gap and after removing one line it gave that error.

got it. i was modifing the wrong file '/activity/gae_models_test.py' instead of '/classifier/gae_models_test.py'.
sorry for the trouble.

@GanitGenius: Re E302, it checks for two lines above top-level functions and class definitions. E301 checks for single line gap between methods (https://github.com/PyCQA/pycodestyle/blob/master/pycodestyle.py#L306:L308).

prasanna08

Hi @GanitGenius LGTM

pranavsid98 · 2018-02-09T09:52:30Z

Hi @seanlip, good to go for merge? Thanks!

seanlip · 2018-02-09T09:55:14Z

I defer entirely to @prasanna08 on this one -- if he's happy, then so am I.

I noticed one unresolved comment in feconf.py?

prasanna08 · 2018-02-09T13:11:50Z

Yes, that comment was meant for @anmolshkl. I wanted to make sure that we have tested the text input classifier from end to end. I will talk about this with him on hangouts. Thanks. Other than that this PR LGTM.

Edit: So I talked about this to @anmolshkl and he did not do manual end to end testing. So I created a new issue to do that test.

* partial work * removed classifiers from backend and frontend Specs * fix lint * added back new framework tests * addressed review comment * address review comments

akumar1503 added 3 commits February 2, 2018 05:05

partial work

69d8989

removed classifiers from backend and frontend Specs

04027d0

fix lint

51cfdc5

akumar1503 requested a review from pranavsid98 February 4, 2018 16:05

pranavsid98 self-assigned this Feb 4, 2018

seanlip requested a review from prasanna08 February 6, 2018 02:27

seanlip assigned prasanna08 Feb 6, 2018

prasanna08 suggested changes Feb 6, 2018

View reviewed changes

added back new framework tests

7839612

pranavsid98 approved these changes Feb 7, 2018

View reviewed changes

akumar1503 added 2 commits February 7, 2018 12:55

addressed review comment

fe09b03

address review comments

3cb1fdd

akumar1503 mentioned this pull request Feb 8, 2018

Deprecate the old classification framework #3797

Closed

prasanna08 approved these changes Feb 8, 2018

View reviewed changes

seanlip merged commit f8800ed into oppia:develop Feb 12, 2018

akumar1503 deleted the deprecate_old_classification_framework branch February 12, 2018 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3797: Deprecate old classification framework #4653

Fix #3797: Deprecate old classification framework #4653

akumar1503 commented Feb 4, 2018

codecov-io commented Feb 4, 2018 •

edited

Loading

prasanna08 left a comment

prasanna08 Feb 6, 2018

akumar1503 Feb 7, 2018

prasanna08 Feb 6, 2018 •

edited

Loading

prasanna08 Feb 6, 2018

prasanna08 Feb 6, 2018

akumar1503 Feb 7, 2018

prasanna08 Feb 6, 2018

akumar1503 Feb 7, 2018

akumar1503 commented Feb 6, 2018

pranavsid98 left a comment •

edited

Loading

pranavsid98 Feb 7, 2018

akumar1503 Feb 7, 2018

pranavsid98 Feb 7, 2018

seanlip Feb 7, 2018 •

edited

Loading

akumar1503 Feb 7, 2018 •

edited

Loading

apb7 Feb 7, 2018 •

edited

Loading

akumar1503 Feb 7, 2018

akumar1503 Feb 7, 2018

akumar1503 Feb 7, 2018

apb7 Feb 7, 2018

prasanna08 left a comment

pranavsid98 commented Feb 9, 2018

seanlip commented Feb 9, 2018

prasanna08 commented Feb 9, 2018 •

edited

Loading

		@@ -199,55 +148,6 @@ def test_give_feedback_handler(self):
		self.logout()


		class ExplorationStateClassifierMappingTests(test_utils.GenericTestBase):

		@@ -24,6 +24,7 @@
		class ActivityListModelTest(test_utils.GenericTestBase):
		"""Tests the ActivityListModel class."""

Fix #3797: Deprecate old classification framework #4653

Fix #3797: Deprecate old classification framework #4653

Conversation

akumar1503 commented Feb 4, 2018

codecov-io commented Feb 4, 2018 • edited Loading

Codecov Report

prasanna08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasanna08 Feb 6, 2018 • 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

akumar1503 commented Feb 6, 2018

pranavsid98 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

seanlip Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

akumar1503 Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

apb7 Feb 7, 2018 • 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

prasanna08 left a comment

Choose a reason for hiding this comment

pranavsid98 commented Feb 9, 2018

seanlip commented Feb 9, 2018

prasanna08 commented Feb 9, 2018 • edited Loading

codecov-io commented Feb 4, 2018 •

edited

Loading

prasanna08 Feb 6, 2018 •

edited

Loading

pranavsid98 left a comment •

edited

Loading

seanlip Feb 7, 2018 •

edited

Loading

akumar1503 Feb 7, 2018 •

edited

Loading

apb7 Feb 7, 2018 •

edited

Loading

prasanna08 commented Feb 9, 2018 •

edited

Loading