Skip to content

STYLE: Fix linting of benchmarks #22884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
datapythonista opened this issue Sep 29, 2018 · 7 comments · Fixed by #22886
Closed

STYLE: Fix linting of benchmarks #22884

datapythonista opened this issue Sep 29, 2018 · 7 comments · Fixed by #22886
Labels
Benchmark Performance (ASV) benchmarks CI Continuous Integration Code Style Code style, linting, code_checks good first issue

Comments

@datapythonista
Copy link
Member

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:.

@datapythonista datapythonista added CI Continuous Integration Code Style Code style, linting, code_checks Effort Low good first issue labels Sep 29, 2018
@TomAugspurger
Copy link
Contributor

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
Copy link
Contributor

@TomAugspurger Is it the case with :
asv_bench/benchmarks/join_merge.py:32:9: E722 do not use bare except' ?

@datapythonista
Copy link
Member Author

@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, using Except is not as important as in the code. We'll just get the benchmarks wrong, but the pandas code won't be affected.

@MatanCohe
Copy link
Contributor

@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 in join_merge.py:32:9 so I want to check if I'm missing something.

@datapythonista
Copy link
Member Author

I'm not quite sure what df.consolidate(inplace=True) can raise tbh, but I don't think it should be ImportError.

I think the best is to replace the pass in the except by a raise, 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 that try/except)

@jbrockmendel
Copy link
Member

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 defines goal_time, but it looks like asv just ignores that: https://asv.readthedocs.io/en/stable/benchmarks.html#benchmark-attributes

So the pertinent suggestion: linting for valid asv directives

@datapythonista
Copy link
Member Author

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmark Performance (ASV) benchmarks CI Continuous Integration Code Style Code style, linting, code_checks good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants