Skip to content
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

Fix ASV imports #23085

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Fix ASV imports #23085

merged 1 commit into from
Oct 11, 2018

Conversation

h-vetinari
Copy link
Contributor

#22947 broke the ASV - second time in a few days that linting the benchmarks has broken something (see #22978 / #22886)

@datapythonista, could you please require that PRs try

cd asv_bench
asv dev

to see if collection of benchmark works? This takes about 30 seconds and then the asv run can be aborted with CTRL+C.

@pep8speaks
Copy link

Hello @h-vetinari! Thanks for submitting the PR.

@codecov
Copy link

codecov bot commented Oct 11, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #23085   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files         169      169           
  Lines       50911    50911           
=======================================
  Hits        46939    46939           
  Misses       3972     3972
Flag Coverage Δ
#multiple 90.61% <ø> (ø) ⬆️
#single 42.3% <ø> (ø) ⬆️

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 296c251...0caad80. Read the comment docs.

@jorisvandenbossche jorisvandenbossche changed the title Fix ASV again Fix ASV imports Oct 11, 2018
@jorisvandenbossche jorisvandenbossche merged commit b28cf5a into pandas-dev:master Oct 11, 2018
@jorisvandenbossche
Copy link
Member

Thanks!
We should indeed be more careful about that (I think a rebase went wrong in the end of that previous pull request)

@jorisvandenbossche jorisvandenbossche added this to the 0.24.0 milestone Oct 11, 2018
@jorisvandenbossche jorisvandenbossche added the Benchmark Performance (ASV) benchmarks label Oct 11, 2018
@h-vetinari h-vetinari mentioned this pull request Oct 11, 2018
3 tasks
@datapythonista
Copy link
Member

In theory the previous changes were tested. I think we need to add asv dev in the CI to make sure we don't break them again. I think asv creates a machine the first time it runs, not sure if that can be avoided, I'll do some research.

@mroeschke
Copy link
Member

@datapythonista you may want to start by seeing if this script that checks for failed asv benchmarks can run on Azure. We tried running it on travis but it would timeout.

https://github.com/pandas-dev/pandas-ci/blob/master/ci/asv.sh

@datapythonista
Copy link
Member

Thanks @mroeschke, I didn't know about that script. I see one problem is that asv dev exits with status 0 even if some benchmarks fail. I need to check in more detail how does the script know that the benchmarks are failing, but will research and give it a try in azure.

@h-vetinari h-vetinari deleted the fix_asv branch October 11, 2018 20:11
@mroeschke
Copy link
Member

Yeah it's a bit fragile in that it just greps for "failed" from the asv dev output. There may be a better approach.

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
@datapythonista
Copy link
Member

@h-vetinari @mroeschke since #22854 we do a dry-run of the benchmarks to test that nothing is broken in the CI. We do it only if something in the benchmarks directory has changed, as it takes too long to do it for every PR/commit. I'm using the same grep, couldn't find a better way.

@h-vetinari
Copy link
Contributor Author

@datapythonista Cool, thanks for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmark Performance (ASV) benchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants