[Findit] Adding Try Job suspected CLs and result status to analysis

[Findit] Adding Try Job suspected CLs and result status to analysis

Previously, only the results from heuristic-based analysis are accounted for when triaging Findit results. This change merges the results found by try jobs with those found by heuristic-based analysis.

BUG=601853
Committed: https://chromium.googlesource.com/infra/infra/+/0eb9ae8bde4c7f5b3d0477080f189f1b4bfa5a0c

[lijeffrey, 2016-04-08 18:06:06]

Description was changed from

==========
[Findit] Adding Try Job suspected CLs and result status to analysis

BUG=601853
==========

to

==========
[Findit] Adding Try Job suspected CLs and result status to analysis

Previously, only the results from heuristic-based analysis are accounted for
when triaging Findit results. This change merges the results found by try jobs
with those found by heuristic-based analysis.

BUG=601853
==========

[lijeffrey, 2016-04-08 18:06:06]

lijeffrey@chromium.org changed reviewers:
+ chanli@chromium.org, stgao@chromium.org

[lijeffrey, 2016-04-08 18:20:38]

This change still needs to be fully-tested with real failures and try jobs:

Cases that work:
1. Compile failure: no heuristic results but try job results availble.
2. Compile failure: both heuristic results and try job results

Cases being tested:
1. Compile failure: no heuristic results, no try job culprits
2. Compile failure: heuristic results, but no try job culprits
3. Test failures: no heuristic results, but try job results available.
4. Test failures: both heuristic results and try job results available
5. Test failures: neither heuristic results nor try job results available
6. Test failures: heuristic results but no try job results (flaky tests)

PTAL when you have the chance.

Thanks,
Jeff

[stgao, 2016-04-11 19:50:13]

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:7: from model
import wf_analysis_result_status
you may want to rebase on the refactoring CL I committed.

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:41: return
wf_analysis_result_status.NOT_FOUND_UNTRIAGED
Why we reset wf_analysis_result_status.NOT_FOUND_INCORRECT and
wf_analysis_result_status.NOT_FOUND_CORRECT to
wf_analysis_result_status.NOT_FOUND_UNTRIAGED if the try-job find nothing?

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:58:
suspected_cls = analysis.suspected_cls or []
Should we do a copy of analysis.suspected_cls instead of updating it directly
through a reference?

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:72: return
suspected_cls
How about the "else" case of line #70?

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:273:
analysis.result_status = _GetResultAnalysisStatus(analysis, result)
If result is empty, no checking is needed in which case we could avoid a
datastore read&write.

[lijeffrey, 2016-04-12 21:10:54]

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:41: return
wf_analysis_result_status.NOT_FOUND_UNTRIAGED
On 2016/04/11 19:50:12, stgao wrote:
> Why we reset wf_analysis_result_status.NOT_FOUND_INCORRECT and
> wf_analysis_result_status.NOT_FOUND_CORRECT to
> wf_analysis_result_status.NOT_FOUND_UNTRIAGED if the try-job find nothing?

Done.

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:58:
suspected_cls = analysis.suspected_cls or []
On 2016/04/11 19:50:13, stgao wrote:
> Should we do a copy of analysis.suspected_cls instead of updating it directly
> through a reference?

Done.

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:72: return
suspected_cls
On 2016/04/11 19:50:12, stgao wrote:
> How about the "else" case of line #70?

Done.

https://codereview.chromium.org/1870103003/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:273:
analysis.result_status = _GetResultAnalysisStatus(analysis, result)
On 2016/04/11 19:50:13, stgao wrote:
> If result is empty, no checking is needed in which case we could avoid a
> datastore read&write.

Done.

[stgao, 2016-04-12 22:41:19]

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:278: analysis =
WfAnalysis.Get(master_name, builder_name, build_number)
nit: If ``culprits`` is None or empty (which means that the try job didn't find
any culprit), do we still need to check?

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:285:
analysis.suspected_cls = updated_suspected_cls
A datastore write is needed as we want to update and save the data.

[lijeffrey, 2016-04-13 21:36:24]

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:278: analysis =
WfAnalysis.Get(master_name, builder_name, build_number)
On 2016/04/12 22:41:19, stgao wrote:
> nit: If ``culprits`` is None or empty (which means that the try job didn't
find
> any culprit), do we still need to check?

This is in case analysis_result was set to None by _GetAnalysisResultStatus from
identify_culprit_pipeline, which we want to update with NOT_FOUND_UNTRIAGED even
if 'culprits' is nne or empty, unless we want to keep it as None instead?

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:285:
analysis.suspected_cls = updated_suspected_cls
On 2016/04/12 22:41:19, stgao wrote:
> A datastore write is needed as we want to update and save the data.

Done. Good catch, was accidentally deleted.

[lijeffrey, 2016-04-13 22:50:17]

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/1870103003/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:278: analysis =
WfAnalysis.Get(master_name, builder_name, build_number)
On 2016/04/13 21:36:24, lijeffrey wrote:
> On 2016/04/12 22:41:19, stgao wrote:
> > nit: If ``culprits`` is None or empty (which means that the try job didn't
> find
> > any culprit), do we still need to check?
> 
> This is in case analysis_result was set to None by _GetAnalysisResultStatus
from
> identify_culprit_pipeline, which we want to update with NOT_FOUND_UNTRIAGED
even
> if 'culprits' is nne or empty, unless we want to keep it as None instead?

Done. Per discussion we don't want to update even if the previous values are
None.

[stgao, 2016-04-13 22:55:46]

lgtm

[lijeffrey, 2016-04-14 00:09:41]

The CQ bit was checked by lijeffrey@chromium.org

[commit-bot: I haz the power, 2016-04-14 00:09:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870103003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870103003/80001

[commit-bot: I haz the power, 2016-04-14 00:15:44]

Description was changed from

==========
[Findit] Adding Try Job suspected CLs and result status to analysis

Previously, only the results from heuristic-based analysis are accounted for
when triaging Findit results. This change merges the results found by try jobs
with those found by heuristic-based analysis.

BUG=601853
==========

to

==========
[Findit] Adding Try Job suspected CLs and result status to analysis

Previously, only the results from heuristic-based analysis are accounted for
when triaging Findit results. This change merges the results found by try jobs
with those found by heuristic-based analysis.

BUG=601853

Committed:
https://chromium.googlesource.com/infra/infra/+/0eb9ae8bde4c7f5b3d0477080f189...
==========

[commit-bot: I haz the power, 2016-04-14 00:15:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/0eb9ae8bde4c7f5b3d0477080f189...