-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48912: [R] Configure C++20 in conda R on continuous benchmarking #48974
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
Conversation
|
|
|
I did some more digging with 🤖 and ended up here @jonkeane - I spent some time asking questions and getting verification from code, so it's a bit over my head, so let me know if this sounds feasible to you!
|
|
That sounds ok to me if it works! |
|
@ursabot please benchmark |
|
Benchmark runs are scheduled for commit f164e4d. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
|
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit f164e4d. There were 2 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
Looks like the benchmarks are building - I think the one which failed is a timeout though unsure why. |
pitrou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @thisisnic . This looks good to me! Just one question.
| cat ci/etc/rprofile >> $(R RHOME)/etc/Rprofile.site | ||
|
|
||
| # Ensure CXX20 is configured in R's Makeconf. | ||
| # conda-forge's R may have empty CXX20 entries even though the compiler supports it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we report an issue to the corresponding conda-forge feedstock, so that we can get rid of this workaround later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure it was a bug but reported it at conda-forge/r-base-feedstock#401
We'll see if that reproduces in later benchmark runs, but this PR can certainly be merged anyway. |
Rationale for this change
Benchmark failing since C++20 upgrade due to lack of C++20 configuration
What changes are included in this PR?
Changes entirely from 🤖 (Claude) with discussion from me regarding optimal approach.
Description as follows:
Are these changes tested?
I got 🤖 to try it locally in a container but I'm not convinced we'll know for sure til we try it out properly.
Are there any user-facing changes?
Nope