Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R #76

Seyi007 · 2024-10-08T06:28:28Z

Implement error handling for mapOption2Process, get_proc_medians, write_proc_medians_table, get_proc_weights, advanced_opts2est_walltime, assign_job_queue, and plot_estimated_walltimes .
Validate input arguments for each function to ensure they meet expected criteria.
Use tryCatch blocks to handle errors and warnings gracefully.
Provide informative error messages and detailed logging where appropriate.
Ensure functions fail gracefully and provide useful feedback.
Feature (adds or updates new capabilities)
Bug fix (fixes an issue).
Enhancement (adds functionality).
Breaking change (these changes would cause existing functionality to not work as expected).

Checklist

I have read and followed the CONTRIBUTING.md guidelines.
I have searched for existing content to ensure this is not a duplicate.
I have performed a self-review of these additions (including spelling, grammar, and related).
I have added comments to my code to help provide understanding.
I have added a test which covers the code changes found within this PR.
I have deleted all non-relevant text in this pull request template.
Reviewer assignment: @jananiravi @the-mayer

- let R-CMD sort NAMESPACE

the-mayer

Thanks for the extra work here @Seyi007! Providing error handling for these functions will definitely be a worthwhile endeavor. I've rebased this PR on the current main branch to pull in some function name changes and .Rd files that have been submitted in other PRs.

I'd like to see a few modifications before I review further:

`tryCatch()`

In R, the finally portion is always evaluated, even if the function generates an error or warning.

Printing “{function} execution completed.” might introduce confusion among our users, as they may think the function executed successfully even if an error occurred. Additionally, it could be overly verbose. I’d prefer to reserve the finally portion of each tryCatch() for cleanup or custom handler functions rather than messaging.

`print()` VS `message()`

This is both a stylistic and functional request. Given the choice between message() and print() I'd prefer that we use message(). Essentially, when using message, users may choose to suppress the additional messaging with suppressWarnings() or suppressMessages() -- print() is harder to escape and I'd rather give the user the choice.

Best option

Use rlang::inform(), rlang::warn(), and rlang::abort() where appropriate:

https://rlang.r-lib.org/reference/abort.html

Next Steps

Please consider the suggested modifications and submit the PR again for further review. Your efforts are greatly appreciated, and I’m looking forward to seeing the improvements!

jananiravi · 2024-10-10T16:51:49Z

Thanks for all your work on this @Seyi007. excellent points, @the-mayer! Looking forward to the changes!

Seyi007 · 2024-10-11T14:26:53Z

Duly noted @the-mayer , I'll get to work on this!

- Replaced base R error handling with rlang functions: `abort()`, `warn()`, and `inform()`. - Improved clarity and consistency in error and warning messages. - Enhanced robustness with detailed context for errors and warnings.

Seyi007 · 2024-10-14T12:17:49Z

Hi @the-mayer @jananiravi , I've implemented the suggestions made earlier and have updated the error handling in the R/acc2lin.R. Kindly review and let me know if I'm missing anything, while I work on the remaining functions.

…file - Replaced base R error handling with rlang functions: `abort()`, `warn()`, and `inform()`. - Improved clarity and consistency in error and warning messages. - Enhanced robustness with detailed context for errors and warnings.

falquaddoomi

Nice work! Generally, I think having better error reporting will really help things, so thank you for the contribution.

I'm inexperienced at R, so take my comments with a grain of salt. Anyway, I made some suggestions in keeping with what @the-mayer had recommended in a previous comment.

falquaddoomi · 2024-10-16T17:14:03Z

R/blastWrappers.R

+    stop("The DELTABLAST executable path is invalid: ", deltablast_path)
+  }
+  if (!dir.exists(db_search_path)) {
+    stop("The database search path is invalid: ", db_search_path)
+  }
+  if (!file.exists(query)) {
+    stop("The query file path is invalid: ", query)
+  }
+  if (!is.numeric(as.numeric(evalue)) || as.numeric(evalue) <= 0) {
+    stop("The evalue must be a positive number: ", evalue)
+  }
+  if (!is.numeric(num_alignments) || num_alignments <= 0) {
+    stop("The number of alignments must be a 
+         positive integer: ", num_alignments)
+  }
+  if (!is.numeric(num_threads) || num_threads <= 0) {
+    stop("The number of threads must be a positive integer: ", num_threads)


IMHO it would be better to use rlang::abort() here rather than stop(), as you do in the rest of your code.

falquaddoomi · 2024-10-16T17:18:30Z

R/blastWrappers.R

+  }, error = function(e) {
+    message(paste("Error in run_deltablast: ", e))
+  }, warning = function(w) {
+    message(paste("Warning in run_deltablast: ", w))
+  }, finally = {
+    message("run_deltablast completed")
+  })


I'd consider switching out these message() calls for the rlang equivalents, e.g. rlang::error() in the error handler, and rlang::warn() in the warning.

As @the-mayer mentioned, I'd also remove the finally clause entirely, since it'll also be displayed with an error/warning output. Generally, finally should be used when you really need to clean something up, even if there's an error that would ordinarily terminate execution, not for regular reporting.

falquaddoomi · 2024-10-16T17:19:24Z

R/blastWrappers.R

+  if (!file.exists(rpsblast_path)) {
+    stop("The RPSBLAST executable path is invalid: ", rpsblast_path)
+  }
+  if (!dir.exists(db_search_path)) {
+    stop("The database search path is invalid: ", db_search_path)
+  }
+  if (!file.exists(query)) {
+    stop("The query file path is invalid: ", query)
+  }
+  if (!is.numeric(as.numeric(evalue)) || as.numeric(evalue) <= 0) {
+    stop("The evalue must be a positive number: ", evalue)
+  }
+  if (!is.numeric(num_threads) || num_threads <= 0) {
+    stop("The number of threads must be a positive integer: ", num_threads)
+  }


Same as above; rlang::abort() is a better option than stop().

falquaddoomi · 2024-10-16T17:20:19Z

R/blastWrappers.R

+  }, error = function(e) {
+    message(paste("Error in run_rpsblast: ", e))
+  }, warning = function(w) {
+    message(paste("Warning in run_rpsblast: ", w))
+  }, finally = {
+    message("run_rpsblast completed")
+  })


Same as above; the error handler should call rlang::error() rather than message(), warning should call rlang::warn(), and the finally handler should be removed.

Yeah, I haven't updated the R/blastWrappers.R file with the suggestions from @the-mayer . I've only updated the first 2 files, and I'm currently working on the said file.

falquaddoomi · 2024-10-16T17:26:20Z

R/assign_job_queue.R

@@ -1,3 +1,4 @@
+suppressPackageStartupMessages(library(rlang))


Ideally packages shouldn't invoke library(), since it'll import symbols into the user's namespace that they might not be expecting.

Instead, you should add the following line to the docstrings of functions that use rlang::abort(), rlang::warn(), etc. (with the elements after rlang being the symbols you're using from rlang in that specific function):

#' @importFrom rlang abort warn

Duly noted! I'll include it in my next commit.

Also, if you are importing multiple functions from that library, it is better to include it as a dependency in the imports list here.

falquaddoomi · 2024-10-16T17:59:41Z

R/msa.R

-    )
-    prot_aa
+  prot_aa <- readAAStringSet(
+    fa_file,


Nice catch! The path parameter is indeed named filepath, not path as it was in the original code.

It's fine to leave it as a positional parameter as you have it here, since it's the first parameter.

falquaddoomi · 2024-10-17T17:33:52Z

R/acc2lin.R

+      # capturing the call stack
+      call = sys.call(),


I think you can leave call at its default value, i.e. remove it from the list of arguments. FAICT the argument is intended to be used when you want the stack trace to appear as if it came from another method (e.g., not a helper method). I didn't see any difference in my testing, at least, with it unset versus setting it to sys.call().

Faisal, what's FAICT?

falquaddoomi · 2024-10-17T17:34:10Z

R/acc2lin.R

+      message = paste("Warning during IPG fetching
+                      or lineage processing:", w$message),
+      class = "lineage_processing_warning",
+      call = sys.call(), # capturing the call stack


Same as above; its default value seems to work fine, so you can remove call here.

falquaddoomi · 2024-10-17T17:34:49Z

R/acc2lin.R

+      abort(
+        message = paste("An error occurred: ", e$message),
+        class = "fetch_error",
+        call = sys.call(),
+        accnums = accnums,
+        out_path = out_path,
+        plan = plan
+      )
+    }, warning = function(w) {
+      warn(
+        message = paste("Warning: ", w$message),
+        class = "fetch_warning",
+        call = sys.call(),
+        accnums = accnums,
+        out_path = out_path,
+        plan = plan
+      )


Like the other invocations above, these both include call as well, but don't need to.

falquaddoomi · 2024-10-17T17:35:18Z

R/acc2lin.R

+    abort(
+      message = paste("An error occurred: ", e$message),
+      class = "processing_error",
+      call = sys.call(),
+      accessions = accessions,
+      ipg_file = ipg_file,
+      assembly_path = assembly_path,
+      lineagelookup_path = lineagelookup_path
+    )
+  }, warning = function(w) {
+    warn(
+      message = paste("Warning: ", w$message),
+      class = "processing_warning",
+      call = sys.call(),
+      accessions = accessions,
+      ipg_file = ipg_file,
+      assembly_path = assembly_path,
+      lineagelookup_path = lineagelookup_path
+    )


Same as above; you don't need to specify call here.

Duly Noted!

- Added robust error handling in run_deltablast and run_rpsblast functions. - Updated Roxygen documentation to import rlang::abort, rlang::warn and rlang::inform for better error management. - Refactored code for clarity and consistency based on the suggestion from the last review.

Seyi007 · 2024-10-20T11:11:09Z

Hi @jananiravi @falquaddoomi, I've implemented all the changes required from the previous review in my latest commit. Could you review this and let me know if I'm missing anything?

falquaddoomi

Nice, looks like you got to everything I raised.

jananiravi · 2024-10-22T22:22:54Z

@falquaddoomi @the-mayer followed the conversation but not the most recent line-by-line edits. Feel free to merge if this looks good.

Thanks, @Seyi007 !

Seyi007 · 2024-10-23T05:07:07Z

Yay! Thank you @falquaddoomi @jananiravi

the-mayer · 2024-10-25T15:14:41Z

Thanks for the additional work here, @Seyi007!

Finishing up with #97 and will tackle merging this one next.

the-mayer

Thanks @Seyi007, sending to main!

Seyi007 requested a review from the-mayer October 8, 2024 07:45

Seyi007 changed the title ~~Add error handling to multiple functions~~ Add error handling to multiple functions in R/assign_job_queue.R Oct 8, 2024

Seyi007 changed the title ~~Add error handling to multiple functions in R/assign_job_queue.R~~ Added error handling to multiple functions in R/assign_job_queue.R Oct 8, 2024

Seyi007 mentioned this pull request Oct 9, 2024

Rename the listed function names in the following files. #44

Closed

Seyi007 changed the title ~~Added error handling to multiple functions in R/assign_job_queue.R~~ Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R Oct 9, 2024

Seyi007 mentioned this pull request Oct 9, 2024

[FEAT] Add error handling for functions #30

Closed

3 tasks

the-mayer linked an issue Oct 9, 2024 that may be closed by this pull request

Rename the listed function names in the following files. #44

Closed

the-mayer self-assigned this Oct 9, 2024

the-mayer added the enhancement New feature or request label Oct 9, 2024

Seyi007 mentioned this pull request Oct 10, 2024

Function names rename for R/combine_analysis.R, R/combine_files.R, R/create_lineage_lookup.R, R/assign_job_queue.R #95

Merged

11 tasks

the-mayer added 2 commits October 10, 2024 08:47

Merge commit 'a9e5d9e67b2e866c253793e802c7e23fa0d6123d'

a270156

- remove old .Rd leftovers and update with new docs

823af96

- let R-CMD sort NAMESPACE

the-mayer requested changes Oct 10, 2024

View reviewed changes

falquaddoomi self-requested a review October 16, 2024 15:08

falquaddoomi reviewed Oct 17, 2024

View reviewed changes

jananiravi added the outreachy for outreachy interns label Oct 20, 2024

falquaddoomi approved these changes Oct 22, 2024

View reviewed changes

the-mayer added 3 commits October 29, 2024 20:33

Merge commit 'bae52b06340c9518a1ca0417a6b52840ef1c4017'

c1ee75c

fix error introduced by merge

f4b50f4

fix .Rd

dd86b3c

the-mayer approved these changes Oct 30, 2024

View reviewed changes

the-mayer merged commit 2ef3f56 into JRaviLab:main Oct 30, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R #76

Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R #76

Seyi007 commented Oct 8, 2024 •

edited by jananiravi

Loading

the-mayer left a comment

jananiravi commented Oct 10, 2024

Seyi007 commented Oct 11, 2024

Seyi007 commented Oct 14, 2024

falquaddoomi left a comment

falquaddoomi Oct 16, 2024

falquaddoomi Oct 16, 2024

falquaddoomi Oct 16, 2024

falquaddoomi Oct 16, 2024

Seyi007 Oct 18, 2024

falquaddoomi Oct 16, 2024

Seyi007 Oct 18, 2024

jananiravi Oct 18, 2024

falquaddoomi Oct 16, 2024

falquaddoomi Oct 17, 2024

jananiravi Oct 18, 2024

falquaddoomi Oct 17, 2024

falquaddoomi Oct 17, 2024

falquaddoomi Oct 17, 2024

Seyi007 Oct 19, 2024

Seyi007 commented Oct 20, 2024

falquaddoomi left a comment

jananiravi commented Oct 22, 2024

Seyi007 commented Oct 23, 2024

the-mayer commented Oct 25, 2024

the-mayer left a comment

		@@ -1,3 +1,4 @@
		suppressPackageStartupMessages(library(rlang))

Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R #76

Added error handling to multiple functions in R/assign_job_queue.R, R/blastWrappers.R #76

Conversation

Seyi007 commented Oct 8, 2024 • edited by jananiravi Loading

Checklist

the-mayer left a comment

Choose a reason for hiding this comment

tryCatch()

print() VS message()

Best option

Next Steps

jananiravi commented Oct 10, 2024

Seyi007 commented Oct 11, 2024

Seyi007 commented Oct 14, 2024

falquaddoomi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seyi007 commented Oct 20, 2024

falquaddoomi left a comment

Choose a reason for hiding this comment

jananiravi commented Oct 22, 2024

Seyi007 commented Oct 23, 2024

the-mayer commented Oct 25, 2024

the-mayer left a comment

Choose a reason for hiding this comment

Seyi007 commented Oct 8, 2024 •

edited by jananiravi

Loading

`tryCatch()`

`print()` VS `message()`