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

Check that version is not the same #727

Merged
merged 18 commits into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

* Updated error messages and type checking (#731) along with testing strategy (#724).

* Added new check for whether a new version is the same as the previous version,
as can happen when writing pin versions very quickly (#727).

# pins 1.1.0

## Breaking changes
Expand Down
5 changes: 3 additions & 2 deletions R/pin-meta.R
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#' b %>% pin_download("mtcars")
#'
#' # Use tags instead
#' b %>% pin_write(head(mtcars), "mtcars", tags = c("fuel-efficiency", "automotive"))
#' b %>% pin_write(tail(mtcars), "mtcars", tags = c("fuel-efficiency", "automotive"))
#' b %>% pin_meta("mtcars")
#'
pin_meta <- function(board, name, version = NULL, ...) {
Expand Down Expand Up @@ -105,7 +105,8 @@ test_api_meta <- function(board) {
testthat::test_that("can update pin metadata", {
# RSC requires at least 3 characters
name <- local_pin(board, 1, title = "xxx-a1", description = "xxx-a2")
pin_write(board, 1, name, title = "xxx-b1", description = "xxx-b2")
# change content so hash changes
pin_write(board, 2, name, title = "xxx-b1", description = "xxx-b2")

meta <- pin_meta(board, name)
testthat::expect_equal(meta$title, "xxx-b1")
Expand Down
9 changes: 8 additions & 1 deletion R/pin_versions.R
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,18 @@ version_from_path <- function(x) {
out
}

version_setup <- function(board, name, new_version, versioned = NULL) {
version_setup <- function(board, name, new_version, versioned = NULL, call = caller_env()) {
if (pin_exists(board, name)) {
versions <- pin_versions(board, name)
old_version <- versions$version[[1]]
n_versions <- nrow(versions)
if (old_version == new_version) {
cli::cli_abort(c(
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Right, so why try to overwrite that file?

"The new version {.val {new_version}} is the same as the most recent version.",
i = "Did you try to create a new version with the same timestamp as the last version?"
),
call = call)
}
} else {
n_versions <- 0
}
Expand Down
2 changes: 1 addition & 1 deletion man/pin_meta.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions tests/testthat/_snaps/pin_versions.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@
Warning:
The `full` argument of `pin_versions()` is deprecated as of pins 1.0.0.

# informative error for writing with same version

Code
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x")
Condition
Error in `pin_store()`:
! The new version "20120304T050607Z-xxxxx" is the same as the most recent version.
i Did you try to create a new version with the same timestamp as the last version?

# can prune old versions

Code
Expand Down
8 changes: 8 additions & 0 deletions tests/testthat/test-pin_versions.R
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ test_that("can parse versions from path", {
expect_equal(out$hash, "hash")
})

test_that("informative error for writing with same version", {
local_mocked_bindings(version_name = function(metadata) "20120304T050607Z-xxxxx")
board <- board_temp(versioned = TRUE)
expect_snapshot(error = TRUE, {
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x")
})
})

# versions pruning --------------------------------------------------------

Expand Down
4 changes: 4 additions & 0 deletions vignettes/pins-update.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ pin_read(board, "mtcars")

Since the board object is always the first argument, you might also want to use the pipe:

```{r, echo=FALSE}
Sys.sleep(1)
```

```{r}
# Modern API
board <- board_local()
Expand Down
4 changes: 4 additions & 0 deletions vignettes/pins.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ This includes:

When creating the pin, you can override the default description or provide additional metadata that is stored with the data:

```{r, echo=FALSE}
Sys.sleep(1)
```

```{r}
board %>% pin_write(mtcars,
description = "Data extracted from the 1974 Motor Trend US magazine, and comprises fuel consumption and 10 aspects of automobile design and performance for 32 automobiles (1973–74 models).",
Expand Down