Skip to content

Fix time_diff* and time_remove benchmarks to account for long RFed interfaces #7502

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

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

yarikoptic
Copy link
Member

Only with fresh asv 0.6.0 which started to exit with exit code 2 if any benchmark failed, we finally had to pay attention to those long failing benchmarks. We have removed the option 'revision' so long ago but first didn't mention and then just ignored. For that and time_remove I also made it into dataset method invocation since otherwise it was not finding the dataset any longer given the path to it (didn't investigate deeper).

…d revision arg etc

Only with fresh asv 0.6.0 which started to exit with exit code 2 if any
benchmark failed, we finally had to pay attention to those long failing
benchmarks. We have removed the option 'revision' so long ago but first didn't
mention and then just ignored. For that and time_remove I also made it into
dataset method invocation since otherwise it was not finding the dataset any
longer given the path to it (didn't investigate deeper).
@yarikoptic yarikoptic added semver-internal Changes only affect the internal API CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Oct 4, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Oct 4, 2023
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (bfcc8a5) 91.55% compared to head (a1fc3e3) 91.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7502      +/-   ##
==========================================
+ Coverage   91.55%   91.56%   +0.01%     
==========================================
  Files         325      325              
  Lines       43443    43443              
  Branches     5827     5827              
==========================================
+ Hits        39774    39780       +6     
+ Misses       3654     3648       -6     
  Partials       15       15              

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mih
Copy link
Member

mih commented Oct 9, 2023

It still exists with code 2. The output is so long that it is not very accessible why this is the case.

@yarikoptic
Copy link
Member Author

I think it's is hailing since it is the second run which uses main branch version of benchmark which isn't fixed - just search for "failed" to find

failed
2023-10-09T01:24:56.2147749Z [54.05%] ···· BM: Setup ran in /tmp/tmpx70kjxrv, existing paths: ['testds1.tar', 'cache.pickle']
2023-10-09T01:24:56.2149462Z               BM: Finished setup for /tmp/datalad_temp_bm_tekk6bcl
2023-10-09T01:24:56.2152249Z               BM: Cleaning up 1 paths
2023-10-09T01:24:56.2155283Z               Traceback (most recent call last):
2023-10-09T01:24:56.2172243Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/asv_runner/server.py", line 179, in _run_server
2023-10-09T01:24:56.2172870Z                   _run(run_args)
2023-10-09T01:24:56.2173636Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/asv_runner/run.py", line 72, in _run
2023-10-09T01:24:56.2174121Z                   result = benchmark.do_run()
2023-10-09T01:24:56.2174836Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/asv_runner/benchmarks/_base.py", line 661, in do_run
2023-10-09T01:24:56.2175376Z                   return self.run(*self._current_params)
2023-10-09T01:24:56.2176090Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/asv_runner/benchmarks/time.py", line 172, in run
2023-10-09T01:24:56.2176749Z                   min_run_count=self.min_run_count,
2023-10-09T01:24:56.2177807Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/asv_runner/benchmarks/time.py", line 258, in benchmark_timing
2023-10-09T01:24:56.2178316Z                   timing = timer.timeit(number)
2023-10-09T01:24:56.2178809Z                 File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/timeit.py", line 177, in timeit
2023-10-09T01:24:56.2179272Z                   timing = self.inner(it, self.timer)
2023-10-09T01:24:56.2179785Z                 File "<timeit-src>", line 6, in inner
2023-10-09T01:24:56.2180266Z                 File "/home/runner/work/datalad/datalad/benchmarks/api.py", line 110, in time_diff
2023-10-09T01:24:56.2180737Z                   diff(self.ds.path, revision="HEAD^")
2023-10-09T01:24:56.2181244Z                 File "/home/runner/work/datalad/datalad/datalad/interface/base.py", line 773, in eval_func
2023-10-09T01:24:56.2181707Z                   return return_func(*args, **kwargs)
2023-10-09T01:24:56.2182202Z                 File "/home/runner/work/datalad/datalad/datalad/interface/base.py", line 763, in return_func
2023-10-09T01:24:56.2182641Z                   results = list(results)
2023-10-09T01:24:56.2183160Z                 File "/home/runner/work/datalad/datalad/datalad/interface/base.py", line 875, in _execute_command_
2023-10-09T01:24:56.2183609Z                   cmd(*cmd_args, **cmd_kwargs),
2023-10-09T01:24:56.2184123Z               TypeError: __call__() got an unexpected keyword argument 'revision'
2023-10-09T01:24:56.2184554Z               asv: benchmark failed (exit status 1)
2023-10-09T01:24:56.2184779Z 
2023-10-09T01:24:56.2185115Z [55.41%] ··· api.supers.time_diff_recursive                              failed

Overall, I would merge and go from there

@yarikoptic
Copy link
Member Author

ok, I am taking responsibility to fix it up further if there is more to do after the merge... let's proceed to make CI green again!

@yarikoptic yarikoptic merged commit a96c51c into datalad:maint Oct 9, 2023
@yarikoptic
Copy link
Member Author

ok, I am taking responsibility to fix it up further if there is more to do after the merge... let's proceed to make CI green again!

@yarikoptic yarikoptic deleted the bf-benchmarks branch November 27, 2023 16:22
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants