Fix part of #122: Add a proto for explorations #128

vinitamurthi · 2019-09-17T18:11:57Z

Explanation

This PR creates a proto for handling an exploration instance. It will be used by the exploration controllers and views
Fixes part of #115

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is assigned to an appropriate reviewer.

model/src/main/proto/subtitled_html.proto

BenHenning

Thanks! Overall LGTM, but I'd like to do one more pass after these comments are addressed.

model/src/main/proto/exploration.proto

BenHenning · 2019-09-18T05:46:01Z

model/src/main/proto/exploration.proto

+
+// Data for a single rule spec
+message RuleSpec {
+  map<string, HtmlStringList> inputs = 1;


Is HtmlStringList the only type for inputs, or can it be something else? It appears this was represented with 'Any' in the original structure.

Looking at the file the comment - https://github.com/oppia/oppia/blob/ec254cfcc87843c36e24d68d335cc811536fb2d4/core/domain/state_domain.py#L1146 points to, I believe they are a list of html strings

Per https://github.com/oppia/oppia/blob/develop/extensions/interactions/rule_templates.json I think the inputs are typed based on the rule spec. E.g. some can be CodeString, others Fraction, or list of HTML strings. I think for the interactions we're supporting (#28, #29, #30, #31, #32, #33, #34, #35) we should support the corresponding rulespecs for them. Should we be using 'anyof' here, instead?

I have added them as part of the InteractionObject message. Added the new types of input based on interactions mentioned in #28 -> #35

BenHenning · 2019-09-18T05:46:34Z

model/src/main/proto/exploration.proto

+// Maps from: data/src/main/java/org/oppia/data/backends/gae/model/GaeCustomizationArgs.kt
+message CustomizationArgs {
+  bool parse_with_jinja = 1;
+  string value = 2;


Is value always type string? This was 'Any' in the GAE model.

Based on a search of the codebase - https://github.com/oppia/oppia/search?l=YAML&p=2&q=customization_args
I could see that each of the customization args have a value field that is a string. Not sure if we need anything else apart from the value field?
@seanlip is there a way to find out all the possible customization args we can get?

Hi @vinitamurthi -- "customization args" is an overloaded term. They exist for interactions, parameters, and a whole bunch of other things.

For interactions: see extensions/interaction/NameOfInteraction and look at the python file. There's a list of customization arg descriptions in there, with their possible schemas.

For parameters, it's defined here: https://github.com/oppia/oppia/blob/develop/core/domain/param_domain.py#L99

Separated customization args for param changes and interactions.
For interactions I noticed this comment: https://github.com/oppia/oppia/blob/1b668fd3c74f9481232a3c1c73ecd632036a902e/core/domain/state_domain.py#L340 where its a map from name to a default value.
Default value can be different types so I looked at the different possible values in extensions/interaction/NameOfInteraction and added them to an InteractionObject message

BenHenning · 2019-09-20T08:12:21Z

Hi @vinitamurthi is this ready for another review? If so, please reassign to me so that I know. Thanks!

vinitamurthi · 2019-09-20T18:34:51Z

This is now ready for a second pass..PTAL @BenHenning !

BenHenning · 2019-09-20T19:37:52Z

model/src/main/proto/exploration.proto

+  Hint hint = 5;
+  Outcome default_outcome = 6;
+  // Mapping from the name of a customization arg to the default interaction
+  // object  of the customization arg


Nit: remove extra space between 'object' and 'of'

BenHenning · 2019-09-20T20:08:06Z

model/src/main/proto/exploration.proto

+
+// Data for a single rule spec
+message RuleSpec {
+  map<string, HtmlStringList> inputs = 1;


Per https://github.com/oppia/oppia/blob/develop/extensions/interactions/rule_templates.json I think the inputs are typed based on the rule spec. E.g. some can be CodeString, others Fraction, or list of HTML strings. I think for the interactions we're supporting (#28, #29, #30, #31, #32, #33, #34, #35) we should support the corresponding rulespecs for them. Should we be using 'anyof' here, instead?

BenHenning · 2019-09-20T20:08:51Z

model/src/main/proto/interaction_object.proto

+}
+
+// Structure representing a graph object.
+message Graph {


See above--let's only focus on the prototype interactions for now: #28->#35.

BenHenning · 2019-09-20T20:09:19Z

model/src/main/proto/interaction_object.proto

+option java_multiple_files = true;
+
+message InteractionObject {
+  oneof object {


Maybe: oneof object_type. Thoughts?

I have added an object type, so changed this field to value - what do you think?

This wasn't quite what I meant--followed up below. Let me know if you want to discuss further.

…lone + review fixes

vinitamurthi · 2019-09-20T21:06:20Z

@BenHenning made the changes you requested, PTAL!

BenHenning

Thanks @vinitamurthi--after my last set of comments are addressed this looks good to me. You can re-assign to me if you want me to take one more pass, but otherwise if you address my comments feel free to submit this.

BenHenning · 2019-09-20T23:10:11Z

model/src/main/proto/interaction_object.proto

+// Structure for any interaction object
+message InteractionObject {
+  ObjectType object_type = 1;
+  oneof value {


Actually, I was wondering if we could call the oneof object_type. Do we need a separate enum? I thought oneof generated an enum for us to switch on & retrieve the value. If that's right, we don't need the extra enum and it's a bit redundant.

Ah oops I misunderstood. Changed it

BenHenning · 2019-09-20T23:11:23Z

model/src/main/proto/interaction_object.proto

+  }
+}
+
+// Structure containing lists. Note we should fill only


Maybe instead we can just make this a StringList. At that point, the extra caveat in the documentation doesn't seem necessary since it's probably sufficiently clear that the purpose of the proto is to be a StringList.

BenHenning · 2019-09-20T23:12:44Z

model/src/main/proto/interaction_object.proto

+  ObjectType number_type = 1;
+  float real = 2;
+  Fraction fraction = 3;
+  // Controllers are expected to convert the unit into a string


This is actually a domain object, not a view model object, so we could conceivably have different model objects that contain UI-specific data. We should model these protos based on what we need to store on-disk. If units are already stored as strings in Oppia, then let's keep this. Otherwise, let's use a type closer to what the backend uses.

Sounds good..based on the schema here - https://github.com/oppia/oppia/blob/ae11e9dbeac66b5faeb68b7a2b1f16ccfdbc5ba6/extensions/objects/models/objects.py#L937
I believe there is a string value and an int value

I confirm there is indeed a string value and an int value, but note that a "Units" object is a list of (string, int) things.

For example, "kg m s^-2" is represented as [(kg, 1), (m, 1), (s, -2)] (but with dicts instead of tuples.

BenHenning · 2019-09-20T23:13:27Z

model/src/main/proto/interaction_object.proto

+
+// Structure for a number with units object.
+message NumberWithUnits {
+  ObjectType number_type = 1;


Could this maybe instead be a oneof between the float & Fraction? I don't think we'd ever have a NumberWithUnits containing a list of strings, for instance.

vinitamurthi · 2019-09-20T23:38:14Z

@BenHenning I made the changes and I am merging this now

vinitamurthi added 2 commits September 15, 2019 20:11

New proto for explorations

ed440bd

Convert any protos to a particular value

3bc3ae5

vinitamurthi requested review from seanlip and BenHenning September 17, 2019 18:11

vinitamurthi assigned BenHenning Sep 17, 2019

seanlip reviewed Sep 17, 2019

View reviewed changes

model/src/main/proto/subtitled_html.proto Outdated Show resolved Hide resolved

Add new line to EOF in subtitled_html.proto

c7be772

BenHenning requested changes Sep 18, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Sep 18, 2019

vinitamurthi added 2 commits September 18, 2019 15:55

Review changes

954a622

Lint fix for the comment

c661a43

BenHenning changed the title ~~Fix part of #115: Add a proto for explorations~~ Fix part of #122: Add a proto for explorations Sep 20, 2019

Change customization args for interactions and param changes

723503c

vinitamurthi assigned BenHenning and unassigned vinitamurthi Sep 20, 2019

BenHenning requested changes Sep 20, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Sep 20, 2019

Updated interaction objects to support the ones for the android app a…

a192ac1

…lone + review fixes

vinitamurthi assigned BenHenning and unassigned vinitamurthi Sep 20, 2019

Added comments for some of the objects

267c33a

BenHenning approved these changes Sep 20, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Sep 20, 2019

Review fixes

363b0db

Removed ObjectType enum

93f8825

vinitamurthi merged commit 8fb73eb into develop Sep 21, 2019

BenHenning mentioned this pull request Sep 23, 2019

WIP part of #161: Exploration player base 2 contentcard #125

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #122: Add a proto for explorations #128

Fix part of #122: Add a proto for explorations #128

vinitamurthi commented Sep 17, 2019

BenHenning left a comment

BenHenning Sep 18, 2019

vinitamurthi Sep 18, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 18, 2019

vinitamurthi Sep 18, 2019

seanlip Sep 19, 2019

vinitamurthi Sep 20, 2019

BenHenning commented Sep 20, 2019

vinitamurthi commented Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi commented Sep 20, 2019

BenHenning left a comment

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

seanlip Sep 20, 2019

BenHenning Sep 20, 2019

vinitamurthi Sep 20, 2019

vinitamurthi commented Sep 20, 2019

Fix part of #122: Add a proto for explorations #128

Fix part of #122: Add a proto for explorations #128

Conversation

vinitamurthi commented Sep 17, 2019

Explanation

Checklist

BenHenning 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Sep 20, 2019

vinitamurthi commented Sep 20, 2019

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

vinitamurthi commented Sep 20, 2019

BenHenning 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Sep 20, 2019