Add priority groups to local paths in the dependency manager.

tools/telemetry/catapult_base/dependency_manager/dependency_info.py

Index: tools/telemetry/catapult_base/dependency_manager/dependency_info.py
diff --git a/tools/telemetry/catapult_base/dependency_manager/dependency_info.py b/tools/telemetry/catapult_base/dependency_manager/dependency_info.py
index c2c860281add26c8af299d536c77bda58a934f60..4714c23ce40c14d6a191f5dcea532aa0396aa1fa 100644
--- a/tools/telemetry/catapult_base/dependency_manager/dependency_info.py
+++ b/tools/telemetry/catapult_base/dependency_manager/dependency_info.py
@@ -2,7 +2,6 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-
 class DependencyInfo(object):
   def __init__(self, dependency, platform, config_file, cs_bucket=None,
                cs_hash=None, download_path=None, cs_remote_path=None,
@@ -40,13 +39,13 @@ class DependencyInfo(object):
     self._dependency = dependency
     self._platform = platform
     self._config_files = [config_file]
-    self._local_paths = local_paths or []
+    self._local_paths = self._ParseLocalPaths(local_paths)

[nednguyen, 2015/10/09 16:20:07]

self._local_paths_groups

[aiolos (Not reviewing), 2015/10/09 16:52:00]

See my response in the comment on _ParseLocalPaths for context.

     self._download_path = download_path
     self._cs_remote_path = cs_remote_path
     self._cs_bucket = cs_bucket
     self._cs_hash = cs_hash
     self._version_in_cs = version_in_cs
-    self.VerifyCloudStorageInfo()
+    self._VerifyCloudStorageInfo()
 
   def Update(self, new_dep_info):
     """Add the information from |new_dep_info| to this instance.
@@ -74,9 +73,16 @@ class DependencyInfo(object):
         self._cs_hash = new_dep_info.cs_hash
         self._version_in_cs = new_dep_info.version_in_cs
     if new_dep_info.local_paths:
-      for path in new_dep_info.local_paths:
-        if path not in self._local_paths:
-          self._local_paths.append(path)
+      for priority_group in new_dep_info.local_paths:
+        group_list = []
+        for path in priority_group:
+          if not self.IsPathInLocalPaths(path):
+            group_list.append(path)
+        if group_list:
+          self._local_paths.append(group_list)
+
+  def IsPathInLocalPaths(self, path):
+    return any(path in priority_group for priority_group in self._local_paths)
 
   @property
   def dependency(self):
@@ -116,10 +122,22 @@ class DependencyInfo(object):
 
   @property
   def has_cs_info(self):
-    self.VerifyCloudStorageInfo()
+    self._VerifyCloudStorageInfo()
     return self.cs_hash
 
-  def VerifyCloudStorageInfo(self):
+  @staticmethod
+  def _ParseLocalPaths(local_paths):

[nednguyen, 2015/10/09 16:20:07]

We should restrict the type of local_paths to list of list. Th rule that if this
is a list of path strings, then each path is transformed into a priority group
with single path is surprising.

[aiolos (Not reviewing), 2015/10/09 16:52:00]

I'm not sure why that's surprising, since priority groups are an abstraction
within the dependency manager that keeps the current behavior with flat
local_paths lists. Priority groups are meant to be an optional feature, and
should not force people to use them in their configs if they don't want to. It's
also why I kept the name "local_files" instead of changing it to
"local_file_priority_groups" or similar.

In the json files:

['1', '2', '3'] means files at path 1, 2 and 3 will all be checked in order.
[['1', '2', '3']] means that files at paths 1, 2 and 3 should all be checked and
the most recently changed one should be used.
['1', ['2', '3']] or [['1'], ['2', '3']] mean that the file at path 1 should be
used if it's available, otherwise use path 2 or 3 depending on which one is
newer.


If we changed it, that would mean forcing the json for the first case to look
like this instead, which seems over complicated. (Note that this example still
works with the current implementation, it just isn't required.)

[['1'], ['2'], ['3']]


If we want to force users to always put files in priority groups for Telemetry,
which I think is questionable, we should be doing so at the config level, not
the dependency_info level. That will allow future configs to specify either
always or never using the priority groups if they so desire.

[nednguyen, 2015/10/09 17:04:04]

The fact that priority groups is an optional feature to users is orthogonal to
the issue of naming "local_paths" variable. If I see
"dependency_info.local_files" in code, how would I suppose to expect that the
variable 'local_files' is meant to be a list of list of files? 

You can keep the "local_paths" argument unchanged in the constructor of
DependencyInfo & add documentation explaining the extra meaning. However the
member method "local_paths" of DependencyInfo should be renamed to reflect that
it's not a simple list of paths.
Hmm, this seems reasonable. But can you update the docstring of BaseConfig to
also reflect this?
 

+    if not local_paths:
+      return []
+    list_of_lists = []
+    for element in local_paths:
+      if isinstance(element, basestring):
+        list_of_lists.append([element])
+      else:
+        list_of_lists.append(element)
+    return list_of_lists
+
+  def _VerifyCloudStorageInfo(self):
     """Ensure either all or none of the needed remote information is specified.
     """
     if ((self.cs_bucket or self.cs_remote_path or self.download_path or