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

ENH: add --commit-label option for 'run' command #794

Merged
merged 4 commits into from
May 18, 2019

Conversation

jeremiedbb
Copy link
Contributor

Fixes #370

Add a --commit-label option to be used when running the benchmarks against an existing environment.
It will be used in place of the commit hash (which is None in that case) in order to be able to save the results.

@pv
Copy link
Collaborator

pv commented Feb 23, 2019

Looks ok. Needs test, however.

@pv pv added the needs-work The PR cannot be merged as is, further work required (explained in comments) label Feb 23, 2019
@jeremiedbb
Copy link
Contributor Author

It was a little bit less straightforward because the commit hash is truncated to it's 8 first characters for the results file name. However, with provided commit labels I think we want to keep their full length, because the risk of collision is quite high (for instance 'test_label_1' and 'test_label_2' would be cut to 'test_lab').

I chosed to add a prefix for the commit label and check for this prefix when dealing with the result file name. Do you think it's reasonable ?

@jeremiedbb
Copy link
Contributor Author

I added a test. I wasn't sure where to put it so I made a new test file for the run command. Is there a better place for it ?


skip_list = skipped_benchmarks[(commit_hash, env.name)]
benchmark_set = benchmarks.filter_out(skip_list)

if isinstance(env, environment.ExistingEnvironment) and commit_label is not None:
commit_hash = 'label-' + commit_label.replace('-', '_')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replace the '-' characters by '_' because the result filename is split by '-' in several places. It should avoid weird behaviors, but may modify the label provided by a user.

@jeremiedbb
Copy link
Contributor Author

The failing tests seems unrelated and are already failing on master https://travis-ci.org/airspeed-velocity/asv/jobs/506823174

@pv pv removed the needs-work The PR cannot be merged as is, further work required (explained in comments) label May 13, 2019
@pv pv force-pushed the commit-label branch from 015d566 to d293725 Compare May 16, 2019 17:28
The main use for this is to be able to run benchmarks in existing
environments.
@pv
Copy link
Collaborator

pv commented May 18, 2019

I think the main use here is to specify existing commit hashes, when using an existing environment.
Adding support for arbitrary commit labels is more complex, because e.g. the asv web interface assumes commits can be found in the repository, and the use cases for them are not clear.

So I changed this to --set-commit-hash, and removed the label prefix. The prefix also prevents the use case of using asv to run benchmarks but not manage the environment, which I think was the main motivation in #370.

@pv pv merged commit 3a0c56c into airspeed-velocity:master May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help comparing benchmarks across virtualenvs
2 participants