Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12381088:
Codes for writing bench data to appengine datastore. (Closed)
|
Created:
7 years, 9 months ago by benchen Modified:
7 years, 9 months ago CC:
skia-review_googlegroups.com, skiabot_google.com, edisonn Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCodes for writing bench data to appengine datastore.
To activate, add flag "-a https://skiadash.appspot.com/add_point" or URL to other chrome-perf app instances. Otherwise behavior will be same as before.
Committed: https://code.google.com/p/skia/source/detail?r=8020
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Messages
Total messages: 16 (0 generated)
I plan to commit this first, then ask for Eric's help on turning it on with the -a flag on the bots.
Adding a few more reviewers who know this code better than I. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:26: DATA_SIZE = 100 Maybe this could be more descriptive? Like DATA_POINTS_TO_SEND or something similar? https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:392: config = config [1 : -1] # remove leading and trailing '_' Can use config.strip('_') https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:403: appengine_url, urllib.urlencode({'data': json.dumps(curr_data)})) Do we need a password? I think we *should* need a password, since we don't want anyone just uploading garbage data. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:412: sys.stderr.write("HTTPException for JSON %s\n" % lines) Should we collect errors so that we can exit non-zero on failure? I think we want to know when we didn't upload as expected.
On 2013/03/04 21:19:10, benchen wrote: > I plan to commit this first, then ask for Eric's help on turning it on with the > -a flag on the bots. As a general comment, what is any of this doing in bench_graph_svg.py? This file is for generating an svg, if one wishes to upload data, that belongs in a new file.
On 2013/03/04 21:39:42, bungeman1 wrote: > On 2013/03/04 21:19:10, benchen wrote: > > I plan to commit this first, then ask for Eric's help on turning it on with > the > > -a flag on the bots. > > As a general comment, what is any of this doing in bench_graph_svg.py? This file > is for generating an svg, if one wishes to upload data, that belongs in a new > file. I agree that it seems misplaced. The background is that we're phasing out the SVG graphs, and I think this script will become something more like "process_bench_data." It's responsibility will be to verify that the data are in the expected range and to upload the data to our dashboard's data store.
https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:32: print ' Example: "https://skiadash.appspot.com/add_point".' This is going to upload to our instance of chromium's appengine? I thought we were aiming to write to a SQL DB somewhere.
The reason to use this script is that it has the raw logs parsing step, which has become increasingly slow. Before Edi helps generate JSON bench outputs, that's inevitable, unless we want to add many more minutes to the whole bench pipeline. We've discussed this last week. Once we are sure we don't need the old bench graphs, we can change this script to doing data upload only. At that time we can change the file name etc. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:26: DATA_SIZE = 100 On 2013/03/04 21:32:15, borenet wrote: > Maybe this could be more descriptive? Like DATA_POINTS_TO_SEND or something > similar? Done. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:392: config = config [1 : -1] # remove leading and trailing '_' On 2013/03/04 21:32:15, borenet wrote: > Can use config.strip('_') Done. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:403: appengine_url, urllib.urlencode({'data': json.dumps(curr_data)})) That's under development. We'll implement an IP filter and later account authorization. But not sure how long it'll take... will leave as is for now (before that, or before we integrate with chrome-perf). On 2013/03/04 21:32:15, borenet wrote: > Do we need a password? I think we *should* need a password, since we don't want > anyone just uploading garbage data. https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:412: sys.stderr.write("HTTPException for JSON %s\n" % lines) Don't want this to block any other steps on the bots yet - this hasn't gone through load test etc. We're dogfooding it even ahead of Chrome. Will see how it goes then decide what to do next. On 2013/03/04 21:32:15, borenet wrote: > Should we collect errors so that we can exit non-zero on failure? I think we > want to know when we didn't upload as expected.
https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/1/bench/bench_graph_svg.py#newc... bench/bench_graph_svg.py:32: print ' Example: "https://skiadash.appspot.com/add_point".' The SQL DB solution is through a separate script that won't run within buildbot steps. Instead it'll be in batch mode done on Borg. This one is for loading data for the other "saving face" solution. Sorry I thought this were pretty clear. One advantage of this is that each bot uploads bench data even before uploading raw bench logs to bigstore. But the datastore solution won't give us the SQL functionality we desire. The corresponding Chrome CL: https://codereview.chromium.org/12317053/ On 2013/03/04 21:49:35, rmistry wrote: > This is going to upload to our instance of chromium's appengine? I thought we > were aiming to write to a SQL DB somewhere.
https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:12: import httplib Please put imports in alpha order https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:26: DATA_POINTS_TO_SEND = 100 Maybe DATA_POINT_BATCHSIZE instead, because you're sending ALL of the data points you found... you're just sending them 100 per HTTP request. Right? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:382: def write_to_appengine(lines, url, newest_revision, platform_and_alg): please add newline before this function https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:383: """Writes latest bench values to appengine datastore.""" please document the parameters https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:386: bot = platform_and_alg[ : platform_and_alg.rfind('-')] Why are we assembling platform_and_alg in line 470 and then taking it apart here? Wouldn't it make more sense to just pass in the platform name WITHOUT algorithm, if that's what we want? And what happens if platform_and_alg does NOT start with TITLE_PREAMBLE (see line 468)? In that case, I guess there is no '-' in the string??? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:387: line_str = str(line)[ : str(line).find('_{')] So, is this just trying to extract the portion of the line before the first "_{" is found? This seems like a needlessly complex way of doing it. What is the desired behavior if "_{" is NOT found in the line? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:393: rev, val = lines[line][-1] Is line a string or an index? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:400: curr_data = data[ : DATA_POINTS_TO_SEND] This seems like a really inefficient way to work our way through a dataset. For each batch, you're going to create 2 new arrays (curr_data and data) and copy over a bunch of data points to each one. Here's a recipe for doing it a faster/simpler way using itertools: http://stackoverflow.com/a/3703710 https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:407: sys.stderr.write("HTTPError: %d for JSON %s\n" % (e.code, data)) Why do you display the details of the exception differently in each case? (e.code for HTTPError, e.reason for URLError, nothing at all for HTTPException) Couldn't you just allow the underlying error to present its description as it sees fit? sys.stderr.write("HTTPError for JSON data %s: %s\n" % (data, e))
On 2013/03/04 21:39:42, bungeman1 wrote: > As a general comment, what is any of this doing in bench_graph_svg.py? This file > is for generating an svg, if one wishes to upload data, that belongs in a new > file. On 2013/03/04 21:51:14, benchen wrote: > The reason to use this script is that it has the raw logs parsing step, which > has become increasingly slow. Before Edi helps generate JSON bench outputs, > that's inevitable, unless we want to add many more minutes to the whole bench > pipeline. We've discussed this last week. benchen, I think what you're saying here is: 1. bench_graph_svg.py already has to parse the raw logs 2. in order to upload data to the appengine datastore, we have to parse it 3. parsing the raw logs takes a long time Therefore, we should take advantage of the fact that bench_graph_svg.py has already incurred the cost of parsing the raw logs, and make that same script also upload data to the appengine datastore. Right?
That's right Elliot. At the current stage I think this is the most convenient way to achieve what we need. On 2013/03/05 18:43:37, epoger wrote: > On 2013/03/04 21:39:42, bungeman1 wrote: > > As a general comment, what is any of this doing in bench_graph_svg.py? This > file > > is for generating an svg, if one wishes to upload data, that belongs in a new > > file. > > On 2013/03/04 21:51:14, benchen wrote: > > The reason to use this script is that it has the raw logs parsing step, which > > has become increasingly slow. Before Edi helps generate JSON bench outputs, > > that's inevitable, unless we want to add many more minutes to the whole bench > > pipeline. We've discussed this last week. > > benchen, I think what you're saying here is: > > 1. bench_graph_svg.py already has to parse the raw logs > 2. in order to upload data to the appengine datastore, we have to parse it > 3. parsing the raw logs takes a long time > > Therefore, we should take advantage of the fact that bench_graph_svg.py has > already incurred the cost of parsing the raw logs, and make that same script > also upload data to the appengine datastore. > > Right?
Thank you Elliot. Please take another look. I've also added 172.29.92.245 to the add_point whitelist, so you can only add data with that IP. https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:12: import httplib Done. Didn't do so since it conflicted with the previous "style". On 2013/03/05 18:38:52, epoger wrote: > Please put imports in alpha order https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:26: DATA_POINTS_TO_SEND = 100 Yeah that's better. Done. On 2013/03/05 18:38:52, epoger wrote: > Maybe DATA_POINT_BATCHSIZE instead, because you're sending ALL of the data > points you found... you're just sending them 100 per HTTP request. Right? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:382: def write_to_appengine(lines, url, newest_revision, platform_and_alg): On 2013/03/05 18:38:52, epoger wrote: > please add newline before this function Done. https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:383: """Writes latest bench values to appengine datastore.""" On 2013/03/05 18:38:52, epoger wrote: > please document the parameters Done. https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:386: bot = platform_and_alg[ : platform_and_alg.rfind('-')] I did not separate platform and alg because there was no need to do so before. You were right that now it's cleaner if we do. I used a separate var "bot" to store the bot platform name. On 2013/03/05 18:38:52, epoger wrote: > Why are we assembling platform_and_alg in line 470 and then taking it apart > here? Wouldn't it make more sense to just pass in the platform name WITHOUT > algorithm, if that's what we want? > > And what happens if platform_and_alg does NOT start with TITLE_PREAMBLE (see > line 468)? In that case, I guess there is no '-' in the string??? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:387: line_str = str(line)[ : str(line).find('_{')] create_lines() will make sure there is _{ in the line. Any suggesting on other ways of stripping the string from _{ ? On 2013/03/05 18:38:52, epoger wrote: > So, is this just trying to extract the portion of the line before the first "_{" > is found? This seems like a needlessly complex way of doing it. > > What is the desired behavior if "_{" is NOT found in the line? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:393: rev, val = lines[line][-1] It's an object with Label class. Please see create_lines() written by bungyman below. On 2013/03/05 18:38:52, epoger wrote: > Is line a string or an index? https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:400: curr_data = data[ : DATA_POINTS_TO_SEND] Nice suggestion. Done. On 2013/03/05 18:38:52, epoger wrote: > This seems like a really inefficient way to work our way through a dataset. For > each batch, you're going to create 2 new arrays (curr_data and data) and copy > over a bunch of data points to each one. > > Here's a recipe for doing it a faster/simpler way using itertools: > http://stackoverflow.com/a/3703710 https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:407: sys.stderr.write("HTTPError: %d for JSON %s\n" % (e.code, data)) I was just copying from https://codereview.chromium.org/12317053/ to make it consistent. Changed to your suggestion. On 2013/03/05 18:38:52, epoger wrote: > Why do you display the details of the exception differently in each case? > (e.code for HTTPError, e.reason for URLError, nothing at all for HTTPException) > Couldn't you just allow the underlying error to present its description as it > sees fit? > > sys.stderr.write("HTTPError for JSON data %s: %s\n" % (data, e))
https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:203: ({int:[BenchDataPoints]}, {str:str}, str?, str?, str?) What this means: INPUT parameters of the function are: 1. a dictionary with integer keys (revision #) and a list of bench data points as values 2. a dictionary of setting names to value 3-5. optional filter parameters: which bench, config, or timer type is of interest. For each one, if None, process them all. 6. optional timer type to ignore OUTPUT is a dictionary of this form: keys = Label objects values = a list of (x, y) tuples sorted such that x values increase monotonically https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:387: line_str = str(line)[ : str(line).find('_{')] On 2013/03/05 20:52:53, benchen wrote: > create_lines() will make sure there is _{ in the line. I don't see any mention of "_{" in create_lines. Overall, I think I just need a better understanding of what the line_data_dict really looks like. https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:394: lines: dictionary from create_lines. {Label:[(x,y)] | x[n] <= x[n+1]} I don't understand the notation at the end of this line. I don't understand the notation at its source, either (lines 212 and 216 above). Hopefully this is just a personal weakness that I can overcome... https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:400: for line in lines: Please rename the "lines" param to something like line_data_dict, because when I see this line I quickly assume "oh, we're iterating over a list of lines from a file". Also.. does this iterate over the dictionary's keys, or values, or tuples? Better to make it explicit... https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:401: line_str = str(line)[ : str(line).find('_{')] Why are we stringifying the Label object in "line" and then parsing it? Seems a lot more straightforward to just grab the Label fields you actually want (label.config or whatever) https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:402: if line_str.find('.skp') < 0 or not line_str.endswith('_'): If you're trying to filter the bench name, use label.bench
Thank you Elliot! I've switched to use the Label object which is much cleaner than str() and parse it. Please take another look. https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:203: ({int:[BenchDataPoints]}, {str:str}, str?, str?, str?) I've added your explanations assuming that bungyman would do so. On 2013/03/06 17:17:22, epoger wrote: > What this means: > > INPUT parameters of the function are: > 1. a dictionary with integer keys (revision #) and a list of bench data points > as values > 2. a dictionary of setting names to value > 3-5. optional filter parameters: which bench, config, or timer type is of > interest. For each one, if None, process them all. > 6. optional timer type to ignore > > OUTPUT is a dictionary of this form: > keys = Label objects > values = a list of (x, y) tuples sorted such that x values increase > monotonically https://codereview.chromium.org/12381088/diff/6001/bench/bench_graph_svg.py#n... bench/bench_graph_svg.py:387: line_str = str(line)[ : str(line).find('_{')] According to Class Label's __str__ method, there's always a "_" before the settings dict, so when stringify it there will always be "_{" separating settings and the rest. On 2013/03/06 17:17:22, epoger wrote: > On 2013/03/05 20:52:53, benchen wrote: > > create_lines() will make sure there is _{ in the line. > > I don't see any mention of "_{" in create_lines. > > Overall, I think I just need a better understanding of what the line_data_dict > really looks like. https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:394: lines: dictionary from create_lines. {Label:[(x,y)] | x[n] <= x[n+1]} Just copied it from create_lines. Shall I remove it? On 2013/03/06 17:17:22, epoger wrote: > I don't understand the notation at the end of this line. I don't understand the > notation at its source, either (lines 212 and 216 above). Hopefully this is > just a personal weakness that I can overcome... https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:400: for line in lines: On 2013/03/06 17:17:22, epoger wrote: > Please rename the "lines" param to something like line_data_dict, because when I > see this line I quickly assume "oh, we're iterating over a list of lines from a > file". > > Also.. does this iterate over the dictionary's keys, or values, or tuples? > Better to make it explicit... Done. https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:401: line_str = str(line)[ : str(line).find('_{')] You're right. I was still doing the "xhtml parsing" stuff, but here I should use Label directly. Done. On 2013/03/06 17:17:22, epoger wrote: > Why are we stringifying the Label object in "line" and then parsing it? Seems a > lot more straightforward to just grab the Label fields you actually want > (label.config or whatever) https://codereview.chromium.org/12381088/diff/11001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:402: if line_str.find('.skp') < 0 or not line_str.endswith('_'): On 2013/03/06 17:17:22, epoger wrote: > If you're trying to filter the bench name, use label.bench Done.
https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:214: INPUT parameters of the function are: If you're going to make this verbose, at least do it like the python style guide.
LGTM assuming a couple of cleanups as described below https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:214: INPUT parameters of the function are: On 2013/03/06 19:26:57, bungeman1 wrote: > If you're going to make this verbose, at least do it like the python style > guide. Now, now, bungeman. Let he who has documented in accordance with the python style guide cast the first stone. It's not clear to me why "terse and not in accordance" is preferable to "verbose and not in accordance". Having said that... yes, benchen, please use the docstring format as in https://www.corp.google.com/eng/doc/pyguide.xml#Comments , like so... """Converts revision data into a dictionary of line data. Args: revision_data_points: a dictionary with integer keys (revision #) and a list of bench data points as values settings: a dictionary of setting names to values ... https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:413: for label in line_data_dict: please change this to: for label in line_data_dict.iterkeys(): ...just to make it clearer that it is iterating over the dictionary *keys*. Results should be the same. https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:418: rev, val = line_data_dict[label][-1] So I guess the assumption here is that newest_revision is >= the revision of the last data point we have for each line? Seems fair, although I could imagine someone trying to call this with a different revision # and being suprised to get nothing back. Maybe add a comment like this... # This assumes that newest_revision is >= the revision of # the last data point we have for each line.
Message was sent while issue was closed.
Thanks Elliot. All done. Committed rev 8020. Now let's see how it works. https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/16001/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:214: INPUT parameters of the function are: On 2013/03/07 16:56:46, epoger wrote: > On 2013/03/06 19:26:57, bungeman1 wrote: > > If you're going to make this verbose, at least do it like the python style > > guide. > > Now, now, bungeman. Let he who has documented in accordance with the python > style guide cast the first stone. It's not clear to me why "terse and not in > accordance" is preferable to "verbose and not in accordance". > > Having said that... yes, benchen, please use the docstring format as in > https://www.corp.google.com/eng/doc/pyguide.xml#Comments , like so... > > """Converts revision data into a dictionary of line data. > > Args: > revision_data_points: a dictionary with integer keys (revision #) and a list > of bench data points as values > settings: a dictionary of setting names to values > ... Done. https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py File bench/bench_graph_svg.py (right): https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:413: for label in line_data_dict: On 2013/03/07 16:56:46, epoger wrote: > please change this to: > > for label in line_data_dict.iterkeys(): > > ...just to make it clearer that it is iterating over the dictionary *keys*. > Results should be the same. Done. https://codereview.chromium.org/12381088/diff/14002/bench/bench_graph_svg.py#... bench/bench_graph_svg.py:418: rev, val = line_data_dict[label][-1] On 2013/03/07 16:56:46, epoger wrote: > So I guess the assumption here is that newest_revision is >= the revision of the > last data point we have for each line? Seems fair, although I could imagine > someone trying to call this with a different revision # and being suprised to > get nothing back. > > Maybe add a comment like this... > > # This assumes that newest_revision is >= the revision of > # the last data point we have for each line. Done. |
