You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/10/31 19:03:11 UTC
[PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
paleolimbot opened a new pull request, #38534:
URL: https://github.com/apache/arrow/pull/38534
### Rationale for this change
A few rough edges exist after https://github.com/apache/arrow/pull/38236 including:
- When zero or 1 nightly with the matching major version exist, detection of the latest nightly might fail
- At least one CI job is pulling nightlies instead of using the version from the current commit
### What changes are included in this PR?
- Clean up `find_latest_nightly()` + add test
- Ensure all CI jobs are not using `find_latest_nightly()`
### Are these changes tested?
Yes (test added)
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1387466508
##########
r/tools/nixlibs.R:
##########
@@ -35,7 +35,9 @@ exit <- function(..., .status = 1) {
# checks the nightly repo for the latest nightly version X.Y.Z.100<dev>
-find_latest_nightly <- function(description_version) {
+find_latest_nightly <- function(description_version,
+ list_uri = "https://nightlies.apache.org/arrow/r/src/contrib/PACKAGES",
Review Comment:
oh yeah that makes more sense ^^
##########
r/tools/nixlibs.R:
##########
@@ -867,6 +887,8 @@ build_ok <- !env_is("LIBARROW_BUILD", "false")
# https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")
+download_libarrow_ok <- download_ok && !env_is("LIBARROW_DOWNLOAD", "false")
Review Comment:
Why this addition? Would not `LIBARROW_BINARY=false` do the same?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1787839713
:warning: GitHub issue #38430 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804526044
Revision: 371e003ba2aba3cdf4a82b967d439722758a6e00
Submitted crossbow builds: [ursacomputing/crossbow @ actions-2269729e3b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-2269729e3b)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6816655256/job/18538470045)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1792718732
Revision: 1e8d8f43a9c1ed22b0ae57714a08c6f7b05446f2
Submitted crossbow builds: [ursacomputing/crossbow @ actions-c6ce43e763](https://github.com/ursacomputing/crossbow/branches/all?query=actions-c6ce43e763)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6747608419/job/18344158148)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1387986814
##########
r/tools/nixlibs.R:
##########
@@ -867,6 +887,8 @@ build_ok <- !env_is("LIBARROW_BUILD", "false")
# https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")
+download_libarrow_ok <- download_ok && !env_is("LIBARROW_DOWNLOAD", "false")
Review Comment:
I forget what exactly it caused to fail, but I think the idea is that LIBARROW_BINARY controls whether or not static libs are attempted, sort of, and LIBARROW_DOWNLOAD strictly controls whether or not an http request is issued to get that binary. Mostly I just noticed that it was already set in the CI job ( https://github.com/apache/arrow/actions/runs/6801463295/job/18492381028?pr=38534#step:5:1094 ) and it was somewhat complicated to figure out how to wire up LIBARROW_BINARY because of the levels of abstraction on top of the R CI jobs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #38534:
URL: https://github.com/apache/arrow/pull/38534
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1388229597
##########
r/tools/nixlibs.R:
##########
@@ -44,17 +46,32 @@ find_latest_nightly <- function(description_version) {
res <- try(
{
# Binaries are only uploaded if all jobs pass so can just look at the source versions.
- urls <- readLines("https://nightlies.apache.org/arrow/r/src/contrib")
- versions <- grep("arrow_.*\\.tar\\.gz", urls, value = TRUE)
- versions <- sub(".*arrow_(.*)\\.tar\\.gz.*", "\\1", x = versions)
- versions <- sapply(versions, package_version)
- versions <- data.frame(do.call(rbind, versions))
- matching_major <- versions[versions$X1 == description_version[1, 1], ]
- latest <- matching_major[which.max(matching_major$X4), ]
- package_version(paste0(latest, collapse = "."))
+ urls <- readLines(list_uri)
+ versions <- grep("Version:\\s*.*?", urls, value = TRUE)
+ versions <- sort(package_version(sub("Version:\\s*", "\\1", versions)))
+ major_versions <- vapply(
+ versions,
+ function(x) as.integer(x[[c(1, 1)]]),
+ integer(1)
+ )
+
+ description_version_major <- as.integer(description_version[1, 1])
+ matching_major <- major_versions == description_version_major
+ if (!any(matching_major)) {
+ lg(
+ "No nightly binaries were found for version %s: falling back to libarrow build from source",
+ description_version
+ )
+
+ return(description_version)
+ }
+
+ versions <- versions[matching_major]
+ versions[[length(versions)]]
Review Comment:
Would this be equivalent here?
```suggestion
max(versions)
```
If so, it might be nicer since it's clearer that you're taking the largest available version (as opposed to the last version available, which above is sorted such that it's actually the largest), no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804960071
Thank you for taking a look! I checked this locally and I'm comfortable merging 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1805278644
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit c260a24fb17a78cde65aabec724a2dace408e5cc.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/18554217907) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1388041448
##########
r/tools/nixlibs.R:
##########
@@ -867,6 +887,8 @@ build_ok <- !env_is("LIBARROW_BUILD", "false")
# https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")
+download_libarrow_ok <- download_ok && !env_is("LIBARROW_DOWNLOAD", "false")
Review Comment:
Ah ok, fine with me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1388225995
##########
r/tools/nixlibs.R:
##########
@@ -44,17 +46,32 @@ find_latest_nightly <- function(description_version) {
res <- try(
{
# Binaries are only uploaded if all jobs pass so can just look at the source versions.
- urls <- readLines("https://nightlies.apache.org/arrow/r/src/contrib")
- versions <- grep("arrow_.*\\.tar\\.gz", urls, value = TRUE)
- versions <- sub(".*arrow_(.*)\\.tar\\.gz.*", "\\1", x = versions)
- versions <- sapply(versions, package_version)
- versions <- data.frame(do.call(rbind, versions))
- matching_major <- versions[versions$X1 == description_version[1, 1], ]
- latest <- matching_major[which.max(matching_major$X4), ]
- package_version(paste0(latest, collapse = "."))
+ urls <- readLines(list_uri)
+ versions <- grep("Version:\\s*.*?", urls, value = TRUE)
+ versions <- sort(package_version(sub("Version:\\s*", "\\1", versions)))
+ major_versions <- vapply(
+ versions,
+ function(x) as.integer(x[[c(1, 1)]]),
+ integer(1)
+ )
Review Comment:
Any reason not to do:
```suggestion
major_versions <- versions$major
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1790728054
Revision: 1c2588934ca49d4f7373475fade87e140e013c98
Submitted crossbow builds: [ursacomputing/crossbow @ actions-84648ca1ab](https://github.com/ursacomputing/crossbow/branches/all?query=actions-84648ca1ab)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6733241358/job/18301532281)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1790722230
@github-actions crossbow submit r-binary-packages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1792713905
@github-actions crossbow submit r-binary-packages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804519516
@github-actions crossbow submit r-binary-packages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1805877273
Thanks for this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1802656074
@assignUser Can you take a look at this?
I'd like to add in a comment regarding whether we do or do not intend to run detection when `source()`ing the nixlibs in the test-nixlibs.R. It's not a particularly good test if that is the intention, although I think it's fine if sourcing the file in test mode happens to run all the way though without warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #38534:
URL: https://github.com/apache/arrow/pull/38534#discussion_r1388221214
##########
r/tools/nixlibs.R:
##########
@@ -867,6 +887,8 @@ build_ok <- !env_is("LIBARROW_BUILD", "false")
# https://arrow.apache.org/docs/developers/cpp/building.html#offline-builds)
download_ok <- !test_mode && !env_is("TEST_OFFLINE_BUILD", "true")
+download_libarrow_ok <- download_ok && !env_is("LIBARROW_DOWNLOAD", "false")
Review Comment:
Would you mind adding this to the other env-var documentation we have at https://github.com/apache/arrow/blob/1f71014435fd56c915aebe0a9ac982f6e8de6f94/r/vignettes/install.Rmd#L277-L336 ?
Doing so might also clarify better for all of us the differences between these / how they work together
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804570099
Hmm...I'm also getting
```
*** Trying Arrow C++ in ARROW_HOME: /Users/deweydunnington/.r-arrow-dev-build/dist
**** Not using: C++ library version (14.0.0-SNAPSHOT) does not match R package (14.0.0.9000)
```
locally, which is problematic!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804594076
Oh just kidding...the C++ version bumped since the last time I rebuilt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38430: [R] Add test + fix corner cases after nixlibs.R refactor [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #38534:
URL: https://github.com/apache/arrow/pull/38534#issuecomment-1804880423
If the CI is good, I'm ok to merge this, though if you're looking for some other people to test this locally let me know and I'll pull it and try it (I will admit I have not yet!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org