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

Cache calls to units::valid_udunits_prefixes() #333

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

amoeba
Copy link
Collaborator

@amoeba amoeba commented Nov 4, 2021

This fixes a performance issue with the entire shiny_attributes app reported in #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.

This fixes a performance issue with the entire shiny_attributes app reported in ropensci#332.
@mbjones
Copy link
Member

mbjones commented Nov 4, 2021

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
Copy link
Collaborator Author

amoeba commented Nov 4, 2021

Agreed. @jeanetteclark offered to look into those #332 (comment) and we'll go from there.

@jeanetteclark
Copy link
Contributor

@amoeba I think I found the source of the failing tests, described here: boettiger-lab/taxadb-cache#1

Awaiting clarification from Carl.

@jeanetteclark
Copy link
Contributor

@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
Copy link
Collaborator Author

amoeba commented Nov 5, 2021

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.

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.

None yet

4 participants