Issue 2369333002: [Findit] Capture versionized metadata for master_flake_analysis

Issue 2369333002: [Findit] Capture versionized metadata for master_flake_analysis (Closed)

Created:
4 years, 2 months ago by lijeffrey

Modified:
4 years, 2 months ago

Reviewers:
chanli, stgao

CC:
chromium-reviews, infra-reviews+infra_chromium.org

Target Ref:
refs/heads/master

Project:
infra

Visibility:
Public.

More Reviews

Description

[Findit] Capture versionized metadata for master_flake_analysis - Versionize master_flake_analysis.py - Merge master_flake_analysis_data.py into master_flake_analysis.py since model is now versioned - Capture metadata about the run, namely in swarming_rerun_results dict which keeps track of each swarming rerun result that was used to determine this analysis' flakiness graph BUG=650788 Committed: https://chromium.googlesource.com/infra/infra/+/535f86f50cdf5fe3b946200fa60b2682e72d2e6b

Patch Set 1 #

Total comments: 43

Patch Set 2 : Addressing comments #

Patch Set 3 : Do not set default value for cache_hit #

Patch Set 4 : Fixing passing version through pipelines #

Total comments: 7

Patch Set 5 : Addressing comments #

Total comments: 4

Patch Set 6 : Fixing nits #

Created: 4 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+527 lines, -381 lines)			Patch
M	appengine/findit/handlers/flake/check_flake.py	View	1 2 3 4 5	2 chunks	+14 lines, -6 lines	0 comments	Download
M	appengine/findit/handlers/flake/test/check_flake_test.py	View	1 2 3 4	5 chunks	+13 lines, -19 lines	0 comments	Download
M	appengine/findit/model/flake/flake_swarming_task.py	View	1 2 3 4 5	2 chunks	+36 lines, -17 lines	0 comments	Download
M	appengine/findit/model/flake/master_flake_analysis.py	View	1 2 3 4	2 chunks	+86 lines, -33 lines	0 comments	Download
D	appengine/findit/model/flake/master_flake_analysis_data.py	View		1 chunk	+0 lines, -92 lines	0 comments	Download
M	appengine/findit/model/flake/test/flake_swarming_task_test.py	View	1 2 3 4	2 chunks	+38 lines, -0 lines	0 comments	Download
M	appengine/findit/model/flake/test/master_flake_analysis_test.py	View	1 2 3 4	2 chunks	+20 lines, -0 lines	0 comments	Download
M	appengine/findit/templates/flake/result.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	appengine/findit/waterfall/flake/initialize_flake_pipeline.py	View	1 2 3 4	5 chunks	+37 lines, -17 lines	0 comments	Download
M	appengine/findit/waterfall/flake/recursive_flake_pipeline.py	View	1 2 3 4	9 chunks	+62 lines, -31 lines	0 comments	Download
M	appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py	View	1 2 3 4	28 chunks	+152 lines, -113 lines	0 comments	Download
M	appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py	View		3 chunks	+2 lines, -3 lines	0 comments	Download
M	appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py	View	1 2 3 4	5 chunks	+33 lines, -16 lines	0 comments	Download
M	appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py	View	1 2 3 4	10 chunks	+30 lines, -30 lines	0 comments	Download

Messages

Total messages: 18 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

chanli

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flake/master_flake_analysis.py File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flake/master_flake_analysis.py#newcode10 appengine/findit/model/flake/master_flake_analysis.py:10: Nit: remove this empty line. https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/flake/initialize_flake_pipeline.py File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): ...

4 years, 2 months ago (2016-09-27 22:15:24 UTC) #4

stgao

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/handlers/flake/check_flake.py File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2369333002/diff/1/appengine/findit/handlers/flake/check_flake.py#newcode60 appengine/findit/handlers/flake/check_flake.py:60: master_flake_analysis.pass_rates) As this is a rename, be sure to ...

4 years, 2 months ago (2016-09-28 00:03:24 UTC) #5

lijeffrey

comments addressed, ptal https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flake/master_flake_analysis.py File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flake/master_flake_analysis.py#newcode10 appengine/findit/model/flake/master_flake_analysis.py:10: On 2016/09/27 22:15:24, chanli wrote: > ...

4 years, 2 months ago (2016-09-28 03:12:31 UTC) #6

comments addressed, ptal

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:10: 
On 2016/09/27 22:15:24, chanli wrote:
> Nit: remove this empty line.

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:64: # The UTC timestamp
the check flake task was requested.
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Let's avoid "check flake task" or similar in code. Just say "analysis"?

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:71: # be used for metrics,
such as number of cache hits, average run time, etc.
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Are the metrics for running Swraming tasks or for running the whole analysis?
> 
> It seems a mixed here.

It is for the whole analysis, but depends on each swarming task. Since there can
be a variety of cache hits/optimization strategies used later, this dict is to
capture each swarming task's data that was relevant to this analysis' results.
Calculating metrics will be based heavily on this dict for cache hits, number of
tasks required to come to a conclusion, average time per swarming task
normalized +/- normalized per iteration, etc.

I'll reword the description to be more clear.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:75: #        
'request_time': 2016-09-06 (10:21:26.288) UTC
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Why do we need these detailed info about the Swarming tasks?

This is for calculating time per iteration. Later when we implement caching,
caching can be 1 of 3-states: hit, miss, or partial. In the case of partial
(another analysis triggered a swarming task on this exact build that has started
but is not finished), without these timestamps we don't know how much the
partial cache hit is benefiting and needs to be separated from complete miss/hit
cases.

I am still thinking on how we want to handle the partial cache hit cases, which
may include introducing another timestamp for the time that this analysis
requested this swarming task, and using request_time as the time that the
swarming task was actually requested (which in cache hit cases will be before
the time that this analysis requested it), but I haven't finalized the design
yet for partial hits that will be in a follow up cl.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:88: swarming_rerun_results
= ndb.JsonProperty(default={}, indexed=False)
On 2016/09/28 00:03:24, stgao (slow) wrote:
> This will grow big very quickly, thus it should be compressed=True.

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:94: correct =
ndb.BooleanProperty(indexed=False)
On 2016/09/28 00:03:24, stgao (slow) wrote:
> correct_regression_range? Later we might need correct_culprit or the like.

Good point. Added correct_culprit and renamed this correct_regression_range.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/master_flake_analysis.py:94: correct =
ndb.BooleanProperty(indexed=False)
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Let's index this field for easier query.

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/templates/...
File appengine/findit/templates/flake/result.html (left):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/templates/...
appengine/findit/templates/flake/result.html:27: // TODO: -1 means that the test
doesn't exist at the build.
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Please keep this. I need to fix it.

Oops sorry I didn't realize I accidentally deleted this line.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:13: def
NeedANewAnalysis(
On 2016/09/27 22:15:24, chanli wrote:
> I think we still need to make it transactional?

The Save() call already happens in a transaction, so this should not be
necessary if I understand correctly. Save() also does an assert that ndb is not
in a transaction, which will complain if @ndb.transactional is left up there.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:21: support for a
force flag to rerun this analysis.
On 2016/09/27 22:15:24, chanli wrote:
> Nit: Maybe move this TODO to a separate line?

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:34:
master_flake_analysis.Reset()
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Why do we need reset here?

We need to clear the fields, especially swarming_rerun_results since a rerun of
this analysis (if the algorithm parameters change or more caching becomes
available) will result in slightly different build numbers that were analyzed.
Additionally all previous fields should be cleared first before being written in
case this task ends prematurely in an error the previous version's fields aren't
accidentally left stale.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:36: if saved:
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Can we just return saved directly?

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:41: # the no cover
pragma.
On 2016/09/27 22:15:24, chanli wrote:
> Same here

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:91: # not
satisfied with with the results generated by the default ones provided
On 2016/09/28 00:03:24, stgao (slow) wrote:
> typo: with with

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:112: master_name,
builder_name, build_number, step_name, test_name,
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Should we pass the version of the master_flake_analysis into the pipeline so
> that only that version will be updated?

I think this is a good idea. Originally the design for not including version
number is it's assumed that always the latest version will be the one to update,
but this is not the case:

For experimental purposes it is possible 1 user triggers 2 identical check flake
analyses but with different algorithm parameters side by side, and in this case
without passing version number the incorrect version will be updated.

Passing version in all cases.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:10: 
On 2016/09/27 22:15:24, chanli wrote:
> Romove this line?

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:53: analysis =
MasterFlakeAnalysis.GetVersion(
On 2016/09/27 22:15:24, chanli wrote:
> Nit: maybe a longer name so we don't confuse it with WfAnalysis?

Unfortunately it seems the #pragma: no branch doesn't work if this is on 2 lines
so the longest we can make it is flake_analysis

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:187: 'message':
'Unknown'
On 2016/09/28 00:03:24, stgao (slow) wrote:
> Can we say "Swarming task failed" for now?

Done.

stgao

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/flake/initialize_flake_pipeline.py File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/flake/initialize_flake_pipeline.py#newcode34 appengine/findit/waterfall/flake/initialize_flake_pipeline.py:34: master_flake_analysis.Reset() On 2016/09/28 03:12:31, lijeffrey wrote: > On 2016/09/28 ...

4 years, 2 months ago (2016-09-30 21:07:31 UTC) #8

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:34:
master_flake_analysis.Reset()
On 2016/09/28 03:12:31, lijeffrey wrote:
> On 2016/09/28 00:03:24, stgao (slow) wrote:
> > Why do we need reset here?
> 
> We need to clear the fields, especially swarming_rerun_results since a rerun
of
> this analysis (if the algorithm parameters change or more caching becomes
> available) will result in slightly different build numbers that were analyzed.
> Additionally all previous fields should be cleared first before being written
in
> case this task ends prematurely in an error the previous version's fields
aren't
> accidentally left stale.

But it is newly created, there is no leftover state.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:112: master_name,
builder_name, build_number, step_name, test_name,
On 2016/09/28 03:12:31, lijeffrey wrote:
> On 2016/09/28 00:03:24, stgao (slow) wrote:
> > Should we pass the version of the master_flake_analysis into the pipeline so
> > that only that version will be updated?
> 
> I think this is a good idea. Originally the design for not including version
> number is it's assumed that always the latest version will be the one to
update,
> but this is not the case:
> 
> For experimental purposes it is possible 1 user triggers 2 identical check
flake
> analyses but with different algorithm parameters side by side, and in this
case
> without passing version number the incorrect version will be updated.

I'm against running two concurrent analysis for the same flake. That would
introduce unneeded complexity.
Does the function ``NeedANewAnalysis`` above supports this case already? If so,
let's comment the code out for now. Later we could revisit the code.

> 
> Passing version in all cases.

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/flake_swarming_task.py (right):

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_swarming_task.py:54: 'created_time':
str(self.created_time),
Convert to UTC timestamp instead?

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:78: #     task_id_1: {
Alternative to this is to create a metadata model, and use
LocalStructuredProperty here.
https://cloud.google.com/appengine/docs/python/ndb/entity-property-reference

stgao

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/flake/flake_swarming_task.py File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/flake/flake_swarming_task.py#newcode54 appengine/findit/model/flake/flake_swarming_task.py:54: 'created_time': str(self.created_time), On 2016/09/30 21:07:31, stgao (slow) wrote: > ...

4 years, 2 months ago (2016-09-30 21:10:54 UTC) #9

chanli

lgtm with a nit https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py File appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py#newcode149 appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py:149: print call_params nit: remove this ...

4 years, 2 months ago (2016-09-30 21:24:36 UTC) #10

lijeffrey

comments addressed, ptal https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/flake/initialize_flake_pipeline.py File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/flake/initialize_flake_pipeline.py#newcode34 appengine/findit/waterfall/flake/initialize_flake_pipeline.py:34: master_flake_analysis.Reset() On 2016/09/30 21:07:31, stgao (slow) ...

4 years, 2 months ago (2016-10-01 01:28:04 UTC) #11

comments addressed, ptal

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right):

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:34:
master_flake_analysis.Reset()
On 2016/09/30 21:07:31, stgao (slow) wrote:
> On 2016/09/28 03:12:31, lijeffrey wrote:
> > On 2016/09/28 00:03:24, stgao (slow) wrote:
> > > Why do we need reset here?
> > 
> > We need to clear the fields, especially swarming_rerun_results since a rerun
> of
> > this analysis (if the algorithm parameters change or more caching becomes
> > available) will result in slightly different build numbers that were
analyzed.
> > Additionally all previous fields should be cleared first before being
written
> in
> > case this task ends prematurely in an error the previous version's fields
> aren't
> > accidentally left stale.
> 
> But it is newly created, there is no leftover state.

Ah yes you are correct, I was looking at the wrong branch. I believe this
Reset() call has been removed in the latest patch.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:51: if saved:  #
pragma: no branch
On 2016/09/28 00:03:24, stgao (slow) wrote:
> same here.

Done.

https://codereview.chromium.org/2369333002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:112: master_name,
builder_name, build_number, step_name, test_name,
On 2016/09/30 21:07:31, stgao (slow) wrote:
> On 2016/09/28 03:12:31, lijeffrey wrote:
> > On 2016/09/28 00:03:24, stgao (slow) wrote:
> > > Should we pass the version of the master_flake_analysis into the pipeline
so
> > > that only that version will be updated?
> > 
> > I think this is a good idea. Originally the design for not including version
> > number is it's assumed that always the latest version will be the one to
> update,
> > but this is not the case:
> > 
> > For experimental purposes it is possible 1 user triggers 2 identical check
> flake
> > analyses but with different algorithm parameters side by side, and in this
> case
> > without passing version number the incorrect version will be updated.
> 
> I'm against running two concurrent analysis for the same flake. That would
> introduce unneeded complexity.
> Does the function ``NeedANewAnalysis`` above supports this case already? If
so,
> let's comment the code out for now. Later we could revisit the code.
> 
> > 
> > Passing version in all cases.
> 

NeedANewAnalysis as is does not support running 2 concurrent analyses, so no
further change here is necessary.

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/flake_swarming_task.py (right):

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/flake_swarming_task.py:54: 'created_time':
str(self.created_time),
On 2016/09/30 21:10:54, stgao (slow) wrote:
> On 2016/09/30 21:07:31, stgao (slow) wrote:
> > Convert to UTC timestamp instead?
> 
> If use LocalStructuredProperty, no change for the conversion.

Done.

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:78: #     task_id_1: {
On 2016/09/30 21:07:31, stgao (slow) wrote:
> Alternative to this is to create a metadata model, and use
> LocalStructuredProperty here.
> https://cloud.google.com/appengine/docs/python/ndb/entity-property-reference

Done.

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/waterf...
File
appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py
(right):

https://codereview.chromium.org/2369333002/diff/60001/appengine/findit/waterf...
appengine/findit/waterfall/test/process_flake_swarming_task_result_pipeline_test.py:149:
print call_params
On 2016/09/30 21:24:36, chanli wrote:
> nit: remove this print

Done.

stgao

lgtm with nits. https://codereview.chromium.org/2369333002/diff/80001/appengine/findit/handlers/flake/check_flake.py File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2369333002/diff/80001/appengine/findit/handlers/flake/check_flake.py#newcode66 appengine/findit/handlers/flake/check_flake.py:66: zipped = zip(build_numbers, pass_rates) Build the ...

4 years, 2 months ago (2016-10-01 01:36:59 UTC) #12

lijeffrey

https://codereview.chromium.org/2369333002/diff/80001/appengine/findit/handlers/flake/check_flake.py File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2369333002/diff/80001/appengine/findit/handlers/flake/check_flake.py#newcode66 appengine/findit/handlers/flake/check_flake.py:66: zipped = zip(build_numbers, pass_rates) On 2016/10/01 01:36:59, stgao (slow) ...

4 years, 2 months ago (2016-10-01 02:51:46 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2369333002/100001

4 years, 2 months ago (2016-10-01 02:52:05 UTC) #16

commit-bot: I haz the power

4 years, 2 months ago (2016-10-01 04:26:06 UTC) #18

Message was sent while issue was closed.

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/535f86f50cdf5fe3b946200fa60b2...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages