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 handling of input size configuration and respect default_experiment setting #117

Merged
merged 5 commits into from
Aug 15, 2019

Conversation

IsaacOscar
Copy link
Contributor

@IsaacOscar IsaacOscar commented May 13, 2019

This minor change allows variables defined for individual benchmarks to override those in the benchmark_suites. In particular, this allows individual benchmarks to have different input_sizes:

benchmark_suites:
    my-suite:
        command: "harness --input-size=%(input)s %(benchmark)s"
        benchmarks:
            - my-benchmark1: { input_sizes: [1, 2] }
            - my-benchmark2: { input_sizes: [3, 4] }

Running my-suite will now run:

my-executor harness -input-size=1 my-benchmark1
my-executor harness -input-size=2 my-benchmark1
my-executor harness -input-size=3 my-benchmark2
my-executor harness -input-size=4 my-benchmark2

Previousesly, it would run:

my-executor harness -input-size=None my-benchmark1
my-executor harness -input-size=None my-benchmark2

@coveralls
Copy link

coveralls commented May 13, 2019

Coverage Status

Coverage increased (+0.08%) to 85.881% when pulling e5a90c4 on IsaacOscar:master into b75a2a0 on smarr:master.

@IsaacOscar
Copy link
Contributor Author

I have also added a fix to respect the "default_experiment" key if it is present in the yml file. Previously, it was ignored.

@smarr
Copy link
Owner

smarr commented May 13, 2019

Hi @IsaacOscar thanks a lot for the fix.
This is indeed a bug and should have already worked. The schema explicitly allows it here: https://github.com/smarr/ReBench/blob/master/rebench/rebench-schema.yml#L116

The EXP_VARIABLES are the three things defined here: https://github.com/smarr/ReBench/blob/master/rebench/rebench-schema.yml#L81-L101

I'll likely not get to it myself this week, so, I was wondering, would it be possible to add tests for these things?
I had a somewhat similar issue earlier, and added tests for it here: https://github.com/smarr/ReBench/pull/113/files#diff-b1e4aefa3d9f06036f4772771d13c2e5

@smarr smarr added the Bug label May 13, 2019
@smarr smarr added this to the v1.0 - Foundation milestone May 13, 2019
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr smarr changed the title Use variables defined by individual benchmarks. Fix handling of input size configuration and respect default_experiment setting Aug 15, 2019
@smarr smarr self-assigned this Aug 15, 2019
@smarr
Copy link
Owner

smarr commented Aug 15, 2019

I added some tests, and the PR seems ready to merge now.

Thanks a lot Isaac!

@smarr smarr merged commit 1f81011 into smarr:master Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants