search

ropensci / EML

Ecological Metadata Language interface for R: synthesis and integration of heterogenous data
https://docs.ropensci.org/EML
Other
97 stars 33 forks source link

Cache calls to units::valid_udunits_prefixes() #333

Closed amoeba closed 2 years ago

amoeba commented 2 years ago

This fixes a performance issue with the entire shiny_attributes app reported in ropensci#332.

Before this change, when launching the Shiny app and when editing attributes, a somewhat slow call from the units package, units::valid_udunits_prefixes() would get called. This meant slow startup time for the Shiny app and also clunky editing.

My solution was to cache the slow call on its first call and re-use the cached value. This was implemented with an Environment.

mbjones commented 2 years ago

Thanks @amoeba this looks great.

Looks like R CMD check is failing on R 4 on several platforms, so we should figure that out before we merge this, just to be sure everything is aok. But looks like a quick merge if the GHA tests pass.

amoeba commented 2 years ago

Agreed. @jeanetteclark offered to look into those https://github.com/ropensci/EML/issues/332#issuecomment-961186525 and we'll go from there.

jeanetteclark commented 2 years ago

@amoeba I think I found the source of the failing tests, described here: https://github.com/boettiger-lab/taxadb-cache/issues/1

Awaiting clarification from Carl.

jeanetteclark commented 2 years ago

@amoeba and @cboettig I confirmed that the new versions of contentid and taxadb pass locally where they didn't before - but haven't tested on all of the OS's that are failing. Perhaps we specify that the package depends on contentid 0.0.14 and taxadb 0.1.4 then merge. Once those versions are on CRAN presumably our tests will pass 🤞

amoeba commented 2 years ago

Thanks for the sleuthing @jeanetteclark, setting a minimum version on the taxadb dependency here sounds good.

I'd say this PR is good to merge, then.