Add priority groups to local paths in the dependency manager.

Add priority groups to local paths in the dependency manager.

BUG=541441

[aiolos (Not reviewing), 2015-10-09 02:33:54]

aiolos@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com

[nednguyen, 2015-10-09 04:44:30]

Making local_paths in dependency_info a list of list seems way too overkill to
me. Can't we just make all the local paths be in the same priority & break tie
by last modified time?

[aiolos (Not reviewing), 2015-10-09 15:08:33]

On 2015/10/09 04:44:30, nednguyen wrote:
> Making local_paths in dependency_info a list of list seems way too overkill to
> me. Can't we just make all the local paths be in the same priority & break tie
> by last modified time?

You're making the assumption that all of the files should have the same
priority, which I don't think can be made. Examples:

What if they aren't from the same config? What if we had a local file path in a
project that should always be overridden by one given via a client config if it
exists?

What if a project specifies a fall-back default file that should only be used if
the more up to date ones aren't available, but it brings it in via isolates or
some other process that can create an artificially new modification date? 

Or if we have files that are being specified via command line and then put in a
config? We wouldn't want to default back to the default one just because it's
newer, because that would break many assumptions about how flags work.

Also note that it's written in such a way that configs can still just have a
list of file paths and it will still work. They will just have the option of
using an internal list to denote a priority group. I agree that it's a super
user feature, but that doesn't mean we shouldn't have it. (And remember that the
dependency manager isn't just for Telemetry; it's a stand-alone project. So just
because Telemetry doesn't mind all of the files being the same priority doesn't
mean that we should make that design assumption.)

[nednguyen, 2015-10-09 15:24:14]

On 2015/10/09 15:08:33, aiolos wrote:
> On 2015/10/09 04:44:30, nednguyen wrote:
> > Making local_paths in dependency_info a list of list seems way too overkill
to
> > me. Can't we just make all the local paths be in the same priority & break
tie
> > by last modified time?
> 
> You're making the assumption that all of the files should have the same
> priority, which I don't think can be made. Examples:
> 
> What if they aren't from the same config? 

Then probably different configs shouldn't specify same dependency. Allow many
layers of configs with the same dependencies is a formula for headache. 

> What if we had a local file path in a
> project that should always be overridden by one given via a client config if
it
> exists?
> 
> What if a project specifies a fall-back default file that should only be used
if
> the more up to date ones aren't available, but it brings it in via isolates or
> some other process that can create an artificially new modification date? 

I am not convinced this is a good use case to support. A binary is either
runnable by your program or not. If there is a newer version of binary, then
build it & update the cloud storage link. 
If the binary need to be compile 

> 
> Or if we have files that are being specified via command line and then put in
a
> config? We wouldn't want to default back to the default one just because it's
> newer, because that would break many assumptions about how flags work.

This is another pattern should be avoided. No files should be specified through
both commandline & config

> 
> Also note that it's written in such a way that configs can still just have a
> list of file paths and it will still work. They will just have the option of
> using an internal list to denote a priority group. I agree that it's a super
> user feature, but that doesn't mean we shouldn't have it. (And remember that
the
> dependency manager isn't just for Telemetry; it's a stand-alone project. So
just
> because Telemetry doesn't mind all of the files being the same priority
doesn't
> mean that we should make that design assumption.)

[nednguyen, 2015-10-09 16:03:12]

nednguyen@google.com changed reviewers:
+ jbudorick@chromium.org

[nednguyen, 2015-10-09 16:03:12]

+John since you need priority groups feature.

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

Chatted offline and devil needs the priority group feature. Please ignore my
comments against it.

Don't you also need to update BaseConfig & the json format so "file_paths" -->
"file_paths_groups" ?

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
File tools/telemetry/catapult_base/dependency_manager/dependency_info.py
(right):

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:30:
local_paths: A list of paths to search in order for a local file.
Docstring needs update

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:42:
self._local_paths = self._ParseLocalPaths(local_paths)
self._local_paths_groups

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:100: def
local_paths(self):
This should be local_paths_groups

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:129: def
_ParseLocalPaths(local_paths):
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.

[eakuefner, 2015-10-09 16:29:17]

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
File tools/telemetry/catapult_base/dependency_manager/dependency_manager.py
(right):

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_manager.py:225:
mtime = os.stat(path).st_mtime
mtime = os.path.getmtime(path)

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
File
tools/telemetry/catapult_base/dependency_manager/dependency_manager_unittest.py
(right):

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_manager_unittest.py:669:
# There is no local path for the given dependency.
Can you put each of these cases into a separate unit test, having names that
match what's given in these comments? It seems like having so many different
cases lumped together will make repairing breakages more painful than if each of
these were a separate test.

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

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
File tools/telemetry/catapult_base/dependency_manager/dependency_info.py
(right):

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:30:
local_paths: A list of paths to search in order for a local file.
On 2015/10/09 16:20:07, nednguyen wrote:
> Docstring needs update

Agreed, but I'm going to wait until the form of the CL is finalized before
updating it.

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:42:
self._local_paths = self._ParseLocalPaths(local_paths)
On 2015/10/09 16:20:07, nednguyen wrote:
> self._local_paths_groups

See my response in the comment on _ParseLocalPaths for context.

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:100: def
local_paths(self):
On 2015/10/09 16:20:07, nednguyen wrote:
> This should be local_paths_groups

Same.

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:129: def
_ParseLocalPaths(local_paths):
On 2015/10/09 16:20:07, nednguyen wrote:
> 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.

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]

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
File tools/telemetry/catapult_base/dependency_manager/dependency_info.py
(right):

https://codereview.chromium.org/1391403003/diff/1/tools/telemetry/catapult_ba...
tools/telemetry/catapult_base/dependency_manager/dependency_info.py:129: def
_ParseLocalPaths(local_paths):
On 2015/10/09 16:52:00, aiolos wrote:
> On 2015/10/09 16:20:07, nednguyen wrote:
> > 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.
> 
> 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.

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.

> 
> 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.

Hmm, this seems reasonable. But can you update the docstring of BaseConfig to
also reflect this?
 
> 
> 
> 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.