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

Is caching not working as expected? #2642

Open
IndrajeetPatil opened this issue Aug 3, 2024 · 6 comments
Open

Is caching not working as expected? #2642

IndrajeetPatil opened this issue Aug 3, 2024 · 6 comments
Labels
feature a feature request or enhancement

Comments

@IndrajeetPatil
Copy link
Collaborator

I was expecting that the second call here would be faster because it would use cache, but that's not what I see (at least locally).

library(lintr)

local_dir <- "/Users/indrajeetpatil/Documents/GitHub/lintr/R"

# the same behaviour is also seen with `lint_package()`
lint_dir(local_dir, cache = TRUE)
lint_dir(local_dir)                # I'd have expected this to be quick 

Either I am doing something wrong, or the caching is not working as expected.

@IndrajeetPatil
Copy link
Collaborator Author

> system.time(lintr::lint_dir("~/Documents/GitHub/lintr/R", cache = TRUE))
  |====================================================================================================================| 100%
   user  system elapsed 
 49.759   1.422  52.014 
> system.time(lintr::lint_dir("~/Documents/GitHub/lintr/R"))
  |====================================================================================================================| 100%
   user  system elapsed 
 58.436   1.401  61.078 

@AshesITR
Copy link
Collaborator

AshesITR commented Aug 4, 2024

Does it work when specifying cache = TRUE both times?

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Aug 4, 2024

This is WAI IMO, cache=FALSE should never try and read an existing cache (which may have been invalidated):

character()

Possibly, we could consider allowing cache=NULL to mean "use cache if found at default path", and further consider making that the default, but the current behavior for FALSE is correct.

@IndrajeetPatil
Copy link
Collaborator Author

Okay, I clearly have an incorrect mental model of {lintr}’s caching algorithm. 

As a user, here is how I thought it worked:

  • step-1: run linter with cache = FALSE and realize it takes a while
  • step-2: run linter with cache = TRUE and assume that unless the source code files, {lintr} package version, base-R version, etc. change, or I run cache_clear(), {lintr} is going to use this cache
  • step-3: rerun linter without specifying the cache argument and expect it to run faster because it’s using previously generated cache 

Admittedly, this UX expectation comes from how caching works in {styler}, and I find it to be much more intuitive than what’s going on in {lintr}.

But I am happy to be challenged on that point :) 

@AshesITR
Copy link
Collaborator

AshesITR commented Aug 5, 2024

I'm happy with revisiting the cache UX.

@MichaelChirico
Copy link
Collaborator

Noting: if cache = 'my_dir', I don't know that we have any way to recover that in the nest invocation. You'll have to keep calling with cache='my_dir'.

Anyone, +1 to revisiting. We don't have a cache_clear() function now, is important context for my reasoning. So if we add one, that changes things.

@IndrajeetPatil IndrajeetPatil added the feature a feature request or enhancement label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants