Fixes for end to end working between Oppia and Oppia-ml. #3824

prasanna08 · 2017-08-28T14:47:01Z

This PR introduces some fixes that we missed during implementing communication interface between Oppia and Oppia-ml

prasanna08 · 2017-08-28T14:48:16Z

core/controllers/classifier.py

-        signature = self.payload.get('signature')
-        message = self.payload.get('message')
-        vm_id = self.payload.get('vm_id')
+        payload = json.loads(self.request.body)


@seanlip when using requests module to make request, self.payload does not get initialized by GAE. Therefore I have to do it manually here. Do you have some idea, why it is like that?

Actually the initialization of self.payload happens in core.controllers.base (at the bottom of __init__). Maybe the request dict doesn't pack everything into a payload like we do for the Oppia client/server communication? If not, it probably should...

I didn't know that. Let me check how to fix it.

Fixed this.

prasanna08 · 2017-08-28T14:48:46Z

core/controllers/classifier.py

@@ -133,11 +135,11 @@ class NextJobHandler(base.BaseHandler):
    REQUIRE_PAYLOAD_CSRF_CHECK = False

    @acl_decorators.open_access
-    def post(self):


This was meant to be GET. How did we miss this? The Oppia-ml Extension describes this as a GET method.

Actually I think if it changes status of models in backend, it should be a POST...

Reverted to POST. I'll change Oppia-ml Extension accordingly.

seanlip · 2017-08-28T19:15:56Z

core/controllers/classifier.py

-        signature = self.payload.get('signature')
-        message = self.payload.get('message')
-        vm_id = self.payload.get('vm_id')
+        payload = json.loads(self.request.body)


Actually the initialization of self.payload happens in core.controllers.base (at the bottom of __init__). Maybe the request dict doesn't pack everything into a payload like we do for the Oppia client/server communication? If not, it probably should...

seanlip · 2017-08-28T19:18:00Z

core/controllers/classifier_test.py

-                                       self.payload, expect_errors=False,
-                                       expected_status_int=200)
+        json_response = self.get_json(
+            '/ml/nextjobhandler', self.payload, expect_errors=False)


Why no check on status_int?

seanlip · 2017-08-28T19:19:44Z

core/controllers/classifier.py

@@ -133,11 +135,11 @@ class NextJobHandler(base.BaseHandler):
    REQUIRE_PAYLOAD_CSRF_CHECK = False

    @acl_decorators.open_access
-    def post(self):


Actually I think if it changes status of models in backend, it should be a POST...

codecov-io · 2017-08-29T01:29:20Z

Codecov Report

Merging #3824 into develop will increase coverage by <.01%.
The diff coverage is 22.22%.

@@             Coverage Diff             @@
##           develop    #3824      +/-   ##
===========================================
+ Coverage    46.01%   46.01%   +<.01%     
===========================================
  Files          279      279              
  Lines        20906    20907       +1     
  Branches      3278     3278              
===========================================
+ Hits          9619     9620       +1     
  Misses       11287    11287

Impacted Files	Coverage Δ
...interactions/CodeRepl/CodeReplPredictionService.js	`53.43% <22.22%> (+0.35%)`	⬆️

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 bc577c6...74027df. Read the comment docs.

prasanna08 · 2017-08-29T01:35:22Z

@seanlip as per my recent commit, no change is needed on Oppia side. We can discard this PR or merge it (it also adds a test exploration for code classifier end to end testing which may be useful to us in future).

seanlip · 2017-08-29T02:24:41Z

OK, sounds good. Happy either way; your choice. Please ask @anmolshkl to review as I will be travelling -- thanks!

anmolshkl · 2017-08-29T06:24:34Z

core/controllers/classifier_test.py

-                                       self.payload, expect_errors=False,
-                                       expected_status_int=200)
+        json_response = self.post_json(
+            r'/ml/nextjobhandler', self.payload, expect_errors=False)


why raw string literals here and on line 176 but not at other places?

My bad. It really does not matter whether we use raw strings or not. We can keep it or remove it, your call?

@prasanna08 let's not block on this. You pick. Just be consistent within these tests and with other tests.

seanlip · 2017-08-29T08:41:38Z

core/controllers/classifier_test.py

-                                       self.payload, expect_errors=False,
-                                       expected_status_int=200)
+        json_response = self.post_json(
+            '/ml/nextjobhandler', self.payload, expect_errors=False)


Why are we getting rid of expected_status_int, here and below? Please check the diff to verify that the changes are exactly what you want them to be, and no more.

Oh, my bad. When I changed POST to GET I removed them and forgot to add them back. I will add them.

seanlip · 2017-08-29T08:42:26Z

core/controllers/classifier.py

@@ -99,6 +99,7 @@ def post(self):
        signature = self.payload.get('signature')
        message = self.payload.get('message')
        vm_id = self.payload.get('vm_id')
+


Please remove if this isn't an intentional part of the PR (make sure to check the diff on GitHub).

prasanna08 · 2017-08-29T08:54:55Z

@seanlip I've also pushed some fixes in CodeReplPredictionService. It was raising exception because this keyword was not working when calling functions defined in same service. So I just defined a variable for the object (which we were returning directly before) and using name of that variable to call functions defined in same object.

@seanlip @anmolshkl @AllanYangZhou with this last commit entire Oppia => Oppia-ml => prediction service workflow is working.

anmolshkl

@prasanna08 LGTM!

…t-m2 * upstream/develop: (27 commits) Fixes 3687: batch call for exploration and exploration rights object together. (oppia#3815) Fix part oppia#3400 Objective field directive (oppia#3740) Fix oppia#3800 Dropdown menu of the ABOUT in navigation bar (oppia#3855) Fix oppia#3295 Save draft change list to local storage. (oppia#3584) Disable learner dashboard feedback updates send button when no text entered. (oppia#3860) Disable save button while uploading audio to server; add Spanish support to audio languages; add audio loading message to learner view (oppia#3856) Fix oppia#3834: Introduce backend event models to record statistics (oppia#3841) Fix oppia#3852: Fix failing e2e test in develop (oppia#3853) Fix oppia#3826: Move validatorsService from app.js to ValidatorsService.js (oppia#3847) Update the max RAND limit for ID generation to fit within 32 bits. (oppia#3843) Fix oppia#3404: Add default placeholder text for mobile devices (oppia#3845) Fix oppia#2553: Change the "index all explorations" job in the admin dashb… (oppia#3831) Add I18N to give up button; change tooltip text. (oppia#3844) Fix oppia#3721: Update the release_info script to use LCA and the most recent release tag in order to compile the list of changes, rather than relying on git describe. (oppia#3838) show_warning_only_on_button_click (oppia#3833) Deprecate splash page experiment (oppia#3829) Fixes for end to end working between Oppia and Oppia-ml. (oppia#3824) Fix 'Empty path passed in method' error on the collection page. (oppia#3827) Learner dashboard 3.2 (oppia#3759) 📝 Fix oppia#3789 :Space out the profile drop down icons (oppia#3789). (oppia#3817) ...

FIxes for end to end working between Oppia and Oppia-ml.

ff49228

prasanna08 commented Aug 28, 2017

View reviewed changes

Disable ML facilities.`

8ac9994

seanlip reviewed Aug 28, 2017

View reviewed changes

Revert classifier.py file handlers.

4c7985d

Disable ML facility.

7e840f1

anmolshkl reviewed Aug 29, 2017

View reviewed changes

Remove raw string literals from classifier_test

c619101

seanlip reviewed Aug 29, 2017

View reviewed changes

prasanna08 added 2 commits August 29, 2017 14:15

Remove unnecessary changes.

a1afcc0

Fix code repl prediction service.

74027df

anmolshkl approved these changes Aug 29, 2017

View reviewed changes

pranavsid98 added the PR: LGTM label Aug 30, 2017

pranavsid98 assigned prasanna08 Aug 30, 2017

seanlip merged commit 135ac4e into oppia:develop Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for end to end working between Oppia and Oppia-ml. #3824

Fixes for end to end working between Oppia and Oppia-ml. #3824

prasanna08 commented Aug 28, 2017

prasanna08 Aug 28, 2017

seanlip Aug 28, 2017

prasanna08 Aug 29, 2017

prasanna08 Aug 29, 2017

prasanna08 Aug 28, 2017

seanlip Aug 28, 2017

prasanna08 Aug 29, 2017

seanlip Aug 28, 2017

seanlip Aug 28, 2017

seanlip Aug 28, 2017

codecov-io commented Aug 29, 2017 •

edited

Loading

prasanna08 commented Aug 29, 2017

seanlip commented Aug 29, 2017

anmolshkl Aug 29, 2017

prasanna08 Aug 29, 2017

seanlip Aug 29, 2017 •

edited

Loading

prasanna08 Aug 29, 2017

seanlip Aug 29, 2017 •

edited

Loading

prasanna08 Aug 29, 2017

seanlip Aug 29, 2017

prasanna08 Aug 29, 2017

prasanna08 Aug 29, 2017

prasanna08 commented Aug 29, 2017 •

edited

Loading

anmolshkl left a comment

Fixes for end to end working between Oppia and Oppia-ml. #3824

Fixes for end to end working between Oppia and Oppia-ml. #3824

Conversation

prasanna08 commented Aug 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 29, 2017 • edited Loading

Codecov Report

prasanna08 commented Aug 29, 2017

seanlip commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Aug 29, 2017 • 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 commented Aug 29, 2017 • edited Loading

anmolshkl left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 29, 2017 •

edited

Loading

seanlip Aug 29, 2017 •

edited

Loading

seanlip Aug 29, 2017 •

edited

Loading

prasanna08 commented Aug 29, 2017 •

edited

Loading