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

{sf} is not Imported by {lutz} #9

Closed
adamhsparks opened this issue Nov 12, 2020 · 12 comments
Closed

{sf} is not Imported by {lutz} #9

adamhsparks opened this issue Nov 12, 2020 · 12 comments

Comments

@adamhsparks
Copy link

This can cause issues when {lutz} is imported by another package and uses functionality in {lutz} that depends on the {sf} package. As a result of {sf} only being in Suggests, tests fail and Notes appear when running checks on the dependent package, which caused me a bit of grief for a bit until I figured out what was going on and recalled a similar issue with another package that {bomrang} uses.

I've implemented a workaround in our package that Imports {lutz}.

# This function is never called.
# It only makes `sf` available in the package for `lutz` to use.
# `lutz` only has `sf` in Suggests, but it is needed for the function we use
# from `lutz`

stub <- function() {
   sf::st_as_sf()
}

Suboptimal, but works without any major side-effects and all tests pass and {sf} is sure to be installed if it's not on the machine that installs our package, etc.

Ideally, {sf} should be imported by {lutz} to avoid workarounds like this.

@ateucher
Copy link
Owner

Thanks @adamhsparks I'll do this when I next crack lutz open for maintenance.

@gavinsimpson
Copy link
Contributor

I just hit this too; would a PR help?

@ateucher
Copy link
Owner

ateucher commented Sep 20, 2023

Hi, thanks for the renewed attention on this! I don't think I will add sf to Imports as it is a very heavy, and optional, dependency. If you need that functionality in your package, adding sf to your package imports and including a "stub" function like @adamhsparks is the right way to do it: https://r-pkgs.org/dependencies-in-practice.html#how-to-not-use-a-package-in-imports.

@gavinsimpson
Copy link
Contributor

gavinsimpson commented Sep 21, 2023

No worries, but then I think you should use anything from Suggests selectively, checking by requireNamespace or similar if a package is available before continuing with code. and you're already doing the right thing with requireNamespace().

Something is getting messed up with dependencies not being installed or loading on GH Actions with the rlibs Actions and lutz, and adding a stub to force loading a package you're using isn't expected behaviour for something that is the functionality in the package name :) I've implemented the the stub fix and will see if that fixes the GH Actions issue and continue digging as there is certainly something wrong with dependency resolution with the Action.

@ateucher
Copy link
Owner

isn't expected behaviour for something that is the functionality in the package name

I get where you're coming from here, but sf is a heavy dependency which can be tricky to install, and only required if you're using the "accurate" method. The "fast" method is very good and will be enough for many users. Have you compared the two methods on the stations your package retrieves? If the "fast" method is sufficient, then you can also avoid imposing sf on your users.

@adamhsparks
Copy link
Author

For me it wasn't about which method was "good enough", it's an issue passing CRAN checks and GH actions, etc. since the package isn't properly loaded.

@gavinsimpson
Copy link
Contributor

Have you compared the two methods on the stations your package retrieves? If the "fast" method is sufficient, then you can also avoid imposing sf on your users.

I considered it, but then I saw #12 and had second thoughts so didn't pursue that option. Ideally I'd have this {sf} dependence on {canadaHCD} in Suggests (as I thought plotting a station map or finding stations using a map would be a nice addition but not necessary, and I only need {sf} currently whenever someone updates the station list in the pkg) so I'll look into that. But as @adamhsparks says something is breaking with dependencies and the setup-r-dependencies Action that I haven't gotten to the bottom of. Will try to pull a MWE package to reproduce the issue and follow up with the Action authors.

I'll also look at whether the fast option works for my specific application.

Thanks for the suggestion @ateucher

@ateucher
Copy link
Owner

I've created a minimal package that depends on {lutz} and the use of the "accurate" method here, with the approach of ignore_unused_imports() + including {sf} in Imports here. The GHA seem to be working, but it's possible I've missed a nuance of what you are trying to do. Let me know!

@ateucher ateucher reopened this Sep 22, 2023
@gavinsimpson
Copy link
Contributor

Yeah, that works (and it's what I have working in {canadaHCD}. But what @adamhsparks and I are talking about is the situation where you don't include {sf} as any kind of dependency of this new skeleton package. I.e. remove {sf} from DESCRIPTION and then the GH actions will fail running tests involving {lutz} functions with errors saying that {sf} in not available.

@ateucher
Copy link
Owner

If you're using a lutz function that requires the use of sf, and not including sf in Imports, then yes, that's expected, as putting sf in Imports in your package is the only way to ensure your user (or GHA) has it installed - since lutz does not do that.

If you're using a lutz function that doesn't require sf then you shouldn't need sf in Imports (or the stub function).

@ateucher
Copy link
Owner

Regarding #12, I will still support some form of fast method bundled with the package (ie not requiring sf), I just need to choose one of the many V8/Rust/C implementations to use. It will still likely be less accurate due to size constraints if the underlying data.

@ateucher
Copy link
Owner

I think I can close this now - if the solutions discussed above aren't working, please let me know and we can reopen it.

@ateucher ateucher closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2023
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

No branches or pull requests

3 participants