Skip to content

Conversation

@jibarozzo
Copy link
Contributor

@jibarozzo jibarozzo commented Dec 7, 2024

Addressing issue #5. Used the defaults from savvy::savvy_init() to create Rust scaffolding inside R package. Depending on what we decide with extendr or savvy we can merge this branch or create another with the default scaffolding from extendr. They provide similar structure and documentation.

@jibarozzo jibarozzo requested review from asbates and jdhoffa December 7, 2024 21:20
@jdhoffa
Copy link
Member

jdhoffa commented Dec 8, 2024

@jibarozzo i have not yet had the chance to dig into the differences between extendr and savvy

I will dig into that early next week, but in the mean time could you please summarize what factors lead you to decide on this approach?

Co-authored-by: Jackson Hoffart <[email protected]>
@jibarozzo
Copy link
Contributor Author

Initial differences

The scaffolding for both is about the same. The main difference I see is in error handling. Error handling with savvy uses Result instead of panic! . extendr doesn't seem to be progressing on that front, see here. I think having better error handling will benefit us in the long run.

Another main difference I see are the dependencies. extendr has many dependencies and relies on rxtendr and to provide helper functions to allow interfacing between R and Rust, meanwhile savvy (crate and R package are called the same) has barely any dependencies and the helper functions are readily available by installing the savvy R package, which installs the crate.

Caveats

  • savvy prefers explicitness over ergonomics
  • savvy provides limited amount of APIs and might not fit for complex usages

Some design ideas from the author (summarized here)

Final thoughts

The idea with extender is to expose Rust functions to R vs. savvy which geared toward working with R data structures inside Rust, rather than wrapping Rust code into an R-facing package. It depends on how we want it to handle the data.
The main decision to go with savvy was the simplicity and error handling. As I have dug deeper into the two frameworks I've learned that we would need to implement your own threading logic (e.g., using rayon in Rust), but integrating this back into R via savvy would be more cumbersome compared to using extendr. Given that the main objective is to re-implement functions using multi-threading savvy is not ideal. We could keep it if we are up to the challenge of a developing custom threading logic.

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

Looks like there is CI failure on R CMD check for ubuntu and windows (however only warnings) but not sure how much that matters.

Approving

@jibarozzo jibarozzo linked an issue Dec 9, 2024 that may be closed by this pull request
@jibarozzo
Copy link
Contributor Author

jibarozzo commented Dec 9, 2024

I checked the warning a little bit closer. For the windows-latest (oldrel4) the logs indicate that R 4.0.5 is being used for the windows-latest (oldrel4) and it could be related to a corrupt .drectve file. I'll dig into this a more. Since the ubuntu-latest (devel) version test is the other one failing, I suspect the source for this is a new roll out of Ubuntu 24.04 and something to do with the the fact that R and Cargo were: Removed from the Ubuntu 24.04 image due to maintenance reasons. It suggests:

  • Switch back to Ubuntu 22 by changing workflow YAML to use runs-on: ubuntu-22.04 We support two latest LTS Ubuntu versions, so Ubuntu 22 will still be maintained for the next 2 years.

For now, I'll merge and open an issue.

@jibarozzo jibarozzo merged commit 35a0612 into main Dec 9, 2024
9 of 11 checks passed
@jdhoffa jdhoffa deleted the savvy_scaffolding branch December 12, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Scaffold Rust

4 participants