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

setup_cache identified by code location, not by class it belongs to cannot benefit from delegation in subclasses #880

Open
yarikoptic opened this issue Sep 10, 2019 · 2 comments
Labels
idea Low-priority enhancement suggestion

Comments

@yarikoptic
Copy link
Contributor

In my use case (datalad) I thought to have a generic setup_cache defined in a base class which would then provide reusable logic in subclasses. E.g. a simplified example:

$> cat benchmarks/check_asv.py
class Base:
 
    def sub_specific(self):
        raise NotImplementedError()

    def setup_cache(self):
        # load data from a file
        ret = self.sub_specific()
        print("setup_cache %s for %s" % (ret, self))


class Benchmarks:
    def time_upper(self):
        i = 1

class Suite1(Base, Benchmarks):
    def sub_specific(self):
        return "Suite1"

class Suite2(Base, Benchmarks):
    def sub_specific(self):
        return "Suite2"

but to my surprise setup_cache was ran only once, e.g.:

$> asv run --bench Suite[12] --python=same
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_home_yoh_proj_datalad_datalad-master_venvs_dev3_bin_python3
[ 25.00%] ··· Setting up check_asv.py:6                                                                       ok
[ 25.00%] ··· Running (check_asv.Suite1.time_upper--)..
[ 75.00%] ··· check_asv.Suite1.time_upper                                                              93.6±30ns
[100.00%] ··· check_asv.Suite2.time_upper                                                               115±30ns

That non-descriptive "Setting up check_asv.py:6 " is the call to setup_cache. This time it is of Suite1 (if running with -v), but would be of Suite2 if I run only Suite2 benchmark.

So setup_cache is indeed identified by code location (which would be ok if just a module bound function) and not by class, which imho would be correct behavior when it is a bound (not static) method. Or that would be somehow against its intended usecase?

asv 0.4.1

@jeremiedbb
Copy link
Contributor

I have the same use case. For now I worked around doing:

class Suite1(Base, Benchmarks):
    def setup_cache(self):
        super().setup_cache()

    def sub_specific(self):
        return "Suite1"

and same for Suite2, but it's not ideal.

@flexatone
Copy link

Many thanks to the ASV maintainers for this excellent tool.

I was similarly surprised that setup_cache on derived benchmark classes does not work as expected. Using derived classes also makes providing distinct pretty_name (as a method attributes) per class impossible.

For these reasons, perhaps the docs should make explicit that subclassing benchmark classes is not recommended?

I found a way to get reuse by using a class decorator to populate methods (from a Prototype class) on classes that need to share benchmark functions but use different data sets provided by distinct setup_cache. Thankfully, I can use pretty_source to provide the function definition from Prototype.

https://github.com/InvestmentSystems/static-frame-benchmark/blob/master/benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Low-priority enhancement suggestion
Projects
None yet
Development

No branches or pull requests

4 participants