Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use global option for R package verbosity #3706

Merged
merged 5 commits into from
May 17, 2024

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented May 15, 2024

This fixes #3692 in just the way that @cgiachalis suggested.

Instead of the verbose option being printed with a default of FALSE (e.g. verbose = FALSE), it is now printed as verbose = getOption("mlpack.verbose", FALSE). That is, it defaults to false unless mlpack.verbose has been set (at least to the best of my limited R understanding).

I also added tests to make sure that it operates correctly.

@eddelbuettel @coatless let me know what you think here, a double-check would be much appreciated. 馃憤

@eddelbuettel
Copy link
Contributor

Using verbose = getOption("mlpack.verbose", FALSE) is indeed a good pattern and somewhat idiomatic ... but I do not see it in the diff for this PR. Did I miss it? It's been a long day...

@coatless
Copy link
Contributor

@eddelbuettel yes; it's here:

https://github.com/mlpack/mlpack/pull/3706/files#diff-0fd950ea62a1bd092169e153e05f92bedfe37985aebfef1f2512cc4493229d8eR34-R43

@rcurtin modified how function signatures are created. So, now when the bindings detect verbose it prints:

knn <- function(.,.,..., verbose = getOption("mlpack.verbose", FALSE) )

instead of:

knn <- function(.,.,..., verbose = FALSE)

@rcurtin
Copy link
Member Author

rcurtin commented May 15, 2024

I realized I also need to update the Markdown documentation; in essence I changed the code so that the table of parameters that gets listed for each binding lists the default of the verbose option as its new value, instead of FALSE.
e.g. this table: https://www.mlpack.org/doc/stable/r_documentation.html#input-options-19

Now, the line of Markdown in that table will instead be:

| `verbose` | [`logical`](#doc_r_logical) | Display informational messages and the full list of parameters and timers at the end of execution. | `getOption("mlpack.verbose", FALSE)` |

@rcurtin
Copy link
Member Author

rcurtin commented May 15, 2024

Using verbose = getOption("mlpack.verbose", FALSE) is indeed a good pattern and somewhat idiomatic ... but I do not see it in the diff for this PR. Did I miss it? It's been a long day...

Sorry about that, I should have been more clear in the description. Here is an example of the newly generated binding:

kfn <- function(algorithm=NA,                                                                                                                                                              
                epsilon=NA,                                                                                                                                                                
                input_model=NA,                                                                                                                                                            
                k=NA,                                                                                                                                                                      
                leaf_size=NA,                                                                                                                                                              
                percentage=NA,                                                                                                                                                             
                query=NA,                                                                                                                                                                  
                random_basis=FALSE,                                                                                                                                                        
                reference=NA,                                                                                                                                                              
                seed=NA,                                                                                                                                                                   
                tree_type=NA,                                                                                                                                                              
                true_distances=NA,                                                                                                                                                         
                true_neighbors=NA,                                                                                                                                                         
                verbose=getOption("mlpack.verbose", FALSE)) {   

I also noticed that the documentation for the parameter verbose in the generated .R files was wrong. So now after the commit I just pushed, verbose is documented as:

#' @param verbose Display informational messages and the full list of                                                                                                                      
#'   parameters and timers at the end of execution.  Default value                                                                                                                         
#'   "getOption("mlpack.verbose", FALSE)" (logical).   

I think I didn't forget any other place documentation should be?

@coatless
Copy link
Contributor

@rcurtin this looks great. I'm going to wait until the R build passes R CMD check/the testthat suite returns an all clear 馃槃

@rcurtin
Copy link
Member Author

rcurtin commented May 15, 2024

Sounds great, fingers crossed 馃槃

Copy link
Contributor

@coatless coatless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Though, need to work on dropping the compile time of the package.

@rcurtin
Copy link
Member Author

rcurtin commented May 16, 2024

The test failure looks like an unrelated unlucky failure; I don't think the changes here caused it. Someday I will figure out how to isolate that test, but I still have yet to manage to reproduce the issue locally.

@rcurtin
Copy link
Member Author

rcurtin commented May 16, 2024

Though, need to work on dropping the compile time of the package.

Totally agree. It is getting painful (even more so than it was before, which was already a considerable amount of pain!) and needs some investigation.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 馃憤

@eddelbuettel
Copy link
Contributor

@rcurtin Just thinking out loud here: maybe ccache if we can persist the cache layer? (I know GHA has some caching logic, but I never used it. There is also sccache which someone at work looking into / uses (?) but I have no simple example to point at.)

What else were you thinking of?

@rcurtin
Copy link
Member Author

rcurtin commented May 17, 2024

ccache would probably be helpful, but I think that there is some lower-level improvements that could be made, like compilation flag improvements, or maybe some CMake improvements, I'm not sure. I need to put a few hours into looking into it...

@rcurtin rcurtin merged commit 4985f4d into mlpack:master May 17, 2024
18 of 20 checks passed
@rcurtin rcurtin deleted the r-global-verbose branch May 17, 2024 15:39
This was referenced May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R] - Global option for 'verbose' argument
3 participants