Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1622813003:
[Findit] Adding support for extracting revisions from dict instead of list (Closed)
Description[Findit] Adding support for extracting revisions from dict instead of list
Currently, Findit expects the recompile recipe to return compile results for each revision as a list. This change will change compile results to a dict inside a 'report' dict and support both formats are supported.
BUG=581527
Committed: https://chromium.googlesource.com/infra/infra/+/132a352637fff9c00bc5781b992d8385d3e42b1d
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing code review comments #
Total comments: 10
Patch Set 3 : Addressing code review comments #
Total comments: 9
Patch Set 4 : Addressing code review comments #Messages
Total messages: 24 (7 generated)
Description was changed from
==========
[Findit] Adding support for extracting revisions from dict instead of list
[Findit] Adding support for interepreting failed revisions from dict instead of
list
BUG=
==========
to
==========
[Findit] Adding support for extracting revisions from dict instead of list
Currently, the findit recompile recipe returns compile results for each revision
in the form
{
'results': [
['rev1', 'passed'],
['rev2', 'failed'],
]
}
This change is to support a new proposed format, which looks like
{
'analysis_info': {
'results': {
'rev1': 'passed',
'rev2': 'failed',
}
}
}
With this change, both formats are supported, though the first will be
deprecated in favor of the second. By inroducing the dict 'analysis_info' we can
package other useful information from the compile recipe as other fields, with
the compile results as one of them, rather than the old approach of only
including the compile results and in the form of a list.
BUG=(will update once crbug is back online)
==========
lijeffrey@chromium.org changed reviewers: + chanli@chromium.org, qyearsley@chromium.org, stgao@chromium.org
Hey guys, this is part 1 of a 4-CL change feature for capturing tryjob data: 1. Make findit support the old format (results as list) as well as new format (results as part of a dict) 2. Update findit compile recipe to return results in the new proposed dict format described in the description of this CL 3. Update findit to deprecate the old list format 4. Capture tryjob data using new dict format ptal when you have the chance :) https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:69: HttpClient()) oops I'll put this back
https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:15: def _getFailedRevisionFromResultsDict(self, results_dict): Nit: In Chromium style, function names start with a capital (except in special cases where the name is determined by something else, e.g the special name run below and the test method names which start with test). https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:32: The revision corresponding to a failed result, if any. Nit: Only need two spaces of indentation for Args and Returns section of docstring. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:77: compile_result['culprit'] = culprit This section (inside `if compile_result`) could potentially be extracted to a helper method -- something like: def _FindCulpritFromCompileResult(self, compile_result): """Returns a culprit (dict) if found, else None.""" ... https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:31: pipeline._getFailedRevisionFromResultsDict({})) Instead of self.assertEqual(None, ...), it's a little more concise to use self.assertIsNone(...).
https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:64: revision_results) How about moving lines #46 - #64 into the new function above and making that function handle both format? If you make the new function a @staticmethod, I believe that the unittests below could be much simplified too as we only need to test the new function itself.
https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:15: def _getFailedRevisionFromResultsDict(self, results_dict): On 2016/01/24 21:01:06, qyearsley wrote: > Nit: In Chromium style, function names start with a capital (except in special > cases where the name is determined by something else, e.g the special name run > below and the test method names which start with test). Done. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:32: The revision corresponding to a failed result, if any. On 2016/01/24 21:01:05, qyearsley wrote: > Nit: Only need two spaces of indentation for Args and Returns section of > docstring. Done. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:64: revision_results) On 2016/01/25 19:19:52, stgao wrote: > How about moving lines #46 - #64 into the new function above and making that > function handle both format? > > If you make the new function a @staticmethod, I believe that the unittests below > could be much simplified too as we only need to test the new function itself. Done. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:69: HttpClient()) On 2016/01/23 02:24:09, lijeffrey wrote: > oops I'll put this back Done. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:77: compile_result['culprit'] = culprit On 2016/01/24 21:01:06, qyearsley wrote: > This section (inside `if compile_result`) could potentially be extracted to a > helper method -- something like: > > def _FindCulpritFromCompileResult(self, compile_result): > """Returns a culprit (dict) if found, else None.""" > ... Done. https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1622813003/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:31: pipeline._getFailedRevisionFromResultsDict({})) On 2016/01/24 21:01:06, qyearsley wrote: > Instead of self.assertEqual(None, ...), it's a little more concise to use > self.assertIsNone(...). Done.
LGTM https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:88: return culprit Slightly different way to write the same thing: ... if not change_log: return None return { 'revision': failed_revision, 'commit_position': change_log.commit_position, 'review_url': change_log.code_review_url }
As we discuss offline, post two more comments :) https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:57: result_for_last_checked_revision = compile_result['result'][-1] result -> report. Need to update buildbucket_client.py and monitor_try_job_pipeline.py too. And in monitor_try_job_pipeline.py, return the build.result/report directly. https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): compile_result.get('analysis_info') -> report.get('result')
https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): Is this format finalized?
https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): On 2016/01/26 18:15:28, chanli wrote: > Is this format finalized? Jeff and I had a discussion on the new format to make it extensible. We could discuss more on it. What's your recommendation?
https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): On 2016/01/26 18:32:26, stgao wrote: > On 2016/01/26 18:15:28, chanli wrote: > > Is this format finalized? > > Jeff and I had a discussion on the new format to make it extensible. > We could discuss more on it. > > What's your recommendation? Should the format like what Jeff mentioned in the description? { 'analysis_info': { 'results': { 'rev1': 'passed', 'rev2': 'failed', } } } If so we need to modify monitor_try_job_pipeline as well.
https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): On 2016/01/26 18:52:19, chanli wrote: > On 2016/01/26 18:32:26, stgao wrote: > > On 2016/01/26 18:15:28, chanli wrote: > > > Is this format finalized? > > > > Jeff and I had a discussion on the new format to make it extensible. > > We could discuss more on it. > > > > What's your recommendation? > > Should the format like what Jeff mentioned in the description? > { > 'analysis_info': { > 'results': { > 'rev1': 'passed', > 'rev2': 'failed', > } > } > } > > If so we need to modify monitor_try_job_pipeline as well. Yes, very similar to that, expect no root node "analysis_info". I guess Jeff will update the CL description when he uploads the new patch.
Description was changed from
==========
[Findit] Adding support for extracting revisions from dict instead of list
Currently, the findit recompile recipe returns compile results for each revision
in the form
{
'results': [
['rev1', 'passed'],
['rev2', 'failed'],
]
}
This change is to support a new proposed format, which looks like
{
'analysis_info': {
'results': {
'rev1': 'passed',
'rev2': 'failed',
}
}
}
With this change, both formats are supported, though the first will be
deprecated in favor of the second. By inroducing the dict 'analysis_info' we can
package other useful information from the compile recipe as other fields, with
the compile results as one of them, rather than the old approach of only
including the compile results and in the form of a list.
BUG=(will update once crbug is back online)
==========
to
==========
[Findit] Adding support for extracting revisions from dict instead of list
Currently, the findit expects recompile recipe to return compile results for
each revision as a list, and stores compile_results in the format:
{
'result': [
['rev1', 'passed'],
['rev2', 'failed'],
],
'url': url,
'try_job_id': 12345
}
This change is to support a new proposed format (which will require updating
from the compile recipe to provide), which looks like
{
'report': {
'result': {
'rev1': 'passed',
'rev2': 'failed',
},
'number_of_commits_in_range': 7,
... (other useful info as needed)
},
'url': url,
'try_job_id': 12345
}
With this change, both formats are supported, though the first will be
deprecated in favor of the second. By introducing the dict 'report' we can
package other useful information from the compile recipe as other fields, with
the compile result mapping revisions to passed/failed results as one of them,
rather than the old approach of only including the revisions-passed/failed
results as a list.
BUG=581527
==========
Hey guys, this latest change also changes the build property 'result' to 'report' which will more closely reflect the proposed change for the compile recipe. I also removed some redundant unit tests since the extraction part of the failed revision was re factored out for improved testability. Ptal :) https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:57: result_for_last_checked_revision = compile_result['result'][-1] On 2016/01/26 01:22:33, stgao wrote: > result -> report. > > Need to update buildbucket_client.py and monitor_try_job_pipeline.py too. > And in monitor_try_job_pipeline.py, return the build.result/report directly. Done. https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:61: elif isinstance(compile_result.get('analysis_info'), dict): On 2016/01/26 19:03:35, stgao wrote: > On 2016/01/26 18:52:19, chanli wrote: > > On 2016/01/26 18:32:26, stgao wrote: > > > On 2016/01/26 18:15:28, chanli wrote: > > > > Is this format finalized? > > > > > > Jeff and I had a discussion on the new format to make it extensible. > > > We could discuss more on it. > > > > > > What's your recommendation? > > > > Should the format like what Jeff mentioned in the description? > > { > > 'analysis_info': { > > 'results': { > > 'rev1': 'passed', > > 'rev2': 'failed', > > } > > } > > } > > > > If so we need to modify monitor_try_job_pipeline as well. > > Yes, very similar to that, expect no root node "analysis_info". I guess Jeff > will update the CL description when he uploads the new patch. Done. https://codereview.chromium.org/1622813003/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:88: return culprit On 2016/01/26 00:40:04, qyearsley wrote: > Slightly different way to write the same thing: > > ... > if not change_log: > return None > > return { > 'revision': failed_revision, > 'commit_position': change_log.commit_position, > 'review_url': change_log.code_review_url > } Done.
lgtm with nits. As a bug is attached to this CL, the CL description could be simplified by deleting the details. https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:115: return None In this case, should we still return the failed revision as it is already found? return { 'revision': failed_revision} https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/monitor_try_job_pipeline.py:50: return try_job_result.compile_results[-1] Return build.report directly if the other fields are not used in other pipeline? https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:107: 'url': 'url', As 'url' is not used and 'try_job_id' is already passed as a separate parameter, should we delete these two fields?
https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:84: if isinstance(report, list): It just occurs to me, since we always check the result from running try job here, report should always be dict, right? I think the only place need to be compatible is the result page?
Description was changed from
==========
[Findit] Adding support for extracting revisions from dict instead of list
Currently, the findit expects recompile recipe to return compile results for
each revision as a list, and stores compile_results in the format:
{
'result': [
['rev1', 'passed'],
['rev2', 'failed'],
],
'url': url,
'try_job_id': 12345
}
This change is to support a new proposed format (which will require updating
from the compile recipe to provide), which looks like
{
'report': {
'result': {
'rev1': 'passed',
'rev2': 'failed',
},
'number_of_commits_in_range': 7,
... (other useful info as needed)
},
'url': url,
'try_job_id': 12345
}
With this change, both formats are supported, though the first will be
deprecated in favor of the second. By introducing the dict 'report' we can
package other useful information from the compile recipe as other fields, with
the compile result mapping revisions to passed/failed results as one of them,
rather than the old approach of only including the revisions-passed/failed
results as a list.
BUG=581527
==========
to
==========
[Findit] Adding support for extracting revisions from dict instead of list
Currently, Findit expects the recompile recipe to return compile results for
each revision as a list. This change will change compile results to a dict
inside a 'report' dict and support both formats are supported.
BUG=581527
==========
https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:84: if isinstance(report, list): On 2016/01/27 18:58:10, chanli wrote: > It just occurs to me, since we always check the result from running try job > here, report should always be dict, right? > > I think the only place need to be compatible is the result page? in the old format result (now renamed report) is still a list. I think the result page should be fine, since it still consumes a try_job_result dict which contains the culprit separately. I'll double check this once we make the change to return the report dict from the recipe https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:115: return None On 2016/01/27 01:24:20, stgao wrote: > In this case, should we still return the failed revision as it is already found? > > return { 'revision': failed_revision} This will be addressed in a separate CL https://code.google.com/p/chromium/issues/detail?id=581922 https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/monitor_try_job_pipeline.py:50: return try_job_result.compile_results[-1] On 2016/01/27 01:24:21, stgao wrote: > Return build.report directly if the other fields are not used in other pipeline? It turns out IdentifyTryJobCulpritPipeline still needs the full compile_result dict when it finds a culprit so it can update it with compile_result['culprit'] = culprit. https://code.google.com/p/chromium/issues/detail?id=581933 Should address refactoring this out in a separate CL. https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:107: 'url': 'url', On 2016/01/27 01:24:21, stgao wrote: > As 'url' is not used and 'try_job_id' is already passed as a separate parameter, > should we delete these two fields? Done.
lgtm https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1622813003/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:107: 'url': 'url', On 2016/01/28 02:14:35, lijeffrey wrote: > On 2016/01/27 01:24:21, stgao wrote: > > As 'url' is not used and 'try_job_id' is already passed as a separate > parameter, > > should we delete these two fields? > > Done. It is fine for the test but the real format have those 2 fields.
The CQ bit was checked by lijeffrey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/1622813003/#ps60001 (title: "Addressing code review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1622813003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1622813003/60001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Adding support for extracting revisions from dict instead of list Currently, Findit expects the recompile recipe to return compile results for each revision as a list. This change will change compile results to a dict inside a 'report' dict and support both formats are supported. BUG=581527 ========== to ========== [Findit] Adding support for extracting revisions from dict instead of list Currently, Findit expects the recompile recipe to return compile results for each revision as a list. This change will change compile results to a dict inside a 'report' dict and support both formats are supported. BUG=581527 Committed: https://chromium.googlesource.com/infra/infra/+/132a352637fff9c00bc5781b992d8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/infra/infra/+/132a352637fff9c00bc5781b992d8... |
