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

Robust fallback mechanism #69

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Robust fallback mechanism #69

merged 3 commits into from
Jan 29, 2024

Conversation

multimeric
Copy link
Contributor

We had some issues running velociraptor::scvelo at my workplace

ImportError: /lib64/libz.so.1: version `ZLIB_1.2.9' not found (required by <USER DIRECTORY>/.cache/R/basilisk/1.14.0/velociraptor/1.12.0/env/lib/python3.8/site-packages/matplotlib/../../.././libpng16.so.16)

I think the issue is that the R process links to our built-in system zlib (/lib64/libz.so.1) which takes precedence over the newer version that is installed into the conda environment (and it does get installed, I checked). Then when reticulate runs Python inside the same process it also uses the old zlib version.

There might be a more elegant solution, but I found a neat workaround to this issue that follows advice in the basilisk manual. Basically we set the testload packages so basilisk can check if there's a linking error, and if there is an error then basilisk creates a fallback environment that contains its own version of R and runs the .run_scvelo function there instead. I namespace (::) every function call inside the basilisk function so that they will still work the fallback R interpreter which has no packages loaded by default. What's nice about this solution is that it changes absolutely nothing for users who didn't encounter this issue, but it fixes it for those who did. The only thing I don't like is having to use ::: to access internal functions inside the velociraptor package, since it won't be available by default in the fallback env, but I can't think of a better solution.

The modified version of this package has been checked by myself and another researcher and it seems to work fine.

@LTLA
Copy link
Collaborator

LTLA commented Dec 20, 2023

👍 exactly what the testload was meant to handle.

Re the ::: situation: currently unavoidable and unpleasant. This is actually a symptom of a more general problem, in that the fallback R environment is going to pull in packages from the current R environment, which is bound to get messy at some point. Hopefully it works well enough for now - if I had some time, I would write a bundler that takes the function to be executed in the fallback process, converts it to a standalone function with no dependencies other than base R (by recursing through all of the calls to other internal functions and copy/pasting their function bodies) and then sending that to the fallback, for a nice clean execution. That kind of Javascript-like tree-shaking should work well enough for pure R functions, i.e., with no C/C++/Fortran dependencies. Maybe a nice GSoC project if someone is interested, I dunno.

@multimeric
Copy link
Contributor Author

@kevinrue?

@kevinrue
Copy link
Owner

Sorry. Fell off my radar. Thanks for refreshing the thread!

@kevinrue
Copy link
Owner

Bringing main branch up to date, fixing github action, then merging this PR (hopefully today)

@kevinrue kevinrue merged commit c798bc5 into kevinrue:devel Jan 29, 2024
1 of 3 checks passed
@multimeric
Copy link
Contributor Author

Great, thanks for the merge!

@kevinrue
Copy link
Owner

No worries. Thanks for the effort and the patience!

PS: I haven't version bump'ed it yet so it won't be available on devel. Do you need it soon? I'm investigating the need for an update in the version of the dependencies while I'm at it today

@multimeric
Copy link
Contributor Author

No rush on the new release. Feel free to whenever is convenient.

@kevinrue
Copy link
Owner

Actually, I've version bump in #70 and pushed upstream. Better getting the version out there earlier, so that it's tried and tested as widely as possible

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.

3 participants