diff --git a/.Rbuildignore b/.Rbuildignore index ec14350..cd4b055 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -10,3 +10,5 @@ ^pkgdown$ ^doc$ ^Meta$ +^cran-comments\.md$ +^touchstone$ diff --git a/DESCRIPTION b/DESCRIPTION index d2cff92..cf2a6e6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Type: Package Package: touchstone Title: Continuous Benchmarking with statistical confidence based on 'Git' branches -Version: 0.0.0.9001 +Version: 0.1.3 Authors@R: c(person(given = "Lorenz", family = "Walthert", @@ -14,12 +14,14 @@ Authors@R: comment = c(ORCID = "0000-0002-7281-3989"))) Description: A common problem of benchmarking in continuous integration is that the computational power of the virtual machines that run the job - varies over time. This package allows users to benchmark two git refs - of the same repo in a random sequence for better comparison. + varies over time. This package runs benchmarking code of two 'Git' + refs of the same repository in a random sequence for better + comparison, either locally or via 'GitHub Actions' and outputs a + confidence interval for the relative difference. License: MIT + file LICENSE -URL: https://github.com/lorenzwalthert/touchstone, - https://lorenzwalthert.github.io/touchstone -BugReports: https://github.com/lorenzwalthert/touchstone/issues +URL: https://github.com/lorenzwalthert/touchstone/, + https://lorenzwalthert.github.io/touchstone/ +BugReports: https://github.com/lorenzwalthert/touchstone/issues/ Imports: bench, callr, @@ -48,7 +50,6 @@ VignetteBuilder: Config/testthat/edition: 3 Config/testthat/parallel: true Encoding: UTF-8 -LazyData: true Roxygen: list(markdown = TRUE, roclets = c("rd", "namespace", "collate", if (rlang::is_installed("pkgapi")) "pkgapi::api_roclet" else { warning("Please install r-lib/pkgapi to make sure the file API is kept diff --git a/NEWS.md b/NEWS.md index 47ab0e6..c9610ae 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -# touchstone 0.1.0-rc +# touchstone 0.1.3 This is the initial CRAN release of {touchstone}. Please see the README.md on how to get started. diff --git a/R/analyze.R b/R/analyze.R index c381901..f14a938 100644 --- a/R/analyze.R +++ b/R/analyze.R @@ -96,6 +96,8 @@ benchmark_analyze <- function(benchmark, refs = c( #' #' `refs` must be passed because the order is relevant. #' @inheritParams benchmark_plot +#' @return +#' A character vector with the text that verbalizes the benchmark. #' @keywords internal benchmark_verbalize <- function(benchmark, timings, refs, ci) { tbl <- timings %>% @@ -160,6 +162,8 @@ confint_relative_get <- function(timings, refs, reference, ci) { #' @param timing a benchmark read with [benchmark_read()], column `name` must #' only contain one unique value. +#' @return +#' The function is called for its side effects and returns `NULL` (invisibly). #' @keywords internal benchmark_plot <- function(benchmark, timings) { timings %>% diff --git a/R/core.R b/R/core.R index bae6578..601ebbc 100644 --- a/R/core.R +++ b/R/core.R @@ -5,6 +5,8 @@ #' @param n Number of iterations to run a benchmark within an iteration. #' @param dots list of quoted expressions (length 1). #' @inheritParams benchmark_write +#' @return +#' A tibble with the benchmarks. #' @importFrom tibble lst tibble #' @keywords internal benchmark_run_iteration <- function(expr_before_benchmark, @@ -109,6 +111,8 @@ benchmark_run_ref <- function(expr_before_benchmark = #' #' @param path_pkg The path to the root of the package you want to benchmark. #' @inheritParams benchmark_run_iteration +#' @return +#' A tibble with the benchmarks. #' @keywords internal benchmark_run_ref_impl <- function(ref, block, diff --git a/R/testing.R b/R/testing.R index 3832a50..66653ad 100644 --- a/R/testing.R +++ b/R/testing.R @@ -4,6 +4,8 @@ #' @inheritParams withr::defer #' @family testers #' @keywords internal +#' @return +#' The output of `withr::defer()`, i.e. a list with `expr` and `envir`. local_clean_touchstone <- function(envir = parent.frame()) { withr::defer(touchstone_clear(all = TRUE), envir = envir) } @@ -27,6 +29,8 @@ path_temp_pkg <- function(name) { #' to validate if the installed package corresponds to source branch for #' testing. If `NULL`, nothing is written. #' @inheritParams withr::defer +#' @return +#' Character vector of length one with path to package. #' @family testers #' @keywords internal local_package <- function(pkg_name = fs::path_file(fs::file_temp("pkg")), @@ -54,7 +58,7 @@ local_package <- function(pkg_name = fs::path_file(fs::file_temp("pkg")), gert::git_add("R/") gert::git_commit("[init]") branches <- gert::git_branch_list() %>% - dplyr::pull(name) %>% + dplyr::pull(.data$name) %>% dplyr::setdiff(branches, .) purrr::walk(branches, gert::git_branch_create) withr::defer(unlink(path), envir = envir) diff --git a/R/utils.R b/R/utils.R index 56353c6..23b5b71 100644 --- a/R/utils.R +++ b/R/utils.R @@ -7,7 +7,9 @@ NULL #' @describeIn touchstone_managers returns the directory where the touchstone database lives. #' @aliases touchstone_managers #' @return -#' Character vector of length one with th path to the touchstone directory. +#' Character vector of length one with the path to the touchstone directory (for +#' `dir_touchstone()`), path to the deleted files (for `touchstone_clear()`) or +#' path to the file containing the pull request comment (for `path_pr_comment()`). #' @export dir_touchstone <- function() { getOption("touchstone.dir", "touchstone") @@ -49,8 +51,6 @@ path_touchstone_script <- function() { #' @aliases touchstone_managers #' @param all Whether to clear the whole touchstone directory or just the #' records sub directory. -#' @return -#' The deleted paths (invisibly). #' @export touchstone_clear <- function(all = FALSE) { paths <- fs::path(dir_touchstone(), if (!all) c("records", "lib") else "") @@ -67,6 +67,8 @@ touchstone_clear <- function(all = FALSE) { #' always perform worse than the second, so the order within the blocks must be #' random. #' @keywords internal +#' @return +#' A tibble with columns `block` and `ref`. ref_upsample <- function(ref, n = 20) { purrr::map_dfr( rlang::seq2(1, n), @@ -121,6 +123,8 @@ local_git_checkout <- function(branch, #' #' Advantages: Keep benchmarked repo in touchstone library only. #' @keywords internal +#' @return +#' The old library paths (invisibly). local_without_touchstone_lib <- function(path_pkg = ".", envir = parent.frame()) { all <- normalizePath(.libPaths()) is_touchstone <- fs::path_has_parent( @@ -133,6 +137,9 @@ local_without_touchstone_lib <- function(path_pkg = ".", envir = parent.frame()) #' Make sure there is no installation of the package to benchmark in the global #' package library +#' @param path_pkg The path to the package to check. +#' @return +#' `TRUE` invisibly or an error if there is a global installation. #' @keywords internal assert_no_global_installation <- function(path_pkg = ".") { local_without_touchstone_lib() @@ -151,13 +158,31 @@ assert_no_global_installation <- function(path_pkg = ".") { #' Check if a package is installed and unloading it +#' @return +#' A list with `name` of the package to check and `installed`, a boolean to +#' indicate if the package is installed or not. #' @keywords internal is_installed <- function(path_pkg = ".") { path_desc <- fs::path(path_pkg, "DESCRIPTION") pkg_name <- unname(read.dcf(path_desc)[, "Package"]) list( name = pkg_name, - installed = pkg_name %in% rownames(utils::installed.packages()) + installed = is_installed_impl(pkg_name) + ) +} + +# When a package is removed from the library with [remove.packages()] and then +# loaded e.g. with [devtools::load_all()], +# [rlang::is_installed()] and similar returns `TRUE`, +# `pkg_name %in% installed.packages()` `FALSE` (but CRAN does not want to see +# `installed.packages()` used since it may be slow). +is_installed_impl <- function(pkg_name) { + rlang::with_handlers( + { + find.package(pkg_name, lib.loc = .libPaths()) + TRUE + }, + error = ~FALSE ) } @@ -311,10 +336,8 @@ get_asset_dir <- function(ref) { } -#' @describeIn touchstone_managers returns the path to the file containing the pr comment. +#' @describeIn touchstone_managers Returns the path to the file containing the pull request comment. #' @aliases touchstone_managers -#' @return -#' Character vector of length one with the path to the pr comment. #' @export #' @seealso [pr_comment] path_pr_comment <- function() { diff --git a/README.Rmd b/README.Rmd index 4a3f52b..9ab660c 100644 --- a/README.Rmd +++ b/README.Rmd @@ -3,7 +3,7 @@ output: github_document --- [![Lifecycle: -experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://www.tidyverse.org/lifecycle/#experimental) +experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://lifecycle.r-lib.org/articles/stages.html#experimental) [![R build status](https://github.com/lorenzwalthert/touchstone/workflows/R-CMD-check/badge.svg)](https://github.com/lorenzwalthert/touchstone/actions) @@ -67,7 +67,7 @@ package developers. The following insights were the motivation: least when benchmark results don't vary significantly (> 50%). - **Dependencies should be identical across versions:** R and package versions - of dependencies must be fixed via [RSPM](http://packagemanager.rstudio.com) to + of dependencies must be fixed via [RSPM](https://packagemanager.rstudio.com/client/#/) to allow as much continuation as possible anyways. Changing the timestamp of RSPM can happen in PRs that are only dedicated to dependency updates. diff --git a/README.md b/README.md index b29b6e2..8c6c5c1 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Lifecycle: -experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://www.tidyverse.org/lifecycle/#experimental) +experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://lifecycle.r-lib.org/articles/stages.html#experimental) [![R build status](https://github.com/lorenzwalthert/touchstone/workflows/R-CMD-check/badge.svg)](https://github.com/lorenzwalthert/touchstone/actions) @@ -68,9 +68,10 @@ motivation: - **Dependencies should be identical across versions:** R and package versions of dependencies must be fixed via - [RSPM](http://packagemanager.rstudio.com) to allow as much - continuation as possible anyways. Changing the timestamp of RSPM can - happen in PRs that are only dedicated to dependency updates. + [RSPM](https://packagemanager.rstudio.com/client/#/) to allow as + much continuation as possible anyways. Changing the timestamp of + RSPM can happen in PRs that are only dedicated to dependency + updates. - **Pull requests are a natural unit for measuring speed:** Linking benchmarking to pull requests make sense because you can easily diff --git a/cran-comments.md b/cran-comments.md new file mode 100644 index 0000000..ad112b6 --- /dev/null +++ b/cran-comments.md @@ -0,0 +1,47 @@ +This is a re-submission based on the feedback from Gregor Seyer received on June +18, 2021. + +## Test environments + +* local R installation, R 4.1.0 +* ubuntu 18.04 (on GitHub Actions), R 4.1.0 as well as R devel. +* Windows Server 10 (on GitHub Actions): R 4.1.0. +* win-builder (devel) + +## R CMD check results + +0 errors | 0 warnings | 2 note + +* This is a new release. +* Found the following calls to attach(): + File 'touchstone/R/core.R': + attach(loadNamespace("touchstone"), name = new_name) + See section 'Good practice' in '?attach'. + +For the latter, I followed the good practices. My package needs to be invoked +from a separate R process (via the R package callr) because library paths +cannot be cleanly removed within an R session and this function requires that +temporarily. Alternatively, I could have exported `exprs_eval` and called it +from that sub-process with `::`, but I prefer not to export it since it's not a +user-facing function. + + +In addition, I addressed all comments by Gregor Seyer. + + +* add \value to .Rd files regarding exported methods. + +> I already had that for all exported methods. You are referring to unexported +methods with the keyword internal. I think the check should be adapted to only +require return value documentation for exported methods (or adapt the standard +text above to include all documented objects). + +* You have examples for unexported functions. `is_installed()`. + +> This is a false positive. I was using `rlang::is_installed()` in an example, +not the function `is_installed()` from my own package. Nevertheless, I worked +around this. + +* You are using installed.packages(): + +> Thanks, I use your suggested way of checking now if a package is installed. diff --git a/inst/WORDLIST b/inst/WORDLIST index 9638893..2a794de 100644 --- a/inst/WORDLIST +++ b/inst/WORDLIST @@ -39,6 +39,7 @@ getwd ggplot github gitignore +Gregor hashFiles href https @@ -57,6 +58,7 @@ LIBS Lifecycle linux listWorkflowRunArtifacts +loadNamespace lorenz lorenzwalthert magrittr @@ -100,6 +102,7 @@ runif saveRDS seealso sep +Seyer SHA sideeffects sprintf diff --git a/man/assert_no_global_installation.Rd b/man/assert_no_global_installation.Rd index 7c2c885..7143fcc 100644 --- a/man/assert_no_global_installation.Rd +++ b/man/assert_no_global_installation.Rd @@ -7,6 +7,12 @@ package library} \usage{ assert_no_global_installation(path_pkg = ".") } +\arguments{ +\item{path_pkg}{The path to the package to check.} +} +\value{ +\code{TRUE} invisibly or an error if there is a global installation. +} \description{ Make sure there is no installation of the package to benchmark in the global package library diff --git a/man/benchmark_run_iteration.Rd b/man/benchmark_run_iteration.Rd index 5f1c01f..840b829 100644 --- a/man/benchmark_run_iteration.Rd +++ b/man/benchmark_run_iteration.Rd @@ -26,6 +26,9 @@ commit, tag, branch etc) of the benchmarking.} \item{n}{Number of iterations to run a benchmark within an iteration.} } +\value{ +A tibble with the benchmarks. +} \description{ Run a benchmark iteration } diff --git a/man/benchmark_run_ref_impl.Rd b/man/benchmark_run_ref_impl.Rd index a7d5a8c..10cf797 100644 --- a/man/benchmark_run_ref_impl.Rd +++ b/man/benchmark_run_ref_impl.Rd @@ -20,6 +20,9 @@ So you can use quasiquotation.} \item{path_pkg}{The path to the root of the package you want to benchmark.} } +\value{ +A tibble with the benchmarks. +} \description{ Checkout a branch from a repo and run an iteration } diff --git a/man/benchmark_verbalize.Rd b/man/benchmark_verbalize.Rd index acf13ba..7e384f5 100644 --- a/man/benchmark_verbalize.Rd +++ b/man/benchmark_verbalize.Rd @@ -6,6 +6,9 @@ \usage{ benchmark_verbalize(benchmark, timings, refs, ci) } +\value{ +A character vector with the text that verbalizes the benchmark. +} \description{ \code{refs} must be passed because the order is relevant. } diff --git a/man/is_installed.Rd b/man/is_installed.Rd index a2b4807..86a366c 100644 --- a/man/is_installed.Rd +++ b/man/is_installed.Rd @@ -6,6 +6,10 @@ \usage{ is_installed(path_pkg = ".") } +\value{ +A list with \code{name} of the package to check and \code{installed}, a boolean to +indicate if the package is installed or not. +} \description{ Check if a package is installed and unloading it } diff --git a/man/local_clean_touchstone.Rd b/man/local_clean_touchstone.Rd index 5921fd3..1091aa2 100644 --- a/man/local_clean_touchstone.Rd +++ b/man/local_clean_touchstone.Rd @@ -11,6 +11,9 @@ local_clean_touchstone(envir = parent.frame()) Typically, this should be either the current environment or a parent frame (accessed through \code{\link[=parent.frame]{parent.frame()}}).} } +\value{ +The output of \code{withr::defer()}, i.e. a list with \code{expr} and \code{envir}. +} \description{ Deletes \code{\link[=dir_touchstone]{dir_touchstone()}} when the local frame is destroyed. } diff --git a/man/local_package.Rd b/man/local_package.Rd index 2886d3c..04870fa 100644 --- a/man/local_package.Rd +++ b/man/local_package.Rd @@ -28,6 +28,9 @@ set to the package root.} Typically, this should be either the current environment or a parent frame (accessed through \code{\link[=parent.frame]{parent.frame()}}).} } +\value{ +Character vector of length one with path to package. +} \description{ Creates a package in a temporary directory and sets the working directory until the local frame is destroyed. diff --git a/man/local_without_touchstone_lib.Rd b/man/local_without_touchstone_lib.Rd index 0210c9e..5111359 100644 --- a/man/local_without_touchstone_lib.Rd +++ b/man/local_without_touchstone_lib.Rd @@ -12,6 +12,9 @@ local_without_touchstone_lib(path_pkg = ".", envir = parent.frame()) \item{envir}{The environment that triggers the deferred action on destruction.} } +\value{ +The old library paths (invisibly). +} \description{ This is useful in conjunction with \code{\link[=run_script]{run_script()}}. } diff --git a/man/ref_upsample.Rd b/man/ref_upsample.Rd index 0ff9058..9e37725 100644 --- a/man/ref_upsample.Rd +++ b/man/ref_upsample.Rd @@ -6,6 +6,9 @@ \usage{ ref_upsample(ref, n = 20) } +\value{ +A tibble with columns \code{block} and \code{ref}. +} \description{ A block is a permutation of all unique elements in \code{ref}. Then, we sample \code{n} blocks. This is better than repeating one sample a certain number of diff --git a/man/touchstone-package.Rd b/man/touchstone-package.Rd index 6377764..70e8a65 100644 --- a/man/touchstone-package.Rd +++ b/man/touchstone-package.Rd @@ -4,16 +4,21 @@ \name{touchstone-package} \alias{touchstone} \alias{touchstone-package} -\title{touchstone: Benchmark Different Git Refs in One Run} +\title{touchstone: Benchmarking Different 'Git' Refs with Confidence in One Go} \description{ -A common problem of benchmarking in continuous integration is that the computational power of the virtual machines that run the job varies over time. This package allows users to benchmark two git refs of the same repo in a random sequence for better comparison. +A common problem of benchmarking in continuous integration is + that the computational power of the virtual machines that run the job + varies over time. This package runs benchmarking code of two 'Git' + refs of the same repository in a random sequence for better + comparison, either locally or via 'GitHub Actions' and outputs a + confidence interval for the relative difference. } \seealso{ Useful links: \itemize{ - \item \url{https://github.com/lorenzwalthert/touchstone} - \item \url{https://lorenzwalthert.github.io/touchstone} - \item Report bugs at \url{https://github.com/lorenzwalthert/touchstone/issues} + \item \url{https://github.com/lorenzwalthert/touchstone/} + \item \url{https://lorenzwalthert.github.io/touchstone/} + \item Report bugs at \url{https://github.com/lorenzwalthert/touchstone/issues/} } } diff --git a/man/touchstone_managers.Rd b/man/touchstone_managers.Rd index 78cf4ba..e073c2f 100644 --- a/man/touchstone_managers.Rd +++ b/man/touchstone_managers.Rd @@ -18,11 +18,9 @@ path_pr_comment() records sub directory.} } \value{ -Character vector of length one with th path to the touchstone directory. - -The deleted paths (invisibly). - -Character vector of length one with the path to the pr comment. +Character vector of length one with the path to the touchstone directory (for +\code{dir_touchstone()}), path to the deleted files (for \code{touchstone_clear()}) or +path to the file containing the pull request comment (for \code{path_pr_comment()}). } \description{ Utilities to manage the touchstone database. @@ -33,7 +31,7 @@ Utilities to manage the touchstone database. \item \code{touchstone_clear}: clears the touchstone database. -\item \code{path_pr_comment}: returns the path to the file containing the pr comment. +\item \code{path_pr_comment}: Returns the path to the file containing the pull request comment. }} \seealso{