feat: add configure arguments to enable features or profiles during installation #475
Conversation
|
fix #474 |
|
In addition, this PR disables the use of |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
|
Thank you so much for the PR! I've not had a chance to give this a full review but my initial reaction is that I am highly skeptical of adding command line args to the tools/config.R file which gets called during the configure step. Moreover these args are hard coded to positions 1 and 2 to handle profile and features. Do you have a reproducible example of what this problem solves and why you would want to specify different features arguments in the build step of the R package? Are you going to create multiple different binaries of the same R package with different functionalities? |
|
I agree, looks a bit strange. Also test fix is a bit unclean: you should not modify production code to satisfy tests rather you should modify tests to run correctly. |
|
Thanks for the comment. I need the If configuration arguments are not accpeted, at the very least there should be a For example, the following function should only be available when the bench feature is enabled (This is particularly important in cases where we want to run benchmarks from the R side by passing R objects into Rust functions): #[extendr]
#[cfg(feature = "bench")]
fn pprof_kractor_koutput(
pprof_file: &str,
) -> std::result::Result<(), String> {
let guard = pprof::ProfilerGuardBuilder::default()
.frequency(2000)
.build()
.map_err(|e| format!("cannot create profile guard {:?}", e))?;
// ...; actual code
if let Ok(report) = guard.report().build() {
let file = std::fs::File::create(pprof_file).map_err(|e| {
format!("Failed to create file {}: {}", pprof_file, e)
})?;
let mut options = pprof::flamegraph::Options::default();
options.image_width = Some(2500);
report
.flamegraph_with_options(file, &mut options)
.map_err(|e| {
format!("Failed to write flamegraph to {}: {}", pprof_file, e)
})?;
};
out
}Regarding the test issue: it cannot be fixed within the unit test file. The reason is that |
|
I want to clarify that this does not depend on the first or second argument, but specifically on the |
|
I think @Ilia-Kosenkov's point is one that I agree with strongly. The R package should be the same whenever built. Typically benchmarks are called directly from the functions that they're testing. Otherwise the benchmarks are not actually comparable. Am I reading your code correctly that your key need for this is for a flamegraph? This would be for profiling rather than benchmarks or testing. An alternative approach to this would be to use use something like the below which is some pseudo-code that would use a oncelock and an env var to perform the profiling if a given environment variable is provided let guard = OnceLock::new();
let prof = std::env::var("MIRE_PROF")
if let Some(prof_path) = prof {
guard.get_or_init(|| pprof::ProfilerGuardBuilder::default()
.frequency(2000)
.build()
.with_context(|| format!("cannot create profile guard"))
.map_err(|e| format!("{:?}", e))?);
}
let res = (); // snip some code sets the result
if let Some(_) = prof {
if let Ok(report) = guard.report().build() {
let file = std::fs::File::create(pprof_file)...// flame graph stuff here
}
}
resI think perhaps a more flexible solution may be to have an savvy provides a savvy-cli which allows you to write rust tests https://yutannihilation.github.io/savvy/guide/test.html. But I think (and this is my perspective not everyone else's) many of us would suggest testing / benching in R rather than rust so that you have apples to apples comparisons. But to my knowledge you can't necessarily inject command line args as needed. |
|
Yes, it’s something like this: https://github.com/WangLabCSU/mire/blob/566070a04b4776049e03e79e1b6d516a98b0c38f/src/rust/src/kractor/mod.rs#L9. Thanks for the recommendations. Yes, I want to profile the rust code, and initially I wanted to do it from R, but I couldn’t find a way to profile the Rust code directly through R. As a workaround, I need to add a It would be even better if I remember |
|
|
|
|
3 participants
This PR introduces two new configure arguments,
--with-featuresand--with-profile, which allow enabling features or profiles during installation.It also respects the environment variables
{pkg_name}_FEATURESand{pkg_name}_PROFILE.