Check that version is not the same

[juliasilge]

Closes #601

This PR checks whether the new and old versions are the same, as can happen when writing pins very fast. What do we think of this error message?

library(pins)
b <- board_temp(version = TRUE)
pin_write(b, iris)
#> Using `name = 'iris'`
#> Guessing `type = 'rds'`
#> Creating new version '20230302T194159Z-8dff5'
#> Writing to pin 'iris'
pin_write(b, iris)
#> Using `name = 'iris'`
#> Guessing `type = 'rds'`
#> Error in `version_setup()` at pins-r/R/board_folder.R:76:2:
#> ! The new version 20230302T194159Z-8dff5 is the same as the most recent
#>   version.
#> ℹ Did you try to create a new version with the same timestamp as the last
#>   version?

#> Backtrace:
#>     ▆
#>  1. └─pins::pin_write(b, iris)
#>  2.   ├─pins::pin_store(...) at pins-r/R/pin-read-write.R:114:2
#>  3.   └─pins:::pin_store.pins_board_folder(...) at pins-r/R/pin-store-fetch.R:25:2
#>  4.     └─pins:::version_setup(board, name, version_name(metadata), versioned = versioned) at pins-r/R/board_folder.R:76:2
#>  5.       └─cli::cli_abort(...) at pins-r/R/pin_versions.R:173:6
#>  6.         └─rlang::abort(...)

^{Created on 2023-03-02 with reprex v2.0.2}

Refined this message in commit c5b0baa

Probably could wait for the testthat release for this one as well.

[juliasilge]

Oh, this needs the changes from #724 because otherwise the vignettes are checked with all the same version. I may not need those Sys.sleep() additions.

[hadley]

I wonder if it might be better to tackle this by thinking more about the data hash — maybe here it's fine to just return the existing version since we know it's the same?

[juliasilge]

I definitely agree that how we make a version/hash could use some rework. I believe, though, if we do make some improvement in how we generate a data hash, we would still end up with this same problem. If you try to store a pin but the data hash hasn't changed and the timestamp hasn't changed, then we try to write to the same file in the local cache and end up with those "[EACCES] Failed to copy" errors. I don't believe making improvements to the data hashing strategy will solve this.

[hadley]

If it's the same data, and less than a second has elapsed, why not just return a path to the existing pin?

[juliasilge]

Let me know if I am not understanding what you are getting at, but that's basically the current situation that is leading to confusing errors; version_setup() returns a version/path to an existing pin but since the file is read only, it can't be overwritten and we get the "[EACCES] Failed to copy" errors with the long paths in them which folks find hard to interpret. Those errors are cropping up when version_setup() is called from the pin_store method (this really just applies to board_folder() since anything with a remote server can't respond that fast).

[hadley]

Right, so why try to overwrite that file?

[juliasilge]

What do you think about this alternative instead @hadley?

Step 1: Here in version_setup() we return NULL when the new version is the same as the old version.

[juliasilge]

Step 2: Then here in pin_store when the version is NULL, we skip all the writing (do not store) and just return the name.

[juliasilge]

Unfortunately this would mean that if you do this really fast:

b <- board_temp()
b %>% pin_write(head(mtcars))
b %>% pin_write(head(mtcars), tags = c("tag1", "tag2")

then that second write would not happen and you would silently not update the metadata.

I do think the error message is the way to go; maybe the way to think about it is that we are replacing an inscrutable error with a helpful error?

[hadley]

Ah, got it. That sounds like a sign that the hash should include the metadata, but that's out of scope for this PR.

So yes, that error is an improvement and you should merge this PR 😄

[github-actions]

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.