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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install R packages using Emscripten's filesystem mounting API #307

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

georgestagg
Copy link
Member

@georgestagg georgestagg commented Oct 25, 2023

With this set of changes, when installing an R package webR will first check for the existence of package_version.data files in the CRAN-like repo. These files should be Emscripten filesystem images. If they exist, they are downloaded and mounted using the Emscripten filesystem API. Otherwise, there is a fallback to the previous method of downloading and extracting .tgz files.

When mounting, the step where R package contents are decompressed and written to the virtual filesystem is not required. The Emscripten filesystem API instead creates nodes on the virtual filesystem that directly reference slices of the ArrayBuffer containing the downloaded data. This makes loading R packages quicker and more efficient (particularly when previously downloaded package files are cached).

The cost is a larger package since the package_version.data files are uncompressed. However, this is not too much of a worry since the files can be compressed over the wire as the user's browser and the static webserver negotiate a compression protocol using the Content-Encoding mechanism. If it is a real worry for certain applications, the .tgz fallback can be used to serve compressed packages.

Comparison on my machine when loading {tidyverse} and its dependencies:

Version Fresh Download Cached
v0.2.1 80 seconds 59.4 seconds
dev 19.6 seconds 2.9 seconds

@georgestagg georgestagg force-pushed the fs-mount-install branch 2 times, most recently from 60d8578 to 2752a9e Compare October 25, 2023 18:33
@georgestagg georgestagg marked this pull request as ready for review October 26, 2023 09:12
@georgestagg georgestagg requested a review from lionel- October 26, 2023 12:43
Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Impressive!

tryCatch({
install_vfs_image(repo, lib, pkg, pkg_ver)
}, error = function(cnd) {
if (!grepl("Unable to download", conditionMessage(cnd))) {
Copy link
Member

Choose a reason for hiding this comment

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

I see that we are generating this error, so matching on the message doesn't expose us to external changes to the message wording or to failures in case of translation (since it's not translated currently).

However it'd still be cleaner to throw a classed error that you could match here. The easiest way is to call a helper on the R side that is in charge of assembling the classed condition based on some failure state and throw it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's a good idea.

I'm going to hold off on implementing something like that for now, because in #308 the piece of code that raises the condition we're testing for moves from the webr package to the webR TypeScript worker anyway.

There, the error is thrown with JS's new Error() rather than using R's Rf_error() directly. This matches the other private functions in that module, so I don't think it makes much sense to set up an R custom condition helper just for that one function.

Instead, I think from JS we should be able to throw subclasses of Error, and webR should be able to capture that information and convert it into classed R error conditions, or attach the JS class information to the condition in some other way. That would be a bigger change, worthy of its own PR, but allow us to handle the general issue better in the future.

I'll make a new issue for tracking.

* If `true`, do not output downloading messages.
* Default: `false`.
*/
quiet?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

verbose so that the default is true?

Copy link
Member Author

@georgestagg georgestagg Oct 30, 2023

Choose a reason for hiding this comment

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

This was chosen to match the quiet argument of webr::install(), I'd like the JS options and R argument names to match for simplicity. The R argument itself was previously changed from verbose to quiet to match the quiet argument of R's install.packages().

@georgestagg georgestagg merged commit 2493c8f into main Oct 30, 2023
2 checks passed
@georgestagg georgestagg deleted the fs-mount-install branch October 30, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants