-
Notifications
You must be signed in to change notification settings - Fork 2
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
package dependencies and interdependencies #11
Comments
Thanks! That all makes sense. Building the two side by side I hadn't encountered this directly. It has also all been working in the automated checks on github which should be installing both packages fresh, but since the co-dependence adds roadblocks to installation it makes sense to simplify. I'm not certain the BirdFlowModels package will pass rcmd check if it doesn't import or depend on the BirdFlowR which defines the class of its object; but as you say there's no code there and with lazy loading I'm not sure that running RCMD check will ever even require the object to be in memory. I guess I'll find out quickly if it doesn't pass the check. |
Sounds good. My current understanding is that the .rda file contains all the necessary class information inside it natively for it to be loaded as an object into any R session, and that the BirdFlowR functions won't actually be needed until a method is called on the |
Just tried it apparently (1) RCMD check does test load the object, (2) if its an S3 class you don't need to have the package that exports the class loaded, and (3) for S4 classes you do need to import or depend on the package that defines the class. So dropping the dependency on BirdFlowR initially broke BirdFlowModels, until I added Matrix to the Imports fields and imported the Matrix classes from it because BirdFlowR imports Matrix and uses it to store the sparse marginal matrices. In any case BirdFlowModels is passing the check now without importing or depending on BirdFlowR. |
A related question, I've been debating moving terra from imports to depends. The difference would be terra functions would be automatically attached to the search path when BirdFlowR is loaded; generally depends is frowned on as it can produce name conflicts and because it's the older way of doing things. That said if you expect your packages users to be using the functions in the other package than depends is recommended. Currently, if users don't load terra explicitly it's possible to produce a SpatRaster object with BirdFlow and then get an error when you try to plot it:
|
Nice (re: getting the packages to install separately and pass the checks)! For imports vs. depends, I am a fan of using imports, and then using terra::plot(a) to disambiguate in this case. Having examples of that and a note in the documentation could clue people in, or, alternatively, BirdFlowR could export a small wrapper function to accomplish this. Not a fan of putting terra into Depends, because then a much larger number of functions from terra will be in the namespace causing other potential issues. |
With the new commit I'm still experiencing this error during vignette build. I'm not entirely sure what's going on, but when I put rnaturalearthdata into Imports for BirdFlowR, it resolved this issue. Maybe that's how to fix it or maybe there's another way?
|
rnaturalearth is weird. It suggests rnaturalearthdata but doesn't depend or import it. Then the functions that need rnaturalearthdata run a check to see if it's installed and attempt to install it if it isn't installed. I think that's the install that's failing for you in docker. The writing r extensions manual states that packages needed for examples and vignettes should be under suggest not imports to make lean installs possible. I suspect that CRAN wouldn't let rnaturalearth include their own large data package in imports and that's why they have it in suggests. All this makes me hesitant to put it in BirdFlowR's imports field. Tomorrow I'll add code to install the rnaturalearth data package to the beginning of the vignette and see if that helps. |
…. Data package is now installed before RBirdFlow in readme and vignette.
Even with the new commit I'm still getting this error during the vignette build:
|
Last night I did some testing and devtools::install_cran() was failing to install rnaturalearthdata with a warning: "package ‘naturalearthdata’ is not available for this version of R"; but utils::install.packages() worked fine. This morning both functions can install it. Both functions are using getOptions("repos") to determine which CRAN mirror to use, set to "https://cran.rstudio.com/" on my system, so it's not a question of different mirrors. Last night my theory was that the devtools function had an excessively stringent requirement about what version of R the package was built under and thus it wasn't considering the binary as appropriate. The rnaturalearth package on cran has never been updated - this is by design - the intent is that the data should never change so shouldn't need to be re-downloaded often, it is probably also not rebuilt often but I'm not certain on that. Both last night and this morning available.packages(ignore_repo_cache = TRUE) returned the same version of rnaturalearthdata and the MD5sum hasn't changed, so I don't think the change in behavior I'm seeing is from an updated package build on the repository. I'm baffled. Possibly this large package intermittently fails to install? The CRAN check on github hasn't had problems though and the behavior last night was consistent over multiple trials. I am going to switch to the standard install.packages() in the Vignette as maybe it's more robust. |
Weird! |
Hmmm, looks like
|
I was curious how this is working on github actions for BirdFlowR Our action calls this action to install the packages: Which uses pak (not devtools!) to install packages. Maybe try this in docker:
That should install both the formal dependencies (depends, imports) and the packages that are under suggests. The pak manual doesn't include the word vignette anywhere and a test shows that it doesn't build vignettes so if we really want a vignette we could follow up with a call to devtools::install_github("birdflow-science/BirdFlowR" or use pak for rnaturalearthdata and then install BirdFlowR with devtools. Both of these options feel like kludges. Once I've setup pkgdown the vignettes will be rendered on the website and it may matter less that they aren't installed locally. |
Or maybe the install instructions if you want to build vignettes should be:
|
Agreed that it might be easier at this point just to provide separate instructions for building the vignette rather than continue troubleshooting rnaturalearthdata installation. The below worked well for me to build the packages and vignette from a clean (We could consider using remotes:: instead of devtools:: in the instructions since it will sometimes require end users to install/compile fewer things) https://devtools.r-lib.org/#conscious-uncoupling
|
Docker file added with: 3885c69 (instructions) |
I ran into some continued installation issues, which I think I've figured out how to resolve. A few observations:
Overall, these things make the installation rather hard to control/troubleshoot.
A proposed solution:
DESCRIPTION
, movernaturalearthdata
fromSuggests
toImports
DESCRIPTION
, removeBirdFlowR
fromDepends
and remove the entireRemotes
sectionNAMESPACE
, removeimport(BirdFlowR)
When I made these changes, everything installed clean for me in a fresh Docker container. Also happy to submit any of this or upcoming suggestions as a PR(s) if that's easier.
The text was updated successfully, but these errors were encountered: