Consolidated Change list mapping method #4729

pranavsid98 · 2018-02-23T13:37:00Z

I've added the system for consolidated change list mapping method and added tests too. I've modified the occurrences of where we can use this method too.

I did not modify the GenerateV1Stats job because its already been run in production.

Thanks
Pranav

codecov-io · 2018-02-23T14:25:31Z

Codecov Report

Merging #4729 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4729      +/-   ##
===========================================
+ Coverage    44.66%   44.69%   +0.02%     
===========================================
  Files          384      385       +1     
  Lines        23432    23462      +30     
  Branches      3799     3802       +3     
===========================================
+ Hits         10467    10486      +19     
- Misses       12965    12976      +11

Impacted Files	Coverage Δ
...ditor/editor_tab/InteractionDetailsCacheService.js	`46.66% <0%> (-7.18%)`	⬇️
core/templates/dev/head/directives.js	`22.05% <0%> (-0.33%)`	⬇️
.../exploration_editor/editor_tab/StateInteraction.js	`31.25% <0%> (-0.18%)`	⬇️
.../head/components/forms/Select2DropdownDirective.js	`3.03% <0%> (-0.1%)`	⬇️
...ns/PencilCodeEditor/directives/PencilCodeEditor.js	`32.32% <0%> (ø)`	⬆️
.../templates/dev/head/services/AudioPlayerService.js	`16.12% <0%> (ø)`	⬆️
.../dev/head/components/forms/AudioPlayerDirective.js	`14.28% <0%> (ø)`
...re/templates/dev/head/services/RteHelperService.js	`84.41% <0%> (+0.41%)`	⬆️
...sions/interactions/CodeRepl/directives/CodeRepl.js	`34.78% <0%> (+0.72%)`	⬆️
core/templates/dev/head/filters.js	`70.3% <0%> (+1.5%)`	⬆️

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 d892834...bbea1d9. Read the comment docs.

seanlip

Good start! PTAL at inline comments.

Also shouldn't we be updating other usages of this functionality (like the ML job)? /cc @prasanna08

seanlip · 2018-02-23T23:09:06Z

core/domain/exp_domain.py

+    exploration.
+
+    Attributes:
+        states_added: list(str). The states added to the exploration from


added_state_names, deleted_state_names, new_to_old_state_names

(they're state names, not states -- might want to change the docstrings accordingly e.g. "Names of states added ...")

seanlip · 2018-02-23T23:10:02Z

core/domain/exp_domain.py

+
+        Args:
+            change_list: list(dict). A list of all of the commit cmds from the
+                old_stats_model up to the next version.


old_stats_model? Do you mean the old version of the exploration?

seanlip · 2018-02-23T23:16:58Z

core/domain/exp_domain.py

+            current_exp_version to the state names of prev_exp_version.
+    """
+
+    def __init__(self, change_list):


Don't you also need the state names of the old version of the exploration to seed this (at least as a sanity check that e.g. added names are not already in the list of states, deleted names are already in the list, etc.)?

It would be OK to re-characterize states_renamed as a mapping from states whose "identities" still exist in both the old and new versions, and have a state name map to itself if nothing has happened to it during the change. That way you can keep track of the state changes fully.

@seanlip Not necessarily would we need it, would we? For example, we can't use the old state names as a sanity check for deletion of a state since a state can be renamed and then deleted. This sanity check could only work for addition of states and over engineering for this case might not be required right?

Re the second point, this would introduce a lot more keys in this dict and iterating through it wouldn't be as efficient. While creating this mapping of renames, I didn't see why we'd need to also keep state names that didn't undergo any change. Is there a use case that'd require this?

Re your first paragraph, let's say A gets renamed to B and then gets deleted. I would expect the states_renamed dict to start with A --> A, then become A --> B, and then before deleting B we check that it's either in states_renamed.values() or states_added. I think you do want to take the initial state names into account because otherwise you're kind of assuming things about the exploration based on the changelist. This also acts as a sanity check on our production data.

Re the second paragraph, IMO the efficiency costs in this case would be negligible. I think it's more definitive if you take all states into account since then in the client code that uses this function you won't have to do "if state name is in renamed states then do this". It'd just be "do this". (So I think it would actually be simpler, at least conceptually.)

seanlip · 2018-02-23T23:53:46Z

core/domain/exp_domain.py

+                if state_name in states_added:
+                    states_added.remove(state_name)
+                else:
+                    states_deleted.append(state_name)


Should we remove it from the "rename dict" in this case?

What if a state gets renamed before it is deleted?

(Add backend tests for these cases too.)

True. Fixed it differently, and tests cases added as well.

seanlip · 2018-02-23T23:59:05Z

core/domain/stats_jobs_one_off.py

+            prev_stats_dict['state_stats_mapping'][state_name] = (
+                stats_domain.StateStats.create_default())
+
+        for state_name in exp_versions_diff.states_deleted:


If a state gets renamed before being deleted, does states_deleted hold the new name or the old name of the state? If the former then this won't work.

(It should probably hold the old name if you want to apply the ExplorationVersionsDiff directly to the new object.)

Also would it make more sense for the rename-dict to be keyed by the old state name? That way the algorithm for transferring the data is simpler: delete stuff, then map old state names to new state names by iterating through the rename dict, then add new states.

@seanlip Re the second point, does it make a difference? The data transfer is pretty simple right now as well.

I think keying by old state names is simpler since you're transforming the old exploration into the new one. So you basically have OldExp and a mapping from stuff in OldExp to stuff in NewExp. Having the mapping go the other way round is conceptually a bit weird.

seanlip

Thanks @pranavsid98! A few more comments.

seanlip · 2018-02-27T07:22:45Z

core/domain/exp_domain.py

+                    added_state_names.remove(state_name)
+                else:
+                    original_state_name = state_name
+                    while original_state_name in new_to_old_state_names:


I don't think I understand this. Why do you need a while loop? Isn't a single lookup sufficient?

seanlip · 2018-02-27T08:30:34Z

core/domain/exp_domain.py

-                    old_to_new_state_names[change_dict['old_state_name']] = (
-                        old_to_new_state_names.pop(change_dict[
-                            'new_state_name']))
+        exp_versions_diff = ExplorationVersionsDiff(change_list)


Hi @pranavsid98 -- I thought we chatted about re-examining this method because we noticed it didn't take into account adds and deletes? Could you please check with @prasanna08 about that and verify that its usage is correct? (With your new method it might be possible to simplify this, too, and perhaps even get rid of get_state_names_mapping() altogether.)

seanlip · 2018-02-27T08:32:19Z

core/domain/stats_jobs_one_off.py

-                    change_dict['new_state_name']] = (
-                        prev_stats_dict['state_stats_mapping'].pop(
-                            change_dict['old_state_name']))
+        for state_name in exp_versions_diff.added_state_names:


I think you've got this in the wrong order. What if a changelist renames A --> B then adds A?

Changed order to delete, rename and then add!

seanlip · 2018-02-27T09:52:04Z

core/domain/classifier_services.py

+        old_state_name = current_state_name
+        if current_state_name in (
+                exp_versions_diff.old_to_new_state_names.values()):
+            # The structure of ExplorationVersionsDiff's


Do a single pass outside the for loop to build the reverse mapping here (locally) and use that. Or better still add that as a method to the exp_versions_diff object.

seanlip · 2018-02-27T09:55:05Z

core/domain/exp_domain.py

+            old_state_name = new_state_name
+            if new_state_name in (
+                    exp_versions_diff.old_to_new_state_names.values()):
+                # The structure of ExplorationVersionsDiff's


See above, compute new-to-old first.

seanlip · 2018-02-27T09:57:25Z

core/domain/stats_jobs_one_off.py

@@ -231,21 +231,22 @@ def _apply_state_name_changes(cls, prev_stats_dict, change_list):
            dict. A dict representation of an ExplorationStatsModel
                with updated state_stats_mapping and version.
        """
+        exp_versions_diff = exp_domain.ExplorationVersionsDiff(change_list)
+
        # Handling state additions, deletions and renames.


Add a note here along the lines of "(Note that the order these are applied is important.)" Also change to say "Handling state deletions, renames and additions (in that order)."

seanlip

LGTM. Thanks @pranavsid98!

consolidation

b1ddc57

pranavsid98 requested a review from seanlip February 23, 2018 13:37

seanlip reviewed Feb 23, 2018

View reviewed changes

pranavsid98 added 2 commits February 25, 2018 14:33

addressed first set of review commits

feb65d5

modified ML functionality

17fade9

seanlip reviewed Feb 27, 2018

View reviewed changes

pranavsid98 added 2 commits February 27, 2018 15:20

deprecated get_state_names_mapping method

7d99b09

nit changes

8e81661

seanlip reviewed Feb 27, 2018

View reviewed changes

addressed review changes

bbea1d9

seanlip approved these changes Feb 27, 2018

View reviewed changes

seanlip merged commit b880727 into oppia:develop Feb 27, 2018

pranavsid98 deleted the change_list_consolidation branch February 27, 2018 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidated Change list mapping method #4729

Consolidated Change list mapping method #4729

pranavsid98 commented Feb 23, 2018

codecov-io commented Feb 23, 2018 •

edited

Loading

seanlip left a comment

seanlip Feb 23, 2018

pranavsid98 Feb 25, 2018

seanlip Feb 23, 2018

pranavsid98 Feb 25, 2018

seanlip Feb 23, 2018

pranavsid98 Feb 25, 2018 •

edited

Loading

seanlip Feb 25, 2018 •

edited

Loading

seanlip Feb 23, 2018

pranavsid98 Feb 25, 2018

seanlip Feb 23, 2018

pranavsid98 Feb 25, 2018

seanlip Feb 25, 2018 •

edited

Loading

seanlip left a comment

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip Feb 27, 2018

pranavsid98 Feb 27, 2018

seanlip left a comment

Consolidated Change list mapping method #4729

Consolidated Change list mapping method #4729

Conversation

pranavsid98 commented Feb 23, 2018

codecov-io commented Feb 23, 2018 • edited Loading

Codecov Report

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

pranavsid98 Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

seanlip Feb 25, 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

seanlip Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

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

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

seanlip left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 23, 2018 •

edited

Loading

pranavsid98 Feb 25, 2018 •

edited

Loading

seanlip Feb 25, 2018 •

edited

Loading

seanlip Feb 25, 2018 •

edited

Loading