BENCH: asv csv reading benchmarks now rewind StringIO objects

[tylerjereddy]

In short, the asv benchmarks used for timing read_csv are actually reading in empty file-like objects most of the time at the moment. This is because the setup() method of the benchmarks is only called between repeats and not between iterations of repeats--see writing benchmarks docs.

You can confirm this by simply printing the size of the dataframes you get from read_csv in any StringIO-dependent benchmarks--the first iteration of a repeat should have the correct size, but the rest will all be 0. You could also just print the memory address (StringIO object)--they'll all be the same after the first iteration of a repeat. I did this with @stefanv and @mattip in the NumPy context.

This can be quite confusing--a bit different than the paradigm you might expect in i.e, a unit test context--for more gory details see my related comment in NumPy loadtxt() asv benchmarks. As noted there, one could probably avoid this by using an actual file object instead / if preferred, but realistically this just boils down to how timeit works. Confounding the results with the seek is unfortunate, so working with file objects may be preferred if dumping files in the bench env is acceptable.

[codecov]

Codecov Report

Merging #21807 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #21807   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         170      170           
  Lines       50708    50708           
=======================================
  Hits        46677    46677           
  Misses       4031     4031

Flag	Coverage Δ
#multiple	90.45% <ø> (ø)	⬆️
#single	42.36% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a2fbce...90d3dda. Read the comment docs.

[jreback]

hmm can we instead make data a property which does this, otherwise this is likely to be forgotten the next time a benchmark is written. can you show a before /after on these benchmarks?

[pep8speaks]

Hello @tylerjereddy! Thanks for updating the PR.

• In the file asv_bench/benchmarks/io/csv.py, following are the PEP8 issues :

Line 184:80: E501 line too long (87 > 79 characters)

Comment last updated on July 26, 2018 at 20:11 Hours UTC

[tylerjereddy]

Revised accordingly with before / after log files from asv run -e -b "ReadCSV*" >& bench_results.txt below. Note that this won't scrape in all of the affected benchmarks because not all CSV reading benchmarks follow a consistent naming scheme for regular expression selection, but should pull in many of them. Not sure if pandas CI does a quick (--quick) check of asv suite to make sure there are no bench errors on PRs (proposals to do this are in progress for NumPy / SciPy).

Before rewinding StringIO objects in csv reading benchmarks (commit hash: 7829ad3): old_bench_results.txt

After rewinding StringIO objects in csv reading benchmarks: bench_results.txt

I agree that using the property approach may alleviate future forgetting-to-rewind issues, though you'll likely notice that one of the classes where two data1/2 objects are used instead of parametrization becomes slightly more complicated to think about.

Most csv reading benchmarks are now a bit slower (based on visual inspection of above logs), but this is obviously going to happen when you don't read in empty file objects by mistake for most of the iterations, and there's also the conflated time from adding in rewinds now too.

There's another alternative here if you prefer--setting number = 1 in each of the class objects will force setup() to run between each iteration; I've preferred not to do that and instead allow the decisions about appropriate number of repeats & iterations to be offloaded to asv and timeit under the hood.

[jreback]

just make this a function I think and pass in the StringIO object

[jbrockmendel]

Travis failure looks unrelated. @tylerjereddy this is almost over the finish line.

[tylerjereddy]

Revised / squashed / rebased / force pushed. Hopefully better now.

[jreback]

thanks @tylerjereddy