Fix classifier models when exploration is reverted and add test case for auto migrations #5888

prasanna08 · 2018-11-18T13:54:17Z

This PR adds code to generate classifier models when exploration is reverted and a test case to ensure that auto migrations (done by Exploration Migration One Off job) generates appropriate classifier models.

…_classifier_training_job

seanlip

Thanks @prasanna08! Took a pass and left a few comments, PTAL.

core/templates/dev/head/app.js

seanlip · 2018-11-18T20:55:43Z

feconf.py

@@ -761,14 +761,14 @@ def get_empty_ratings():
 MAX_PLAYTHROUGHS_FOR_ISSUE = 5

 # Unfinished features.
-SHOW_TRAINABLE_UNRESOLVED_ANSWERS = False
+SHOW_TRAINABLE_UNRESOLVED_ANSWERS = True


Please revert all changes to this file, we aren't ready to launch ML in production yet.

Yep, my mistake. Will revert these in next commit.

core/domain/exp_services.py

core/domain/classifier_services.py

core/domain/exp_jobs_one_off_test.py

core/domain/classifier_services.py

core/domain/exp_jobs_one_off_test.py

seanlip · 2018-11-18T21:15:38Z

core/domain/exp_jobs_one_off_test.py

+        # Start migration job on sample exploration.
+        job_id = exp_jobs_one_off.ExplorationMigrationJobManager.create_new()
+        exp_jobs_one_off.ExplorationMigrationJobManager.enqueue(job_id)
+        with self.swap(feconf, 'MIN_TOTAL_TRAINING_EXAMPLES', 0):


Actually I think you should swap these constants to nonzero numbers and add a bit of training data. Otherwise you run the risk of this test passing simply because there is nothing ML-related to migrate. Ideally the training job model gets created organically as a result of making appropriate changes to the lesson and saving them.

Hmm, I thought about that but in schema 0 we didn't really have a notion of state or training data. To be frank, I am unable to figure out what goes where when that schema is updated to the latest version and hence I refrained from putting training data there. Maybe I will have to create a similar function which creates an exploration of a more recent schema version, where we do have a notion of training data. Also, training data is not part of such default explorations, so what it needs is a highly specific function (that creates an exploration with training data and also creates appropriate classifier data and mapping models) which is useful only in this particular test case.

I don't think you need to start at schema 0. You can start at the schema where the ML fields are introduced (as I think you're suggesting).

Also, I think creating such setup functions in test_utils.py would probably be useful, since we're likely to have more backend tests for the ML stuff down the road. So it probably will have usefulness beyond this particular case.

Modified test case.

prasanna08 · 2018-11-25T09:48:22Z

Hi, my end semester exams are upcoming in next couple of weeks and so I might not be able to work on this PR for a while. After my exams I'll pick this up from here.

EDIT: I will pick this up after 12 December.
Thanks

oppiabot · 2018-12-05T11:07:37Z

Hi @prasanna08, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

…_classifier_training_job

codecov-io · 2018-12-17T12:35:00Z

Codecov Report

Merging #5888 into develop will increase coverage by 0.36%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #5888      +/-   ##
===========================================
+ Coverage     45.3%   45.66%   +0.36%     
===========================================
  Files          520      516       -4     
  Lines        30667    30147     -520     
  Branches      4597     4535      -62     
===========================================
- Hits         13893    13766     -127     
+ Misses       16774    16381     -393

Impacted Files	Coverage Δ
core/templates/dev/head/app.js	`59.25% <100%> (ø)`	⬆️
...mplates/dev/head/pages/skill_editor/SkillEditor.js	`60% <0%> (-6.67%)`	⬇️
...emplates/dev/head/pages/preferences/Preferences.js	`45.61% <0%> (-0.94%)`	⬇️
.../dev/head/domain/question/QuestionObjectFactory.js	`92.98% <0%> (-0.47%)`	⬇️
...es/state_editor/StateInteractionEditorDirective.js	`33.33% <0%> (-0.4%)`	⬇️
...es/dev/head/domain/story/StoryNodeObjectFactory.js	`93.13% <0%> (-0.33%)`	⬇️
...plates/dev/head/domain/story/StoryUpdateService.js	`96.96% <0%> (-0.14%)`	⬇️
...v/head/pages/creator_dashboard/CreatorDashboard.js	`18.59% <0%> (-0.07%)`	⬇️
...plates/dev/head/services/StateRulesStatsService.js	`100% <0%> (ø)`	⬆️
... and 35 more

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 0f9c573...f2a7a22. Read the comment docs.

codecov-io · 2018-12-17T12:35:00Z

Codecov Report

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

@@             Coverage Diff             @@
##           develop    #5888      +/-   ##
===========================================
- Coverage     45.3%   45.21%   -0.09%     
===========================================
  Files          520      523       +3     
  Lines        30667    30722      +55     
  Branches      4597     4605       +8     
===========================================
- Hits         13893    13892       -1     
- Misses       16774    16830      +56

Impacted Files	Coverage Δ
...dev/head/pages/exploration_editor/RouterService.js	`17.5% <0%> (-5.31%)`	⬇️
...head/pages/exploration_editor/ExplorationEditor.js	`5.12% <0%> (-0.11%)`	⬇️
...lates/dev/head/pages/library/SearchBarDirective.js	`1.02% <0%> (-0.1%)`	⬇️
...es/exploration_editor/EditorNavigationDirective.js	`2.08% <0%> (-0.1%)`	⬇️
...ges/exploration_editor/settings_tab/SettingsTab.js	`0.62% <0%> (ø)`	⬆️
...ad/pages/skill_editor/SkillEditorRoutingService.js	`4% <0%> (ø)`	⬆️
...s/exploration_editor/ExplorationWarningsService.js	`11.34% <0%> (ø)`	⬆️
...loration_editor/EditorNavbarBreadcrumbDirective.js	`5.55% <0%> (ø)`	⬆️
...xploration_player/ExplorationPlayerStateService.js	`1.53% <0%> (ø)`	⬆️
...ad/pages/topic_editor/TopicEditorRoutingService.js	`3.33% <0%> (ø)`	⬆️
... and 14 more

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 0f9c573...4e78a0f. Read the comment docs.

oppiabot · 2018-12-28T07:05:30Z

Hi @prasanna08, I'm going to mark this PR as stale because it hasn't had any updates for 10 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

seanlip · 2019-01-04T14:33:11Z

Sorry, this needed to be reviewed by me. Reopening, and will review shortly.

seanlip

Hi @prasanna08, sorry for the delay in reviewing this. Just two docstring fixes but otherwise LGTM and feel free to merge. Thanks!

seanlip · 2019-01-04T14:39:47Z

core/tests/test_utils.py

@@ -930,6 +999,71 @@ def save_new_exp_with_states_schema_v0(self, exp_id, user_id, title):
        exploration = exp_services.get_exploration_from_model(exp_model)
        exp_services.create_and_save_state_id_mapping_model(exploration, [])

+    def save_new_exp_with_states_schema_v21(self, exp_id, user_id, title):


Can you explain in the docstring the significance of version 21? Just a note at the end of the first paragraph saying "Version 21 is where XXX is implemented." will do.

seanlip · 2019-01-04T14:40:14Z

core/domain/classifier_services.py

+
+    Args:
+        exploration: Exploration. Exploration domain object.
+        exploration_to_revert_to: Exploration. Exploration to which revert to.


Extra "which": Exploration to revert to.

prasanna08 added 3 commits October 21, 2018 09:41

Fix classifier training jobs when exploration is reverted.

c205d56

test classifier verification test for auto migration of exploration.

1980fe6

Merge branch 'develop' of github.com:oppia/oppia into fix_exp_reverts…

1df9ac0

…_classifier_training_job

prasanna08 requested a review from seanlip November 18, 2018 13:54

seanlip requested changes Nov 19, 2018

View reviewed changes

oppiabot bot added the stale label Dec 5, 2018

oppiabot bot closed this Dec 12, 2018

prasanna08 added 2 commits December 17, 2018 16:51

Merge branch 'develop' of github.com:oppia/oppia into fix_exp_reverts…

e13ea44

…_classifier_training_job

Update test case

f2a7a22

prasanna08 reopened this Dec 17, 2018

prasanna08 requested a review from apb7 as a code owner December 17, 2018 12:34

oppiabot bot removed stale labels Dec 17, 2018

prasanna08 added 2 commits December 17, 2018 18:29

Addressed review comments

9cf1172

Add test case for exploration revert

372cf90

oppiabot bot added the stale label Dec 28, 2018

oppiabot bot closed this Jan 4, 2019

seanlip reopened this Jan 4, 2019

oppiabot bot removed stale labels Jan 4, 2019

seanlip approved these changes Jan 4, 2019

View reviewed changes

kevinlee12 assigned prasanna08 Jan 5, 2019

Addressed review comments.

4e78a0f

seanlip merged commit becb800 into oppia:develop Jan 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix classifier models when exploration is reverted and add test case for auto migrations #5888

Fix classifier models when exploration is reverted and add test case for auto migrations #5888

prasanna08 commented Nov 18, 2018

seanlip left a comment

seanlip Nov 18, 2018

prasanna08 Nov 20, 2018

seanlip Nov 18, 2018

prasanna08 Nov 20, 2018

seanlip Nov 20, 2018

prasanna08 Dec 17, 2018

prasanna08 commented Nov 25, 2018 •

edited

Loading

oppiabot bot commented Dec 5, 2018

codecov-io commented Dec 17, 2018

codecov-io commented Dec 17, 2018 •

edited

Loading

oppiabot bot commented Dec 28, 2018

seanlip commented Jan 4, 2019

seanlip left a comment

seanlip Jan 4, 2019

prasanna08 Jan 6, 2019

seanlip Jan 4, 2019

prasanna08 Jan 6, 2019

Fix classifier models when exploration is reverted and add test case for auto migrations #5888

Fix classifier models when exploration is reverted and add test case for auto migrations #5888

Conversation

prasanna08 commented Nov 18, 2018

seanlip left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasanna08 commented Nov 25, 2018 • edited Loading

oppiabot bot commented Dec 5, 2018

codecov-io commented Dec 17, 2018

Codecov Report

codecov-io commented Dec 17, 2018 • edited Loading

Codecov Report

oppiabot bot commented Dec 28, 2018

seanlip commented Jan 4, 2019

seanlip left a 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

prasanna08 commented Nov 25, 2018 •

edited

Loading

codecov-io commented Dec 17, 2018 •

edited

Loading