Skip to content
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

revdep issues with R-devel is.atomic #5733

Closed
tdhock opened this issue Nov 8, 2023 · 4 comments
Closed

revdep issues with R-devel is.atomic #5733

tdhock opened this issue Nov 8, 2023 · 4 comments
Assignees
Labels
revdep Reverse dependencies
Milestone

Comments

@tdhock
Copy link
Member

tdhock commented Nov 8, 2023

Recent merge of #5691 has caused two new revdep checks to fail.

revdep TheOpenAIR


* checking tests ...
 ERROR
Running the tests in 'tests/testthat.R' failed.
Last 13 lines of output:
  -- Error ('test-count_tokens.R:12:3'): count_tokens() correctly counts the number of tokens in a text string --
  Error: 'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)
  Backtrace:
      x
   1. +-testthat::expect_equal(count_tokens(""), 0) at test-count_tokens.R:12:3
   2. | \-testthat::quasi_label(enquo(object), label, arg = "object")
   3. |   \-rlang::eval_bare(expr, quo_get_env(quo))
   4. \-TheOpenAIR::count_tokens("")
   5.   +-tokens_dt[, .N, by = NULL]
   6.   \-data.table:::`[.data.table`(tokens_dt, , .N, by = NULL)
   7.     \-data.table:::stopf("'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)")
  
  [ FAIL 1 | WARN 0 | SKIP 19 | PASS 51 ]
  Error: Test failures
  Execution halted

revdep sdcLog:


* checking examples ... ERROR
Running examples in 'sdcLog-Ex.R' failed
The error most likely occurred in:

> ### Name: sdc_descriptives
> ### Title: Disclosure control for descriptive statistics
> ### Aliases: sdc_descriptives
> 
> ### ** Examples
> 
> sdc_descriptives(
+   data = sdc_descriptives_DT,
+   id_var = "id",
+   val_var = "val_1"
+ )
Error in `[.data.table`(dt, id_na == TRUE, list(value_share_na = value_share),  : 
  'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)
Calls: sdc_descriptives -> check_dominance -> [ -> [.data.table
Execution halted
...

* checking re-building of vignette outputs ... ERROR
Error(s) in re-building vignettes:
  ...
--- re-building 'FAQ.Rmd' using rmarkdown
--- finished re-building 'FAQ.Rmd'

--- re-building 'intro.Rmd' using rmarkdown

Quitting from lines 115-116 [descriptives_simple] (intro.Rmd)
Error: processing vignette 'intro.Rmd' failed with diagnostics:
'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)
--- failed re-building 'intro.Rmd'

--- re-building 'options.Rmd' using rmarkdown

Quitting from lines 52-53 [example1_sdc.n_ids] (options.Rmd)
Error: processing vignette 'options.Rmd' failed with diagnostics:
'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)
--- failed re-building 'options.Rmd'

SUMMARY: processing the following files failed:
  'intro.Rmd' 'options.Rmd'

Error: Vignette re-building failed.
Execution halted

I assume these should be fixed in revdeps, but I have not analyzed them yet.

@tdhock tdhock self-assigned this Nov 8, 2023
@tdhock tdhock added the revdep Reverse dependencies label Nov 8, 2023
@ben-schwen
Copy link
Member

ben-schwen commented Nov 9, 2023

@tdhock I don't know why but I can only reproduce the error of TheOpenAIR with 1.14.8 and not with master branch.

edit: I misread the output of revdep, current master is fine thanks to #5691, CRAN and release are failing due to the changes of is.atomic in Rdevel. Same holds for sdcLog

library(data.table)
dt = data.table(tokens=character())
dt[,.N, by=NULL]
#> Error in `[.data.table`(dt, , .N, by = NULL): 'by' or 'keyby' must evaluate to a vector or a list of vectors (where 'list' includes data.table and data.frame which are lists, too)
sessionInfo()
#> R Under development (unstable) (2023-11-08 r85496)
#> Platform: x86_64-pc-linux-gnu
#> Running under: Ubuntu 20.04.3 LTS
#> 
#> Matrix products: default
#> BLAS:   /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0 
#> LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0
#> 
#> locale:
#>  [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
#>  [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
#>  [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
#>  [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
#>  [9] LC_ADDRESS=C               LC_TELEPHONE=C            
#> [11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       
#> 
#> time zone: Europe/Vienna
#> tzcode source: system (glibc)
#> 
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> 
#> other attached packages:
#> [1] data.table_1.14.8
#> 
#> loaded via a namespace (and not attached):
#>  [1] digest_0.6.33   fastmap_1.1.1   xfun_0.41       glue_1.6.2     
#>  [5] knitr_1.45      htmltools_0.5.7 rmarkdown_2.25  lifecycle_1.0.4
#>  [9] cli_3.6.1       reprex_2.0.2    withr_2.5.2     compiler_4.4.0 
#> [13] tools_4.4.0     evaluate_0.23   yaml_2.3.7      rlang_1.1.2    
#> [17] fs_1.6.3

@jangorecki jangorecki added this to the 1.14.10 milestone Nov 9, 2023
@jangorecki jangorecki added the dev label Nov 9, 2023
@jangorecki
Copy link
Member

Thanks Ben. Confirming it is already fixed in devel. Another reason to release hotfix ASAP.

docker pull rocker/r-devel
docker run --rm -it rocker/r-devel /bin/bash
Rdevel
install.packages("sdcLog")
library("sdcLog")
example("sdc_descriptives", echo=FALSE)
#error
q("no")

Rdevel
data.table::update_dev_pkg(repo="https://rdatatable.github.io/data.table")
library("sdcLog")
example("sdc_descriptives", echo=FALSE)
#no error

Expect to wait a bit due to many heavy dependencies that needs to be installed.
I checked only sdcLog because other one has even more dependencies.

tools::package_dependencies(c("sdcLog","TheOpenAIR"), recursive=TRUE)
$sdcLog
 [1] "broom"      "checkmate"  "cli"        "data.table" "mathjaxr"  
 [6] "stats"      "utils"      "backports"  "dplyr"      "ellipsis"  
[11] "generics"   "glue"       "lifecycle"  "purrr"      "rlang"     
[16] "stringr"    "tibble"     "tidyr"      "methods"    "magrittr"  
[21] "pillar"     "R6"         "tidyselect" "vctrs"      "stringi"   
[26] "fansi"      "pkgconfig"  "cpp11"      "grDevices"  "utf8"      
[31] "tools"      "withr"      "graphics"  

$TheOpenAIR
 [1] "reticulate"  "magrittr"    "tibble"      "methods"     "stringr"    
 [6] "stringi"     "data.table"  "httr"        "R.utils"     "xml2"       
[11] "utils"       "cli"         "rstudioapi"  "R.oo"        "tools"      
[16] "R.methodsS3" "curl"        "jsonlite"    "mime"        "openssl"    
[21] "R6"          "Matrix"      "Rcpp"        "RcppTOML"    "graphics"   
[26] "here"        "png"         "rappdirs"    "rlang"       "withr"      
[31] "stats"       "glue"        "lifecycle"   "vctrs"       "fansi"      
[36] "pillar"      "pkgconfig"   "grDevices"   "grid"        "lattice"    
[41] "rprojroot"   "askpass"     "utf8"        "sys"        

@jangorecki jangorecki removed the dev label Nov 9, 2023
@tdhock
Copy link
Member Author

tdhock commented Nov 12, 2023

you are both correct.
The issue does not happen using data.table master with R-devel.
This issue only appears when using data.table release with R-devel.
It was my mistake for incorrectly interpreting the revdep results.
I saw that there was a significant difference between DT master/release using R-devel, and usually this means a regression in master, but this time it meant a fix in master, which is apparent by looking at this line of the revdep check output:

> (sig.diff.dt <- myDiff(Rvers))
   checking master release
1:    tests     OK   ERROR

Sorry for the confusion. I have updated https://github.com/Rdatatable/data.table/wiki/Release-management-and-revdep-checks to explain that we should check to make sure the issue is in master (not release).

@jangorecki
Copy link
Member

No worries. Running revdeps regularly is an amazing improvement in our testing process. False positives will surely be happening because scale is already big enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
revdep Reverse dependencies
Projects
None yet
Development

No branches or pull requests

3 participants