[JOSS review] gvegayon

[gvegayon]

This review goes with #110 as well as with the JOSS review issue (link).

• [ ] Authors.: The list of authors includes Vahid Daneshmand and Quin Thomas; what was their contribution? I don't see any commits associated with them in the repo.

• [ ] Related packages. Doing a search of "AWS Lambdas" on CRAN yielded two other R packages that I failed to see mentioned in the paper: lambdr and aws.lambda. How these packages compare to FaaSr? The authors should mention these in their paper.

• [ ] Code coverage. Running covr::package_coverage(".") yields approximately 9% coverage (see below). Although I understand writing tests can be cumbersome, I would encourage you to add the coverage badge and ensure that all new commits should have a corresponding test as you continue extending/working on the R package. Again, you can defer this for after publication, but make sure you have an issue for this point (if you don't have one already).

• [ ] Many functions return "nothing", yet I see that that's not the explicit behavior, and sometimes, may be returning something; for instance, the following code returns the value 10, even when we are not doing so explicitly:

  myfun0 <- \(x) return(x)
  
  myfun1 <- \(x) {
  z <- myfun0(x)
  }
  
  x <- myfun1(10)
  x # 10

  You can use invisible(NULL) to make "nothing returned" explicit.

• [ ] Missing a test via GHA. I think the package would greatly improve if the authors included a GitHub Action in which they use the package via GHA/AWS. They could have a GHA that is triggered manually (or automatically) to check the basic functionality works.

• [ ] Improper way to call stop(). Across the package, the function stop() is called right after printing a message on the screen. The proper way to do that is to put the message as part of the stop() argument, e.g., instead of doing the following:

  msg <- ....
  message(msg)
  stop()

  do

  stop(...)

• [ ] Improper way of using on.exit.: The package incorrectly uses on.exit(). Currently, it is not inserting any bugs, but most of the time, the authors call on.exit to restore the working directory while doing so again right before returning the function. The following is an example of what the package currently does. The best practice would be leave the setwd (or whatever other operations need to be done at the end) by the on.exit() call:

  my_setwd <- \(..., which.) {
  message(which.)
  setwd(...)
  }
  
  myfun <- function(x) {
  wd <- getwd() 
  on.exit(my_setwd(wd, which. = "Last")) 
  my_setwd(file.path(wd, ".."), which. = "First")
  
  # This line shouldn't exist since `on.exit()` already does this
  my_setwd(wd, which. = "Second")
  return(x)
  
  }
  
  myfun(1)
  #> First
  #> Second
  #> Last

• [ ] Usate of tmp directories. R has a robust interface for creating temporary files and directories, namely tempfile() and tempdir(). That interface is safer as it (i) can check whether the path already exists and (ii) points to the system temp path, which can be cleared after the R session ends. Currently, the package makes artificial temporary directories manually, for instance, in the faasr() function.

• [ ] Use of paste0() for path creation. To ensure cross-os compatibility, it is recommended that paths should be created using the file.path() function. I have added a few examples in a PR I am submitting with this review.

Appendix

Output from covr

> covr::package_coverage(".")
FaaSr Coverage: 9.08%
R/faasr_abort_on_multiple_invocations.R: 0.00%
R/faasr_arrow_s3_bucket.R: 0.00%
R/faasr_client_api_aws-lambda.R: 0.00%
R/faasr_delete_file.R: 0.00%
R/faasr_get_file.R: 0.00%
R/faasr_get_folder_list.R: 0.00%
R/faasr_init_log_folder.R: 0.00%
R/faasr_lock.R: 0.00%
R/faasr_log.R: 0.00%
R/faasr_put_file.R: 0.00%
R/faasr_rank.R: 0.00%
R/faasr_run_user_function.R: 0.00%
R/faasr_s3_check.R: 0.00%
R/faasr_start.R: 0.00%
R/faasr_test.R: 0.00%
R/faasr_trigger.R: 0.00%
R/faasr_client_api_github-actions.R: 3.61%
R/faasr_client_api_openwhisk.R: 4.91%
R/faasr_client_tools.R: 24.94%
R/faasr_get_user_function_args.R: 80.00%
R/faasr_check_workflow_cycle.R: 93.62%
R/faasr_parse.R: 100.00%
R/faasr_predecessors_list.R: 100.00%

[danielskatz]

Issues related to JOSS review https://github.com/openjournals/joss-reviews/issues/7027

[renatof]

Thank you so much @gvegayon for the detailed comments! We will work on these and add a more complete comment here, but in the meantime to address your first point: both Quinn and Vahid were instrumental to the specification of requirements and the design of FaaSr (Vahid also contributed to the FaaSr-Docker repo which is used in building Docker images for users).