Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243673002:
[Findit] Added algorithm to analysis (Closed)
Description[Findit] Added algorithm to analysis
BUG=617808
Committed: https://chromium.googlesource.com/infra/infra/+/7a554f883a19261fdf5bccdd81c187b37d873b2a
Patch Set 1 #
Total comments: 12
Patch Set 2 : addressed comments #
Total comments: 12
Patch Set 3 : addressed comments, implemented algorithm to get it to one CL, made some UI changes. Unfortunately… #
Total comments: 28
Patch Set 4 : addressed comments #
Total comments: 4
Patch Set 5 : addressed comments #
Total comments: 1
Patch Set 6 : addressed comments #Patch Set 7 : gclient sync #Messages
Total messages: 30 (7 generated)
caiw@google.com changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:77: algo_dict = { not quite sure why you chose this name for this varable? It looks like it'll be used to collect metrics? https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: if last_result < .02 or last_result > .98: move all these values to constants at the top of the file, under the TODO that they should be part of config. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:79: new_algo_dict = { it looks like this is a direct copy. You can do new_algo_dict (please come up with a better name) = copy.deepcopy(algo_dict). Alternatively i think dict(algo_dict) should also work but you'd have to try it and make sure they're not just references to the same dict
https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:5: from datetime import datetime datetime should be in a separate group https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:24: queue_name=constants.DEFAULT_QUEUE): Could you add a docstring to explain the args? For example what's the difference between run_build_number and master_build_number? https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:76: next_run = False Maybe set next_run to 0 or -1 instead of False?
https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:77: algo_dict = { On 2016/08/11 22:22:25, lijeffrey wrote: > not quite sure why you chose this name for this varable? It looks like it'll be > used to collect metrics? it's a dictionary which keeps track of numbers used by the algorithm. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:5: from datetime import datetime On 2016/08/11 23:59:39, chanli wrote: > datetime should be in a separate group Done. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:24: queue_name=constants.DEFAULT_QUEUE): On 2016/08/11 23:59:39, chanli wrote: > Could you add a docstring to explain the args? For example what's the difference > between run_build_number and master_build_number? Done. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: if last_result < .02 or last_result > .98: On 2016/08/11 22:22:25, lijeffrey wrote: > move all these values to constants at the top of the file, under the TODO that > they should be part of config. Done. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:76: next_run = False On 2016/08/11 23:59:39, chanli wrote: > Maybe set next_run to 0 or -1 instead of False? Done. https://codereview.chromium.org/2243673002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:79: new_algo_dict = { On 2016/08/11 22:22:25, lijeffrey wrote: > it looks like this is a direct copy. You can do new_algo_dict (please come up > with a better name) = copy.deepcopy(algo_dict). > > Alternatively i think dict(algo_dict) should also work but you'd have to try it > and make sure they're not just references to the same dict Done.
https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: BUILD_NUMBERS_BACK = 1000 nit: Rename this MAX_BUILD_NUMBERS_TO_LOOK_BACK or something more descriptive https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:77: algo_dict = { nit: In general avoid abbreviating variable names (except dict or a few others are ok) for readability. Maybe something like flakiness_algorithm_results_dict or something https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:72: flake_swarming_task = FlakeSwarmingTask.Get( nit: it looks like this can be moved to before getting master = MasterFlakeAnalysis since master isn't needed till later https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:81: last_result = master.success_rates[-1] I would move this stuff into a new function that returns next_run and possibly new_algo_dict too to make it more testable and this run method less long https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:84: algo_dict['stable_in_a_row'] += 1 just an idea but it looks like algo_dict can be implemented as a class with a ToDict() method that translates it. This is in case you have multiple algorithms that may produce different results or capture different data we can compare which is more effective in the future. Just a thought though, no code change needed for now.
https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:95: next_run = min(master.build_numbers) - step_size Just a thought: Say after 10 flakes in a row, we start to get stables in a row. that means there are 10 builds between last flake and first stable, right? Do we need to run swarming runs for this 10 builds to further narrow down the actual build where the culprit's at?
https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: BUILD_NUMBERS_BACK = 1000 On 2016/08/15 04:14:07, lijeffrey wrote: > nit: Rename this MAX_BUILD_NUMBERS_TO_LOOK_BACK or something more descriptive Done. https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:77: algo_dict = { On 2016/08/15 04:14:07, lijeffrey wrote: > nit: In general avoid abbreviating variable names (except dict or a few others > are ok) for readability. Maybe something like flakiness_algorithm_results_dict > or something Done. https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:72: flake_swarming_task = FlakeSwarmingTask.Get( On 2016/08/15 04:14:07, lijeffrey wrote: > nit: it looks like this can be moved to before getting master = > MasterFlakeAnalysis since master isn't needed till later Done. https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:81: last_result = master.success_rates[-1] On 2016/08/15 04:14:08, lijeffrey wrote: > I would move this stuff into a new function that returns next_run and possibly > new_algo_dict too to make it more testable and this run method less long Done. https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:84: algo_dict['stable_in_a_row'] += 1 On 2016/08/15 04:14:07, lijeffrey wrote: > just an idea but it looks like algo_dict can be implemented as a class with a > ToDict() method that translates it. This is in case you have multiple algorithms > that may produce different results or capture different data we can compare > which is more effective in the future. > > Just a thought though, no code change needed for now. Acknowledged. https://codereview.chromium.org/2243673002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:95: next_run = min(master.build_numbers) - step_size On 2016/08/15 23:01:05, chanli wrote: > Just a thought: Say after 10 flakes in a row, we start to get stables in a row. > that means there are 10 builds between last flake and first stable, right? Do we > need to run swarming runs for this 10 builds to further narrow down the actual > build where the culprit's at? Yes Shuotao and I talked about this today. I will add it.
caiw@google.com changed reviewers: + stgao@chromium.org
https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/templa... File appengine/findit/templates/flake/result.html (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/templa... appengine/findit/templates/flake/result.html:51: <td>Status of Analysis: {{analysis_status}} How about putting these at the top of the page above the trend diagram? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: def get_next_run(master, flakiness_algorithm_results_dict): It would be awesome if a detailed description of the algorithm is added here or embedded into the code below for the if/elif cases. By reading the code directly, I'm not straightforward to understand the algorithm correctly. Another option is to put the description in a readme.md file. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: def get_next_run(master, flakiness_algorithm_results_dict): The above comment is still valid, but let's discuss in person first :) https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:62: last_result = master.success_rates[-1] Should we have an assert that the list is not empty? Have we taken care of this scenario? A flaky test is a newly-added one and doesn't exist in the build cycle of a Swarming rerun. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:72: flakiness_algorithm_results_dict['upper_boundary'] = cur_run If we go in this sequence for the Swarming rerun 50->40->30->20, why 20 is picked as the upper boundary instead of 50 as the lower boundary? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:73: flakiness_algorithm_results_dict['lower_boundary'] = False it seems the boundary is a mix of int number and boolean. Should we use None if a boundary is not found yet? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:107: if flakiness_algorithm_results_dict['sequential_run_index'] > 0: sequential_run_index seems not set before use. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:108: if (last_result_status != With the current approach, how big is the gap between upper and lower boundary? 5? If it is in 10s like (40, 60), should we do bisect or exponential search instead of linear search like this? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:135: master.status = analysis_status.ERROR the update is not saved. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:149: new_flakiness_algorithm_results_dict = copy.deepcopy( What's the reason this dict should be deep copied here? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:160: master.status = analysis_status.COMPLETED same here: the update is not saved.
https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/templa... File appengine/findit/templates/flake/result.html (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/templa... appengine/findit/templates/flake/result.html:51: <td>Status of Analysis: {{analysis_status}} On 2016/08/17 19:16:45, stgao wrote: > How about putting these at the top of the page above the trend diagram? Done. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: def get_next_run(master, flakiness_algorithm_results_dict): On 2016/08/17 19:16:45, stgao wrote: > It would be awesome if a detailed description of the algorithm is added here or > embedded into the code below for the if/elif cases. By reading the code > directly, I'm not straightforward to understand the algorithm correctly. > > Another option is to put the description in a readme.md file. Sure - I will write something up. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:62: last_result = master.success_rates[-1] On 2016/08/17 19:16:46, stgao wrote: > Should we have an assert that the list is not empty? > > Have we taken care of this scenario? A flaky test is a newly-added one and > doesn't exist in the build cycle of a Swarming rerun. Then analysis_status will be error in NextBuildNumberPipeline so it won't try to get the next_run. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:72: flakiness_algorithm_results_dict['upper_boundary'] = cur_run On 2016/08/17 19:16:45, stgao wrote: > If we go in this sequence for the Swarming rerun 50->40->30->20, why 20 is > picked as the upper boundary instead of 50 as the lower boundary? Let's talk in person about the algorithm. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:73: flakiness_algorithm_results_dict['lower_boundary'] = False On 2016/08/17 19:16:46, stgao wrote: > it seems the boundary is a mix of int number and boolean. Should we use None if > a boundary is not found yet? Is using None better style than using False? I can switch it if that's the case. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:107: if flakiness_algorithm_results_dict['sequential_run_index'] > 0: On 2016/08/17 19:16:45, stgao wrote: > sequential_run_index seems not set before use. It's set in initialize_flake_pipeline https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:108: if (last_result_status != On 2016/08/17 19:16:46, stgao wrote: > With the current approach, how big is the gap between upper and lower boundary? > 5? > > If it is in 10s like (40, 60), should we do bisect or exponential search instead > of linear search like this? This depends on how big the step size gets, but I don't think bisect/exponential search is really an option. Let's talk about it. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:135: master.status = analysis_status.ERROR On 2016/08/17 19:16:45, stgao wrote: > the update is not saved. Done. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:149: new_flakiness_algorithm_results_dict = copy.deepcopy( On 2016/08/17 19:16:45, stgao wrote: > What's the reason this dict should be deep copied here? I think Jeff told me to do this but I forget the reasoning. Can we just pass along the same dictionary? https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:160: master.status = analysis_status.COMPLETED On 2016/08/17 19:16:46, stgao wrote: > same here: the update is not saved. Done.
some more comments. Mostly style nits and for more comments in code for the algorithm. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: def get_next_run(master, flakiness_algorithm_results_dict): On 2016/08/17 21:30:32, caiw wrote: > On 2016/08/17 19:16:45, stgao wrote: > > It would be awesome if a detailed description of the algorithm is added here > or > > embedded into the code below for the if/elif cases. By reading the code > > directly, I'm not straightforward to understand the algorithm correctly. > > > > Another option is to put the description in a readme.md file. > > Sure - I will write something up. As we decided to go with a design doc, it might still help to also add some simple words in the code below. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:62: last_result = master.success_rates[-1] On 2016/08/17 21:30:32, caiw wrote: > On 2016/08/17 19:16:46, stgao wrote: > > Should we have an assert that the list is not empty? > > > > Have we taken care of this scenario? A flaky test is a newly-added one and > > doesn't exist in the build cycle of a Swarming rerun. > > Then analysis_status will be error in NextBuildNumberPipeline so it won't try to > get the next_run. As discussed, we should handle this corner case. A TODO is needed somewhere for this. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:73: flakiness_algorithm_results_dict['lower_boundary'] = False On 2016/08/17 21:30:31, caiw wrote: > On 2016/08/17 19:16:46, stgao wrote: > > it seems the boundary is a mix of int number and boolean. Should we use None > if > > a boundary is not found yet? > > Is using None better style than using False? I can switch it if that's the > case. Yes, None is better in this case. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:149: new_flakiness_algorithm_results_dict = copy.deepcopy( On 2016/08/17 21:30:31, caiw wrote: > On 2016/08/17 19:16:45, stgao wrote: > > What's the reason this dict should be deep copied here? > > I think Jeff told me to do this but I forget the reasoning. Can we just pass > along the same dictionary? Yes, we could pass it over directly as it is not modified after the code here. https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:87: 'sequential_run_index': 0 It would be great if we have brief comments on what these variables are for. https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:63: cur_run = min(master.build_numbers) style nits: no abbreviations on variable names.
https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py (right): https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:13: from waterfall.flake.recursive_flake_pipeline import get_next_run nit: move this line up
https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:60: def get_next_run(master, flakiness_algorithm_results_dict): On 2016/08/18 00:38:00, stgao wrote: > On 2016/08/17 21:30:32, caiw wrote: > > On 2016/08/17 19:16:45, stgao wrote: > > > It would be awesome if a detailed description of the algorithm is added here > > or > > > embedded into the code below for the if/elif cases. By reading the code > > > directly, I'm not straightforward to understand the algorithm correctly. > > > > > > Another option is to put the description in a readme.md file. > > > > Sure - I will write something up. > > As we decided to go with a design doc, it might still help to also add some > simple words in the code below. Done. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:62: last_result = master.success_rates[-1] On 2016/08/18 00:38:00, stgao wrote: > On 2016/08/17 21:30:32, caiw wrote: > > On 2016/08/17 19:16:46, stgao wrote: > > > Should we have an assert that the list is not empty? > > > > > > Have we taken care of this scenario? A flaky test is a newly-added one and > > > doesn't exist in the build cycle of a Swarming rerun. > > > > Then analysis_status will be error in NextBuildNumberPipeline so it won't try > to > > get the next_run. > > As discussed, we should handle this corner case. > A TODO is needed somewhere for this. Done. https://codereview.chromium.org/2243673002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:149: new_flakiness_algorithm_results_dict = copy.deepcopy( On 2016/08/18 00:38:00, stgao wrote: > On 2016/08/17 21:30:31, caiw wrote: > > On 2016/08/17 19:16:45, stgao wrote: > > > What's the reason this dict should be deep copied here? > > > > I think Jeff told me to do this but I forget the reasoning. Can we just pass > > along the same dictionary? > > Yes, we could pass it over directly as it is not modified after the code here. Done. https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py (right): https://codereview.chromium.org/2243673002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:13: from waterfall.flake.recursive_flake_pipeline import get_next_run On 2016/08/18 00:50:31, chanli wrote: > nit: move this line up Done.
One last comment :) https://codereview.chromium.org/2243673002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2243673002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:72: # Identified a candidate for the upper boundary. A reason of why this is the upper or lower boundary would be better instead. From the code, we could tell a upper boundary is found, but not clear for why it is. Same for below.
BTW, the link to the algorithm doc is not added either.
lgtm
On 2016/08/18 20:57:53, stgao wrote: > lgtm lgtm
The CQ bit was checked by caiw@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30b8f57a9fc1ad10) Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30b8f579de026d10) Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30b8f57a32e53910)
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2243673002/#ps120001 (title: "gclient sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Added algorithm to analysis BUG=617808 ========== to ========== [Findit] Added algorithm to analysis BUG=617808 Committed: https://chromium.googlesource.com/infra/infra/+/7a554f883a19261fdf5bccdd81c18... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/infra/infra/+/7a554f883a19261fdf5bccdd81c18... |
