Closed
Description
We are currently not linting (validating PEP-8) of the benchmarks in asv_bench/benchmarks
. But it'd be good to start doing it as with the rest of the code.
Before starting the automatic validation, we need to fix all the style problems. Those can be obtained by executing flake8 --config=none asv_bench/benchmarks --ignore=F811,E731
This is currently returning:
asv_bench/benchmarks/algorithms.py:12:5: E722 do not use bare except'
asv_bench/benchmarks/timeseries.py:1:1: F401 'warnings' imported but unused
asv_bench/benchmarks/stat_ops.py:21:9: E722 do not use bare except'
asv_bench/benchmarks/stat_ops.py:59:9: E722 do not use bare except'
asv_bench/benchmarks/pandas_vb_common.py:5:1: F401 'pandas.Panel' imported but unused
asv_bench/benchmarks/pandas_vb_common.py:12:5: E722 do not use bare except'
asv_bench/benchmarks/pandas_vb_common.py:37:9: E722 do not use bare except'
asv_bench/benchmarks/join_merge.py:32:9: E722 do not use bare except'
asv_bench/benchmarks/io/csv.py:2:1: F401 'timeit' imported but unused
asv_bench/benchmarks/io/csv.py:8:1: F401 'pandas.compat.PY2' imported but unused
asv_bench/benchmarks/io/csv.py:184:80: E501 line too long (87 > 79 characters)
All them should be fixed. Most of them should be immediate to fix, but for E722 do not use bare except
make sure the specific exception is captured, and don't simply use except Exception:
.
Activity
TomAugspurger commentedon Sep 29, 2018
Using
except Exception
seems fine in the benchmarks, since it isn't library code.That said, it looks like most are just import errors.
MatanCohe commentedon Sep 29, 2018
@TomAugspurger Is it the case with :
asv_bench/benchmarks/join_merge.py:32:9: E722 do not use bare except'
?datapythonista commentedon Sep 29, 2018
@MatanCohe that's right, and also all the rest in the list that has the error
E722 do not use bare except
.But still, it'd be nice to have the right exception if it's easy, we may unintentionally silent bugs with
Exception
. But if it's tricky to see which exception we're trying to capture, usingExcept
is not as important as in the code. We'll just get the benchmarks wrong, but the pandas code won't be affected.MatanCohe commentedon Sep 29, 2018
@datapythonista sorry about not being clear about what I'm asking.
The need of the correct exceptions is clear to me but, I see no reason to capture
ImportError
injoin_merge.py:32:9
so I want to check if I'm missing something.datapythonista commentedon Sep 29, 2018
I'm not quite sure what
df.consolidate(inplace=True)
can raise tbh, but I don't think it should beImportError
.I think the best is to replace the
pass
in theexcept
by araise
, so you can see what is the exception that this is causing (I guess under some condition it should raise one, otherwise I'm not sure why do we need thattry/except
)jbrockmendel commentedon Sep 29, 2018
lint-adjacent for the benchmarks: I think some of the directives being used are deprecated in asv. e.g. in
algorithms
each of the benchmark classes definesgoal_time
, but it looks like asv just ignores that: https://asv.readthedocs.io/en/stable/benchmarks.html#benchmark-attributesSo the pertinent suggestion: linting for valid asv directives
datapythonista commentedon Sep 29, 2018
That's good to know @jbrockmendel. I think there are some benchmarks that are failing too. I'll see if I can create an issue for them later. Do you want to create an issue for the linting of directives? It'd be nice to get #22863 merged first, as it refactors the whole linting, but I'll fix the conflicts if someone is so fast to implement the linting before. :)