Check smc-mallows branch on rhub using valgrind

wleoncio commented 3 years ago

Following a recommendation from Dan during our last meeting, we're doing a one-off check of the smc-mallows branch on R-hub using Valgrind to check some extra memory and boundary issues on the C++ code.

Local checks using R CMD check --as-cran --use-valgrind returned no errors. As a precaution, we would also like to run it once on R-hub as well. However, a run of rhub::check_with_valgrind() returns the following (email address redacted, but it does match the package maintainer's email in the DESCRIPTION):

‘o***5@gmail.com’ is not validated, or does not match the package maintainer's email. To validate it now, please enter the email address below. Note that R-hub will send a token to this address. If the address does not belong to you, quit now by pressing ENTER .

So this seems like an operation restricted to the package maintainer. @osorensen, would you please give it a try?

osorensen commented 3 years ago

Sure, I'll pull the smc-mallows branch and check with valgrind.

osorensen commented 3 years ago

It failed with this message (full output https://builder.r-hub.io/status/original/BayesMallows_1.0.2.9005.tar.gz-92ae44c9e9a245ad9d8334db5daf595b):

checking tests ... Running ‘testthat.R’Build timed out (after 20 minutes). Marking the build as failed. Build was aborted Pinging https://builder.r-hub.io/build/FAILURE/BayesMallows_1.0.2.9005.tar.gz-92ae44c9e9a245ad9d8334db5daf595b/2021-06-23T09:49:27Z {"status":"ok"} Finished: FAILURE

osorensen commented 3 years ago

I'll check the master branch with valgrind as well, to confirm that that works fine.

osorensen commented 3 years ago

Haven't run the master branch on valgrind yet, but when running devtools::test() on the smc-mallows branch I get the following warning:

✓ |  13   1   | SMC-Mallows complete rankings: sequence [7.2 s]                                                                       
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Warning (test_smc_mallows_complete_rankings.R:81:1): (code run outside of `test_that()`)
`funs()` was deprecated in dplyr 0.8.0.
Please use a list of either functions or lambdas: 

  # Simple named list: 
  list(mean = mean, median = median)

  # Auto named with `tibble::lst()`: 
  tibble::lst(mean, median)

  # Using lambdas
  list(~ mean(., trim = .2), ~ median(., na.rm = TRUE))
This warning is displayed once every 8 hours.
Call `lifecycle::last_warnings()` to see where this warning was generated.
Backtrace:
 1. BayesMallows::compute_rho_consensus(...) test_smc_mallows_complete_rankings.R:81:0
 2. BayesMallows::compute_consensus_smc(...) /Users/oyss/Repos/BayesMallows/R/smc_post_processing_functions.R:445:4
 3. BayesMallows::.compute_cp_consensus_smc(model_fit, burnin = burnin) /Users/oyss/Repos/BayesMallows/R/smc_post_processing_functions.R:254:4
 8. dplyr::funs(as.character)

I don't get this error on the master branch.

osorensen commented 3 years ago

So the problem seems to lie in these lines of the post processing: https://github.com/ocbe-uio/BayesMallows/blob/04f2a566bba3a668cbda3206ff9dacff4c8aa783/R/smc_post_processing_functions.R#L289-L290

It does not like dplyr::funs.

osorensen commented 3 years ago

I suggest you update your dplyr package to confirm that you also get this warning, then I'll run valgrind again once the smc-mallows branch has been updated.

osorensen commented 3 years ago

Can also confirm that valgrind finished with status "SUCCESS" on the master branch.

osorensen commented 3 years ago

@wleoncio, I also notice that running tests on the smc-mallows branch takes very long. I suggest reducing the number of Monte Carlo samples considerably, for example here: https://github.com/ocbe-uio/BayesMallows/blob/04f2a566bba3a668cbda3206ff9dacff4c8aa783/tests/testthat/test_smc_mallows_complete_rankings.R#L27-L36

It should be enough to have, e.g., burnin = 5 and nmc = 20, and then checkout some exact numbers in the output, conditional on setting the seed.

wleoncio commented 3 years ago

Hi Øystein,

I confirm that I get that dplyr::funs warning. I was ignoring it for now because that fix required some code restructuring (plus some major dusting-off of my dplyr skills), but I guess now is as good a time as any to tacke this. Also, I don't remember about R-hub, but these online tools tend to default to treating warnings as fatal errors, so it sounds proper to wait for this (i.e. issue #99) to be resolved before proceeding.

Thank you for the tip on how to reduce the runtime of the smc_mallows_complete_rankings test. I was aware of its length and the importance of reducing it, but was afraid to do so until I was confident the function was working as expected. I'll implement the optimizations you recommended (see issue #100).

wleoncio commented 3 years ago

I suggest you update your dplyr package to confirm that you also get this warning, then I'll run valgrind again once the smc-mallows branch has been updated.

All clear to proceed, deprecated function was removed on 2c732e06.

osorensen commented 3 years ago

Still fails. I suspect it happens because the tests take too long to run.

checking for unstated dependencies in ‘tests’ ... OK

checking tests ... Running ‘testthat.R’Build timed out (after 20 minutes). Marking the build as failed. Build was aborted Pinging https://builder.r-hub.io/build/FAILURE/BayesMallows_1.0.2.9005.tar.gz-ffbc6a0f9b5e4f978c64d9b73ae1e918/2021-06-24T11:22:22Z {"status":"ok"} Finished: FAILURE

Here is the full output https://builder.r-hub.io/status/original/BayesMallows_1.0.2.9005.tar.gz-ffbc6a0f9b5e4f978c64d9b73ae1e918

On my Mac, running system.time({devtools::test()}) returns the following on the smc-mallows branch:

> system.time({devtools::test()})
(...)
   user  system elapsed 
 30.016   0.367  30.694

On the master branch it returns this:

> system.time({devtools::test()})
(...)
   user  system elapsed 
 19.715   0.444  20.464

So, the tests on the smc-mallows branch take just 50 % longer to run than the tests on the master branch. However, since valgrind seems to run so slow, I'll try again now, but disable all tests except the ones related to smc-mallows, and see if that works.

osorensen commented 3 years ago

@wleoncio , valgrind has now successfully run. I removed all the non-smc unit tests.

Build step 'Send files or execute commands over SSH' changed build result to SUCCESS Pinging https://builder.r-hub.io/build/SUCCESS/BayesMallows_1.0.2.9005.tar.gz-0e7a91b85def426d8db54fd17b9511c5/2021-06-24T12:31:06Z {"status":"ok"} Finished: SUCCESS

Closing this one, but opening a new issue for making all tests run faster.

ocbe-uio / BayesMallows

Check smc-mallows branch on rhub using valgrind #97