You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/12 21:27:52 UTC
[GitHub] [arrow] alistaire47 opened a new pull request, #12867: ARROW-14168: [R] Warn only once about arrow function differences
alistaire47 opened a new pull request, #12867:
URL: https://github.com/apache/arrow/pull/12867
Addresses [ARROW-14168](https://issues.apache.org/jira/browse/ARROW-14168) by changing `median()` and `quantile()` to warn once, and adjusts the tests accordingly.
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849822327
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -17,7 +17,11 @@
skip_if(on_old_windows())
-withr::local_options(list(arrow.summarise.sort = TRUE))
+withr::local_options(list(
+ arrow.summarise.sort = TRUE,
+ rlib_warning_verbosity = "verbose",
+ dplyr.summarise.inform = FALSE
Review Comment:
```suggestion
# This prevents the warning in `summarize()` about having grouped output without
# also specifying what to do with `.groups`
dplyr.summarise.inform = FALSE
```
It might be nice to have a comment about why we need the `dplyr.summarise.inform=FALSE` here so we remember. The others are more-or-less straightforward, but that one I'm not sure I would remember what causes it + where.
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849542564
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
We're also raising 10 or so warnings here that
```r
`summarise()` has grouped output by 'some_grouping'. You can override using the `.groups` argument.
```
Do we want to suppress those as well while we're at it? Or specify `.groups`?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098531476
Revision: c276e7685e41496e6619077dc88cfb0222fb06b4
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1852](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1852)
|Task|Status|
|----|------|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1852-github-test-r-versions)|
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098530648
@github-actions crossbow submit test-r-versions
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849904322
##########
r/tests/testthat/helper-arrow.R:
##########
@@ -56,6 +56,18 @@ test_that <- function(what, code) {
})
}
+# backport of 4.0.0 implementation
+suppressWarnings <- function (expr, classes = "warning") {
+ withCallingHandlers(
+ expr,
+ warning = function(w) {
+ if (inherits(w, classes)) {
+ invokeRestart("muffleWarning")
+ }
+ }
+ )
+}
Review Comment:
Could we wrap this in an `if(getRversion() < "4.0.0") {...}` So that we can continue using the official `suppressWarnings()` (with the additions of tryInvokeRestart, etc.) for versions that support it?
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098080576
Awesome, I've kicked off the CI again (sorry about the delay on that! Feel free to @ me when you need that approved again — it'll only be needed for this first PR!)
I know this wasn't in the original ticket, but would you mind also removing the lines that set the testthat edition: `local_edition(2)` and `local_edition(3)` https://github.com/apache/arrow/blob/18803eaafdea1994da42c5ea4bd8cf046057c357/r/tests/testthat/test-dplyr-summarize.R#L300-L302 https://github.com/apache/arrow/blob/18803eaafdea1994da42c5ea4bd8cf046057c357/r/tests/testthat/test-dplyr-summarize.R#L345 (and then there are at least two more later on...) and then add `SuppressWarnings()` around the tests that have multiple warnings?
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848915197
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -317,6 +319,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
+
Review Comment:
I tried `default`, but in some cases (maybe consistent? but I don't want to promise that) the `.frequency = "once"` ones still threw a warning afterwards. Easier/safer to just make it verbose at the start of the file and back to default at the end; let me try it out
--
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
[GitHub] [arrow] alistaire47 commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098531199
wait crap that has an extra `}`; fix incoming
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849542564
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
We're also raising 10 or so warnings here that
```r
`summarise()` has grouped output by 'some_grouping'. You can override using the `.groups` argument.
```
Do we want to suppress those as well while we're at it?
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849538369
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Yeah, I was trying to figure out what scope that gets in the testthat context. If it's file scope (probably, but I'm not confident), that's simpler. I'll try it and see if anything goes wrong
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849628975
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Wait we could set an option for this too instead. From `?dplyr::summarize`:
> In addition, a message informs you of that choice, unless the result is ungrouped, the option "dplyr.summarise.inform" is set to FALSE, or when summarise() is called from a function in a package.
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849628975
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Wait we could set an option for this too instead. From `?dplyr::summarize`:
```
In addition, a message informs you of that choice, unless the result is ungrouped, the option "dplyr.summarise.inform" is set to FALSE, or when summarise() is called from a function in a package.
```
--
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
[GitHub] [arrow] alistaire47 commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098483838
> Hmm, it looks like `tryInvokeRestart` isn't available prior to 3.6
Oops, we can just use `invokeRestart()`, which did exist, or backport `tryInvokeRestart()` too, or add [the backports package](https://github.com/r-lib/backports) to `Suggests` and use that (it's probably a recursive dependency already? but not sure). For now I went with the first option since it's the simplest.
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098401794
@github-actions crossbow submit test-r-versions
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098498809
Revision: 6c64ccb68e7d63d9327481b283093d98100f32f9
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1851](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1851)
|Task|Status|
|----|------|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1851-github-test-r-versions)|
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849542564
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
We're also raising 10 or so warnings here that
```r
`summarise()` has grouped output by 'some_grouping'. You can override using the `.groups` argument.
```
Do we want to suppress those as well while we're at it? Or specify `.groups`?
---
Edit: wait those are messages, not warnings
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848922126
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -317,6 +319,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
+
Review Comment:
Changed and pushed
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848887678
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -392,44 +398,38 @@ test_that("quantile()", {
"quantile() currently returns an approximate quantile in Arrow",
fixed = TRUE
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
# without groups
- expect_warning(
Review Comment:
These big diffs are just removing `expect_warning()` and dedenting everything accordingly
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848905693
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -392,44 +398,38 @@ test_that("quantile()", {
"quantile() currently returns an approximate quantile in Arrow",
fixed = TRUE
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
# without groups
- expect_warning(
Review Comment:
🙏 thanks for this heads up, it's really fantastic!
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848921771
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -317,6 +319,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
+
Review Comment:
🤔 I wonder if changing that setting actually resets the counter?
Anyway, for our purposes, during tests, we probably do want it to always end up firing cause that's easier to catch + test or ignore.
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849624997
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -341,10 +337,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
- local_edition(3)
-})
+}, classes = "arrow.median.approximate"))
Review Comment:
OOOH, that's fantastic. I don't think I knew that `suppressWarnings()` would let you specify classes, that's really nice!
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -290,16 +290,12 @@ test_that("Functions that take ... but we only accept a single arg", {
expect_error(call_binding_agg("max", 1, 2), "Multiple arguments to max()")
})
-test_that("median()", {
+test_that("median()", suppressWarnings({
Review Comment:
I like that we're not repeating `suppressWarnings()` a bunch, but I wonder if it might be better in this case to wrap each `compare_dplyr_binding()` separately so it's a bit close to the code we know will be generating too many warnings for us. I'm on the fence and fine to do either way — totally up to you!
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098302926
@github-actions crossbow submit test-r-versions
Running our version check nightly job based on https://github.com/apache/arrow/pull/12867#discussion_r849645179 to see if we might need to gate the class specification for a little while.
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098461877
Hmm, it looks like `tryInvokeRestart` isn't available prior to 3.6: https://github.com/ursacomputing/crossbow/runs/6013703013?check_suite_focus=true#step:5:21063
https://github.com/wch/r-source/blob/7d25524f8b83d63979cc18f0914a321bfbe9a819/src/library/base/R/conditions.R#L86-L92 might be helpful if we wanted to approximate the old behavior. Or maybe we should just do something like this:
```
.suppressWarnings <- suppressWarnings
suppressWarnings <- function(exprs, ...) {
if ({r version > 4.0.0}) {
.suppressWarnings(exprs, ...)
} else {
.suppressWarnings(exprs)
}
}
```
And then we don't have to worry about the internals too much?
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849645179
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -341,10 +337,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
- local_edition(3)
-})
+}, classes = "arrow.median.approximate"))
Review Comment:
Yeah, I think this was an R 4.0 thing. Ironically I was playing around with similar ideas at the same time in [rror](https://github.com/alistaire47/rror). Maybe there's still some value to that package, but I suspect it's mostly obsolete now. I should really write a blog post about the concepts, though; nobody classes conditions enough
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098540426
@github-actions crossbow submit test-r-versions
--
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
[GitHub] [arrow] jonkeane closed pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
URL: https://github.com/apache/arrow/pull/12867
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849931695
##########
r/tests/testthat/helper-arrow.R:
##########
@@ -56,6 +56,22 @@ test_that <- function(what, code) {
})
}
+# backport of 4.0.0 implementation
+suppressWarnings <- function(expr, classes = "warning") {
+ if (getRversion() < "4.0.0") {
+ withCallingHandlers(
+ expr,
+ warning = function(w) {
+ if (inherits(w, classes)) {
+ invokeRestart("muffleWarning")
+ }
+ }
+ )
+ } else {
+ base::suppressWarnings(expr = expr, classes = classes)
+ }
Review Comment:
Thoughts about
```suggestion
if (getRversion() < "4.0.0") {
suppressWarnings <- function(expr, classes = "warning") {
withCallingHandlers(
expr,
warning = function(w) {
if (inherits(w, classes)) {
invokeRestart("muffleWarning")
}
}
)
}
}
```
Minor, but slightly more simple + won't need to do anything if `suppressWarnings()` ever grows more arguments
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849585161
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Could we assert the message once and then wrap in `suppressMessages()` like we do for the warnings?
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849657132
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Oh nice, yeah maybe do a local option there setting it to false with a comment as to why | what the message is that we are suppressing
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098315988
Revision: 14d1aefceb0fd2d8e133e2f98e42cf531dfa483e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1848](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1848)
|Task|Status|
|----|------|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1848-github-test-r-versions)|
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098561696
Ok, I'm just waiting for the rest of the CI to be green. The failure in the versions https://github.com/ursacomputing/crossbow/actions/runs/2164085819 is not due to this PR, that's there in the nightlies, but we _do_ see 3.6 and 3.5 both passing, which is fantastic!
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849627375
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
Oops, just saw this; another commit incoming once I figure out where they're all coming from
--
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
[GitHub] [arrow] ursabot commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1099568876
Benchmark runs are scheduled for baseline = 5d5ccebeed155f6d6ee10cefee3cb295fc300c85 and contender = 9d24ded7f7d58717c9d78308b0c59ab7a9636006. 9d24ded7f7d58717c9d78308b0c59ab7a9636006 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/70461192e0ff481e84eb9090015aa4e2...ec04896c71ff4b8482bf36d00a87e04f/)
[Finished :arrow_down:0.46% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/b10dabec0ddc48d796c172d80802def9...24680a30cc9a4292aac0fbdc01945061/)
[Finished :arrow_down:0.71% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/248e43e2373c4fd68445f91a826eb4dc...190b365264bc48d1a96f5711875a1701/)
[Finished :arrow_down:0.55% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/c58bce7f47cb4858b252766f0c252b6f...d3d34794a0644ea3946f5da4f4407516/)
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/505| `9d24ded7` ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/492| `9d24ded7` test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/491| `9d24ded7` ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/502| `9d24ded7` ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/504| `5d5ccebe` ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/490| `5d5ccebe` test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/488| `5d5ccebe` ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/500| `5d5ccebe` ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098497882
@github-actions crossbow submit test-r-versions
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098407635
Revision: 602fe1cd36f4f3e451eb602f039ec4764904b15f
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1849](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1849)
|Task|Status|
|----|------|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1849-github-test-r-versions)|
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098541041
Revision: ea9d80bddb3d77f4426497d9564b22c16430b834
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1853](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1853)
|Task|Status|
|----|------|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1853-github-test-r-versions)|
--
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
[GitHub] [arrow] alistaire47 commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
alistaire47 commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849649224
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -290,16 +290,12 @@ test_that("Functions that take ... but we only accept a single arg", {
expect_error(call_binding_agg("max", 1, 2), "Multiple arguments to max()")
})
-test_that("median()", {
+test_that("median()", suppressWarnings({
Review Comment:
Yeah, I was on the fence too; I went this route because it's a minimal diff. It's easier to see what's going on if it's per-call, though, so I can switch it.
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r849513694
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -18,6 +18,7 @@
skip_if(on_old_windows())
withr::local_options(list(arrow.summarise.sort = TRUE))
+old_options <- options(rlib_warning_verbosity = "verbose")
Review Comment:
using `options` directly totally works, but since we have a call to `withr::local_options()` here already, does it work if we put `rlib_warning_verbosity = "verbose"` inside that list?
--
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
[GitHub] [arrow] jonkeane commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1098464682
We could also skip these tests on older versions if getting the suppress behavior is too onerous. We don't do it anymore, but when we had 3.3 support, we did it with median:
https://github.com/apache/arrow/pull/12072/files#diff-5315fa7158cd65ca9405299bf0b4551952e2555b82ea403204df2da1dc7b0013R283-L287
--
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
[GitHub] [arrow] jonkeane commented on a diff in pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
jonkeane commented on code in PR #12867:
URL: https://github.com/apache/arrow/pull/12867#discussion_r848906577
##########
r/tests/testthat/test-dplyr-summarize.R:
##########
@@ -317,6 +319,9 @@ test_that("median()", {
tbl,
warning = "median\\(\\) currently returns an approximate median in Arrow"
)
+ # silence warnings
+ rlang::local_options(rlib_warning_verbosity = "quiet")
+
Review Comment:
Hmm, this will quiet all rlang-type warnings, which is probably not quite what we want. Maybe this should be `"default"` instead of `"quiet"`? Though, this is moot if we go in the other direction up above.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12867: ARROW-14168: [R] Warn only once about arrow function differences
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12867:
URL: https://github.com/apache/arrow/pull/12867#issuecomment-1097237124
https://issues.apache.org/jira/browse/ARROW-14168
--
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