Handles default revision ranges.

Handles default revision ranges.

If the user doesn't supply point ids, defaults to most recent milestone point id thru most recent point id. Also display a special format for downstream bots.

Demo (must be logged in): https://dev-jessimb-ec068d40-dot-chromeperf.appspot.com/speed_releasing/MemoryTest7

http://i.imgur.com/noqzeaI.png

BUG=catapult:#3141

[jessimb, 2017-01-26 21:54:49]

Description was changed from

==========
Creates a clank friendly commit revision to display.


BUG=catapult:#3141
==========

to

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.


BUG=catapult:#3141
==========

[jessimb, 2017-01-26 22:54:39]

Description was changed from

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.


BUG=catapult:#3141
==========

to

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-f9754180-dot-chromeperf.appspot.com/speed_releasing/Memor...


BUG=catapult:#3141
==========

[jessimb, 2017-01-27 18:53:42]

Description was changed from

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-f9754180-dot-chromeperf.appspot.com/speed_releasing/Memor...


BUG=catapult:#3141
==========

to

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-f9754180-dot-chromeperf.appspot.com/speed_releasing/Memor...

http://i.imgur.com/wvHOcot.png

BUG=catapult:#3141
==========

[jessimb, 2017-01-27 18:57:10]

Description was changed from

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-f9754180-dot-chromeperf.appspot.com/speed_releasing/Memor...

http://i.imgur.com/wvHOcot.png

BUG=catapult:#3141
==========

to

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-cabb717c-dot-chromeperf.appspot.com/speed_releasing/Memor...

http://i.imgur.com/wvHOcot.png

BUG=catapult:#3141
==========

[jessimb, 2017-02-04 00:07:35]

Description was changed from

==========
Creates a clank friendly commit revision to display.

When using the 'ClankInternal' master, we do r_commit_pos - r_clank[:3]. If the
user doesn't supply point ids, defaults to most recent milestone point id thru
most recent point id.

Demo (must be logged in):
https://dev-jessimb-cabb717c-dot-chromeperf.appspot.com/speed_releasing/Memor...

http://i.imgur.com/wvHOcot.png

BUG=catapult:#3141
==========

to

==========
Handles default revision ranges.

If the user doesn't supply point ids, defaults to most recent milestone point id
thru most recent point id. Also display a special format for downstream bots.

Demo (must be logged in):
https://dev-jessimb-ec068d40-dot-chromeperf.appspot.com/speed_releasing/Memor...

http://i.imgur.com/noqzeaI.png

BUG=catapult:#3141
==========

[jessimb, 2017-02-04 00:11:21]

jessimb@chromium.org changed reviewers:
+ eakuefner@chromium.org

[jessimb, 2017-02-04 00:11:22]

PTAL. This handles default milestones and renaming point ids to r_commit_pos.
Look for a future CL to allow us to navigate between the milestones.

https://codereview.chromium.org/2657043002/diff/60001/dashboard/dashboard/spe...
File dashboard/dashboard/speed_releasing.py (right):

https://codereview.chromium.org/2657043002/diff/60001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:207: if row and hasattr(row,
'r_commit_pos'): # Rule out masters like V8
While I want to use a_display_rev here, I will still have to support Rows
without it, so in the end it is much less complex to default to r_commit_pos and
a_default_rev.

[sullivan, 2017-02-08 22:19:52]

sullivan@chromium.org changed reviewers:
+ sullivan@chromium.org

[sullivan, 2017-02-08 22:19:54]

I think there needs to be better high-level documentation about the
inputs/outputs here. What url params are taken, and what do we show based on
those?

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
File dashboard/dashboard/speed_releasing.py (right):

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:17: CLANK_MILESTONES = {
Can you add a high-level comment explaining what these are?

And it looks like we will just have one set of hard-coded milestones for all
release pages?

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:22: CHROUMIUM_MILESTONES = {
Nit: CHROMIUM_MILESTONES

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:74: master_bot_pairs, masters =
_GetMasterBotPairs(table_entity.bots)
It's confusing to change the return value of this function, and then not update
its name. But then what would the new name be,
"_GetMasterBotPairsAndMasterList"? I think that's a sign that the return value
of the function should be simpler. What about:

master_bot_pairs = _GetMasterBotPairs(table_entity.bots)
masters = set([m.split('/')[0] for m in master_bot_pairs])

If master_bot_pairs instead returned a list of tuples, it could be:
masters = set([m[0] for m in master_bot_pairs])

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:172: def _GetMilestone(masters,
num_from_end=0):
This is only ever called with num_from_end=0, why not just change it to
_GetLastMilestone()?

Also this is getting the revision of the last milestone, not the milestone. The
naming is confusing with _GetNewestRev below, it returns a revision and so does
this.

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:204: for key, value in
milestone_dict.iteritems():
To get rid of the pylint warning:

for _, value

https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
dashboard/dashboard/speed_releasing.py:225: def _GetDefaultRev(rev_a, rev_b,
masters, bots, tests):
This function should be named better, something like
GetMilestoneRevisionsForRevs? I am actually not 100% sure what it's supposed to
do, and why we snap revisions to milestones from the url parameters. Wouldn't we
want to just grab the data between the revisions exactly if they are specified?
And use milestones if milestones are specified?

[jessimb, 2017-02-08 22:51:31]

On 2017/02/08 at 22:19:54, sullivan wrote:
> I think there needs to be better high-level documentation about the
inputs/outputs here. What url params are taken, and what do we show based on
those?
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> File dashboard/dashboard/speed_releasing.py (right):
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:17: CLANK_MILESTONES = {
> Can you add a high-level comment explaining what these are?
> 
> And it looks like we will just have one set of hard-coded milestones for all
release pages?
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:22: CHROUMIUM_MILESTONES = {
> Nit: CHROMIUM_MILESTONES
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:74: master_bot_pairs, masters =
_GetMasterBotPairs(table_entity.bots)
> It's confusing to change the return value of this function, and then not
update its name. But then what would the new name be,
"_GetMasterBotPairsAndMasterList"? I think that's a sign that the return value
of the function should be simpler. What about:
> 
> master_bot_pairs = _GetMasterBotPairs(table_entity.bots)
> masters = set([m.split('/')[0] for m in master_bot_pairs])
> 
> If master_bot_pairs instead returned a list of tuples, it could be:
> masters = set([m[0] for m in master_bot_pairs])
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:172: def _GetMilestone(masters,
num_from_end=0):
> This is only ever called with num_from_end=0, why not just change it to
_GetLastMilestone()?
> 
> Also this is getting the revision of the last milestone, not the milestone.
The naming is confusing with _GetNewestRev below, it returns a revision and so
does this.
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:204: for key, value in
milestone_dict.iteritems():
> To get rid of the pylint warning:
> 
> for _, value
> 
>
https://codereview.chromium.org/2657043002/diff/80001/dashboard/dashboard/spe...
> dashboard/dashboard/speed_releasing.py:225: def _GetDefaultRev(rev_a, rev_b,
masters, bots, tests):
> This function should be named better, something like
GetMilestoneRevisionsForRevs? I am actually not 100% sure what it's supposed to
do, and why we snap revisions to milestones from the url parameters. Wouldn't we
want to just grab the data between the revisions exactly if they are specified?
And use milestones if milestones are specified?

Just a quick note, I have another CL that I built off of this one. During
creating that one, I realized much of this CL needed to be changed. 
https://codereview.chromium.org/2684813002 it's a work in progress and needs to
be split up, but I hope it will address some of your comments (which I have not
read yet)!