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

Out of memory caching #25

Closed
wants to merge 30 commits into from
Closed

Out of memory caching #25

wants to merge 30 commits into from

Conversation

jimhester
Copy link
Member

No description provided.

BugReports: https://github.com/hadley/memoise/issues
Imports:
digest (>= 0.6.3)
digest (>= 0.6.3),
googleAuthR,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cache specific dependencies should probably go in Suggests: instead of Imports:

cache_reset <- function() {
query_results <- query_ds(the_body = list(gqlQuery = list(queryString = paste0("SELECT * FROM ", cache_name))))
while((query_results$batch$moreResults != "NO_MORE_RESULTS") | is.null(query_results$batch$entityResults) == FALSE) {
ids <- (query_results$batch$entityResults$entity$key$path %>% dplyr::bind_rows())$name
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to remove use of %>% and the dplyr dependency here.

I don't know exactly what data structure is being returned, but from the code this looks like you can use vapply() with an extractor function.

@jimhester
Copy link
Member Author

@danielecook Please look at the travis build results (https://travis-ci.org/hadley/memoise/builds/148021731#L1180-L1217) and fix all Warnings, most are due to documentation mismatches or unimported functions.

@danielecook
Copy link

@jimhester Hey thanks! I wasn't sure whether the authors would want to add this so I renamed the package and had started my own fork but I'll change it back and resolve these issues. I'll also add a file-based cache soon (for dropbox/Google Drive). I was in a bit of a rush to get started using this for some of my own projects (-;

@codecov-io
Copy link

codecov-io commented Jul 28, 2016

Current coverage is 36.11% (diff: 0.00%)

Merging #25 into master will decrease coverage by 62.81%

@@             master        #25   diff @@
==========================================
  Files             2          5     +3   
  Lines            93        252   +159   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits             92         91     -1   
- Misses            1        161   +160   
  Partials          0          0          

Powered by Codecov. Last update 4d8718b...baed71c

@danielecook
Copy link

Got this to work, but had to remove quite a bit of documentation. I tend not to be so stringent as to not allow things like:

cache_filesystem("~/Dropbox/.rcache")

@jimhester
Copy link
Member Author

They should be in #' @examples not #' @usage, and if you don't want them to run you need to put them in a dontrun block.

#' \dontrun{
#'  # ... code to not run
#' }

See (http://r-pkgs.had.co.nz/man.html#man-functions) for a good reference.

@jimhester
Copy link
Member Author

I ended up merging this with b3b9ae6.

I cleaned up some things, but decided to remove the google datastore backend, there was too much API specific code that I don't think belongs in this package. Also I was a little confused why you were using datastore for this, Google cloud store (with https://github.com/cloudyr/googleCloudStorageR) seems like a better fit.

Thank you again for the PR and sorry it took so long to get merged. I think this is a very nice improvement to the package!

@jimhester jimhester closed this Oct 7, 2016
@danielecook
Copy link

Hey just saw these options were folded in. Thanks for taking care of this! Google Cloud storage is a better fit but it's also probably slower. Regardless, glad to see these changes implemented. Have a good rest of the year!

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