Skip to content

BENCH pin dependencies in asv benchmarks #27264

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 3 commits into from
Sep 6, 2023

Conversation

jeremiedbb
Copy link
Member

From the discussions in #27086

Currently the asv benchmarks run against the latest available versions of the dependencies.
This PR allows to easily distinguish between perf regressions due to a change in the codebase from perf regressions due to an upgrade of some dependency. This way, bumping a dependency in the benchmarks will correspond to a specific commit in the history.

For now I pinned cython < 3 because of the perf regressions we're observing in #27086. Then I propose to make a new PR after that bumping to cython 3 to have a specific commit identifying the cython 3 regressions.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: c9f85be. Link to the linter CI: here

@jjerphan jjerphan added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Sep 2, 2023
@betatim
Copy link
Member

betatim commented Sep 4, 2023

I think it makes sense to do this.

What do you think of documenting somewhere close to where the other lock files are generated that the benchmark dependencies are controlled from somewhere else? Worth it? I'm -1 on trying to automated it or also control the benchmark dependencies from the other lock file generation stuff, but unsure about documentation. Maybe ASV enthusiasts know and expect that dependencies are set in the config file (which would make it redundant to document it elsewhere)

@jeremiedbb
Copy link
Member Author

Well I'm not sure where to document that. We don't necessarily want to update these ones at the same time that we udate the lock files. But on the other hand there's a risk to forget to update them for a while if we don't write it somewhere.

@betatim
Copy link
Member

betatim commented Sep 5, 2023

I don't have a great idea where to put the documentation either. So maybe let's not for now unless someone comments with a smart idea.

@jjerphan
Copy link
Member

jjerphan commented Sep 5, 2023

Building on the proposal of Tim in #27264 (comment), if #27266 is approved, we can add a comment next to the Cython specification indicating that the asv configuration might need to be updated as well.


Unrelated: Generally, I think we would be nice to have "Community Reminders", indicating updates or changes that we might want to perform at given frequencies.

@jeremiedbb
Copy link
Member Author

I added a comment in the asv conf, it doesn't hurt.

@jeremiedbb
Copy link
Member Author

Unrelated: Generally, I think we would be nice to have "Community Reminders", indicating updates or changes that we might want to perform at given frequencies.

related to #27038

@betatim
Copy link
Member

betatim commented Sep 6, 2023

I'll enable auto-merge for this.

@betatim betatim enabled auto-merge (squash) September 6, 2023 14:17
@betatim betatim merged commit 95778fb into scikit-learn:main Sep 6, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants