Skip to content

Fix verbose for R bindings #3691

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
coatless merged 3 commits into from
Apr 18, 2024
Merged

Fix verbose for R bindings #3691

coatless merged 3 commits into from
Apr 18, 2024

Conversation

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Apr 17, 2024

This fixes #3675.

I reproduced the issue and found that the verbose argument to mlpack's R bindings had no effect, even though the EnableVerbose() function was successfully being called.

This is because Log::Info (which is what the EnableVerbose() function modified) is a singleton object, and exists once in each translation unit. Since EnableVerbose() is compiled into the translation unit for the file r_util.cpp, the Log::Info modified by EnableVerbose() ends up being different than the Log::Info that is being used by each individual binding---so, in essence, we are enabling verbose mode... with the wrong Log::Info object.

I fixed this by removing the standalone EnableVerbose() and DisableVerbose() functions, and instead modifying the Log::Info object's verbose state directly inside each printed binding. So, e.g., this block is now gone from the R code:

  if (verbose) {
    EnableVerbose()
  } else {
    DisableVerbose()
  }

and the actual binding code, which before looked like this:

// [[Rcpp::export]]
void knn_call(SEXP params, SEXP timers)
{
  util::Params& p = *Rcpp::as<Rcpp::XPtr<util::Params>>(params);
  util::Timers& t = *Rcpp::as<Rcpp::XPtr<util::Timers>>(timers);

  if (p.Has("verbose"))
    Log::Info.ignoreInput = false;
  else
    Log::Info.ignoreInput = true;

  BINDING_FUNCTION(p, t);
}

now looks like this:

// [[Rcpp::export]]
void knn_call(SEXP params, SEXP timers)
{
  util::Params& p = *Rcpp::as<Rcpp::XPtr<util::Params>>(params);
  util::Timers& t = *Rcpp::as<Rcpp::XPtr<util::Timers>>(timers);

  if (p.Has("verbose"))
    Log::Info.ignoreInput = false;
  else
    Log::Info.ignoreInput = true;

  BINDING_FUNCTION(p, t);
}

and so when I run the example code provided by @cgiachalis:

set.seed(1234)
x <- matrix(rnorm(10*5), ncol = 5)

# KNN EXAMPLE
res <- mlpack::knn(query = x, reference = x, k = 3, verbose = TRUE)

I get this output instead of nothing:

> res <- mlpack::knn(query = x, reference = x, k = 3, verbose = TRUE)
[INFO ] Using reference data from 5x10 matrix.
[INFO ] Building reference tree...
[INFO ] Tree built.
[INFO ] Using query data from 5x10 matrix.
[INFO ] Searching for 3 neighbors with dual-tree kd-tree search...
[INFO ] 11 node combinations were scored.
[INFO ] 100 base cases were calculated.
[INFO ] 11 node combinations were scored.
[INFO ] 100 base cases were calculated.
[INFO ] Search complete.

Hooray!

@rcurtin rcurtin mentioned this pull request Apr 17, 2024
Closed
rcurtin
rcurtin commented Apr 17, 2024
View reviewed changes
src/mlpack/bindings/R/mlpack/src/rcpp_mlpack.h
#include <Rcpp.h>

// To suppress Found '__assert_fail', possibly from 'assert' (C).
#define BOOST_DISABLE_ASSERTS
Copy link
Member Author

@rcurtin rcurtin Apr 17, 2024

Choose a reason for hiding this comment

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

@coatless @eddelbuettel @Yashwants19 if you don't mind sanity checking me on this one since I am not an R expert... I think it's no longer needed since we aren't using Boost.

Copy link
Contributor

@eddelbuettel eddelbuettel Apr 17, 2024

Choose a reason for hiding this comment

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

Agreed, just remove.

Copy link
Contributor

@coatless coatless Apr 18, 2024

Choose a reason for hiding this comment

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

Yup, no longer needed with the shift away from boost!

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Apr 17, 2024

That's a fine catch of the singleton issue. I guess we could add a testthat predicate or two for silent vs verbose expectations? (Not a big user of testthat myself but these test runners are all roughly equivalent.)

@cgiachalis
Copy link
Contributor

cgiachalis commented Apr 17, 2024 •
edited
Loading

Thanks @rcurtin !

I just noticed that I forgot to mention a small suggestion in relation to verbose argument. It might be worth adding the ability to set the verbose state at the package level using verbose =getOption(mlpack.verbose, default = FALSE).

Practically, if a user wants to use verbose in all invocations then using options(mlpack.verbose=TRUE) will do the job.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Apr 17, 2024

Good point. And by using global state from R (as offered by R) you can also flip it on/off programmatically in subroutines.

@cgiachalis
Copy link
Contributor

cgiachalis commented Apr 17, 2024

Yeap!

eddelbuettel
eddelbuettel approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Approving now with most but not all CI runs green. The Azure stuff I will ignore 😎

coatless
coatless approved these changes Apr 18, 2024
View reviewed changes
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.

On the note of global options raised earlier, I would suggest adding that as a separate issue as it would involve modifications to how the function parameters are populated to get getOption() component, c.f.

mlpack/src/mlpack/bindings/R/print_R.cpp

Line 158 in 3e2a677

for (size_t i = 0; i < inputOptions.size(); ++i)

src/mlpack/bindings/R/mlpack/src/rcpp_mlpack.h
#include <Rcpp.h>

// To suppress Found '__assert_fail', possibly from 'assert' (C).
#define BOOST_DISABLE_ASSERTS
Copy link
Contributor

@coatless coatless Apr 18, 2024

Choose a reason for hiding this comment

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

Yup, no longer needed with the shift away from boost!

@coatless coatless merged commit e4ab99d into mlpack:master Apr 18, 2024
@cgiachalis cgiachalis mentioned this pull request Apr 18, 2024
Closed
@rcurtin rcurtin deleted the r-binding-verbose branch April 22, 2024 20:01
@rcurtin rcurtin mentioned this pull request Apr 22, 2024
Merged
This was referenced May 26, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eddelbuettel eddelbuettel eddelbuettel approved these changes

@coatless coatless coatless approved these changes

@Yashwants19 Yashwants19 Awaiting requested review from Yashwants19

Assignees

No one assigned

Labels

c: automatic bindings t: bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] - verbose argument has no effect

4 participants

@rcurtin @eddelbuettel @cgiachalis @coatless