{FIMS} logging system overhaul

[kellijohnson-NOAA]

The logging system in FIMS is used by some and not used by others. It tends to have a dual purpose right now, one for debugging a model and two for giving a user information. Unfortunately, in my 👀, because of this duality the system is not as good as it could be if it had one main goal.

@nathanvaughan-NOAA posted on Issue #1258 the following comment:

Taking a quick look at the logging again. We currently have a bunch of functions get_log(), get_log_errors(), get_log_module(string). Would it make more sense to collapse this down to just get_log() that returns the full log and get_log(string) where the string gets checked against a list of acceptable names ("errors", "warnings", "info", "population", "fleet", or any other module names) then it spits an error if the name isn't on the list or uses the appropriate get_errors(), get_warnings(), get_info(), or get_module(string) function call internally? This would also help with interpretation, as the module option currently uses the file name the log came from, which wouldn't be known to a user without the source code.

I see some straightforward tasks

• remove legacy log entries that are no longer needed
• Fix [Bug:] get_log_module() crashes when no arguments are passed #1258

Then, we have some decisions to make and once we decide on these, we can make some additional tasks

• update the logging vignette or get rid of it
• decide if we want to keep the log in the json output
• decide if we want to continue to export all of the log functions
• decide if the logging system is meant for developers or for users

Additional comments are welcome on anything related to the logging system but it would be particularly helpful if people could indicate what they would like to see in a logging system for FIMS and if people can link to examples of logging systems that they particularly enjoy that would be great to.

[Andrea-Havron-NOAA]

Now that we have FIMS code set up to safely fail without crashing R, I would either like to replace the logging system with warnings/errors that print to the R console or add these warnings/errors in addition to the logging system.

[kellijohnson-NOAA]

What about allowing users to set a global flag that specifies if they want them printed to the screen or if they want them saved to a file? The tidyverse design principles talks about this.

[Andrea-Havron-NOAA]

@nathanvaughan-NOAA and I were just discussing this! Yes, I think that would be a good way forward.

[msupernaw]

@kellijohnson-NOAA and @Andrea-Havron-NOAA If there's a handled exception the user can call print(get_log()) for the entire log, additional options are print(get_log_errors()), print(get_log_warnings(), and print(get_log_info()). Also there is an option to throw immediately on error. set_log_throw_on_error(TRUE)

[nathanvaughan-NOAA]

This all sounds good to me! @kellijohnson-NOAA a quick fix for the bug may just be to change get_log_module(string) to get_log(string). That way the module call will work if a string is input and it will default to the everything log if no string is added.

[Andrea-Havron-NOAA]

@kellijohnson-NOAA and @Andrea-Havron-NOAA If there's a handled exception the user can call print(get_log()) for the entire log, additional options are print(get_log_errors()), print(get_log_warnings(), and print(get_log_info()). Also there is an option to throw immediately on error. set_log_throw_on_error(TRUE)

I think for me it comes back to the user experience, which is in the R environment. Error messages printed directly to the R console is more user friendly than having to call additional functions to know how/why the model has failed. In my opinion, it is more intuitive for the default be set so that errors print to the screen and warnings collect in a file with a global flag allowing users to divert error messages to a file.

[kellijohnson-NOAA]

@nathanvaughan-NOAA for

quick fix for the bug may just be to change get_log_module(string) to get_log(string). That way the module call will work if a string is input and it will default to the everything log if no string is added.
do you mean to get rid of get_log_module() entirely or just not export it to R?

[nathanvaughan-NOAA]

I think we could get rid of it entirely. Based on my current branch the solution would be: 1) Rename get_log_module to get_log in rcpp_interface.hpp so there would be two get_log functions one that takes no input and one that takes a string with a module name (line 374). 2) remove the export of get_log_module in FIMS-package.R (line 23). 3) Rename get_log_module to get_log or maybe remove entirely in the RCPP module in fims_modules.hpp (lines 79:80). 4) Replace get_log_module with get_log in fims-demo.Rmd (line 263). Line numbers could be different between my branch and yours but gets you close probably :)

[msupernaw]

@nathanvaughan-NOAA Rcpp doesn't allow two functions with the same name but different signature. You would have to create a function "get_log" and use Rcpp::Nullable as the function argument.

[nathanvaughan-NOAA]

That makes sense why you didn't set it up that way in the first place then :)

[kellijohnson-NOAA]

Right now we have get_log(), get_log_warnings(), and get_log_errors()` but the latter are just subsets of the former and you can subset them in R using

log_data_frame <- jsonlite::fromJSON(get_log())
dplyr::filter(log_data_frame , label == "warning")

And the documentation for get_log_warnings() and get_log_errors() does not give a use case on where the R code above would not be sufficient. Should we remove get_log_warnings() and get_log_errors() or do we have direct needs for them?

[kellijohnson-NOAA]

It seems like to me that very few people use the fims logging system. It might be easiest to strip it out and rebuild it to suite our current needs. It might be best to create something that is strictly for developers and rely on R internally for users. Please respond here if you use the logging system as is and its removal would disrupt your workflow. @msupernaw @nathanvaughan-NOAA @Andrea-Havron-NOAA thoughts?

[nathanvaughan-NOAA]

The log seemed good at the start but I don't use it often now. So no strong opinion from me.

[msupernaw]

@kellijohnson-NOAA I second @nathanvaughan-NOAA on this one! If anything, I might suggest keeping for errors and maybe warnings, but I'm also fine with nuking it.

[e-perl-NOAA]

Andrea did want to add something about how she doesn't just want to get rid of it, she wants to replace it with something more informative. I just don't know what that looks like and we are busy with the DFO course, so maybe hold off on nuking it until she can weigh in on her vision for what the function needs to become to be better 😅