-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-45716: [R][CI] Refactor skip_on_python_older_than to not initialize reticulate #46079
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
|
@github-actions crossbow submit test-r-offline-maximal |
|
Revision: c76756e Submitted crossbow builds: ursacomputing/crossbow @ actions-d817e71ea4
|
|
Crossbow job passes, I see the test we expect to be skipped skips when checking Python version, I'm going to exercise the new helper code by installing a newer Python version in the runner temporarily. |
|
@github-actions crossbow submit test-r-offline-maximal |
|
Revision: c82635de9d908644f7476f20c5c752df9c13fdc8 Submitted crossbow builds: ursacomputing/crossbow @ actions-accaa08b45
|
797afd3 to
c76756e
Compare
|
I had hoped to exercise the new helper code (I still can) but I figured something out that makes me think we can just merge this as-is. The skip I mentioned above was actually because "Python not available" not because we fail the version check as I had hoped. This happens due to what appears to be a bug in reticulate which is confusing because it's a very basic thing to have not work. The code in this PR uses However, in a VM with R and This seems to be a bug and may explain why none of our CI jobs actually run the reticulate tests @jonkeane. If I explicitly tell reticulate which Python to use, this new helper works fine, |
Do our force all tests checks actually run this? Or are those skipping there too? |
jonkeane
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 for this. Yeah, I definitely agree this is an improvement even if it's not totally what we were hoping for
It seems so, in the logs of the I'm not sure reticulate picks up |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 409a016. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…tialize reticulate (apache#46079) ### Rationale for this change This fixes our `test-r-offline-maximal` job which started failing because of a change in reticulate 1.41.0. ### What changes are included in this PR? Changed internals of our skip_on_python_older_than helper. ### Are these changes tested? Locally, need to test in CI before merging. ### Are there any user-facing changes? No. * GitHub Issue: apache#45716 Authored-by: Bryce Mecum <[email protected]> Signed-off-by: Bryce Mecum <[email protected]>
Rationale for this change
This fixes our
test-r-offline-maximaljob which started failing because of a change in reticulate 1.41.0.What changes are included in this PR?
Changed internals of our skip_on_python_older_than helper.
Are these changes tested?
Locally, need to test in CI before merging.
Are there any user-facing changes?
No.