New `force_identical_write` arg #735

juliasilge · 2023-04-26T22:47:36Z

Addresses #589

This does not solve all our hashing issues (for example, the pin_hash still only looks at the pin contents and not the metadata) but it is a small step toward a better hashing situation.

After talking with users, I think that an opt-in new argument is the best way forward (rather than, say, a new function or new default behavior). This new check_hash arg comes after the dots and defaults to FALSE. It does require another read of pin_meta() to get the old hash.

library(pins)
b <- board_temp(versioned = TRUE)
b %>% pin_write(1:10, "nice-numbers", check_hash = TRUE)
#> Guessing `type = 'rds'`
#> Creating new version '20230426T224520Z-7f884'
#> Writing to pin 'nice-numbers'

## to change timestamp:
Sys.sleep(2)

b %>% pin_write(1:10, "nice-numbers", check_hash = TRUE)
#> Guessing `type = 'rds'`
#> Warning: The hash of pin "nice-numbers" has not changed and will not be stored.

^{Created on 2023-04-26 with reprex v2.0.2}

With this PR as is now, pin_write() will still return the name invisibly but does not store the pin and generates a warning. Thoughts?

kmasiello · 2023-05-01T14:01:42Z

Tested and works as expected.
If I'm a user, I can work with approach well, though I think the documentation should be explicit in noting that the hash is not affected by changed metadata. I like that it's an opt-in argument, which means I will be reading the docs to understand what I'm getting into.
There might be situations where the user would want the status (pinned or didn't pin) to be returned in addition to the pin name. Every use case I envision though, I can noodle through how I'd determine the pinning status by other means. I think it would be wise to keep our ears open as to whether this is a need from the community that we'd implement in the future.

juliasilge · 2023-05-01T17:06:17Z

Thank you so much @kmasiello! 🙌

There might be situations where the user would want the status (pinned or didn't pin) to be returned in addition to the pin name.

This is a good point. Right now, pin_write() only returns the name (like "julia.silge/nice-numbers") but we have been discussing in #592 how to better organize what should be returned. I'll add this info to that issue and we can see what would work best.

Do we think that now in this PR pin_write() should return something different than the name if it does not update the pin? Or should we wait for broader changes to what is returned?

juliasilge · 2023-05-01T17:36:36Z

R/pin-read-write.R

+      cli::cli_warn(
+        "The hash of pin {.val {name}} has not changed and will not be stored."
+      )
+      return(invisible(name))


We could return something different here, like NULL maybe.

I think that's correct; it should return what pin_write() usually returns.

juliasilge · 2023-05-01T17:45:10Z

Refined the warning message a smidge:

library(pins)
b <- board_connect()
#> Connecting to Posit Connect 2023.03.0 at <https://colorado.posit.co/rsc>
b %>% pin_write(1:10, "julia.silge/really-nice-numbers", check_hash = TRUE)
#> Guessing `type = 'rds'`
#> Writing to pin 'julia.silge/really-nice-numbers'
b %>% pin_write(1:10, "julia.silge/really-nice-numbers", check_hash = TRUE)
#> Guessing `type = 'rds'`
#> Warning: 
#> ! The hash of pin "julia.silge/really-nice-numbers" has not changed.
#> • Your pin will not be stored.
b %>% pin_write(1:11, "julia.silge/really-nice-numbers", check_hash = TRUE)
#> Guessing `type = 'rds'`
#> Writing to pin 'julia.silge/really-nice-numbers'

^{Created on 2023-05-01 with reprex v2.0.2}

kmasiello · 2023-05-01T18:51:39Z

Do we think that now in this PR pin_write() should return something different than the name if it does not update the pin? Or should we wait for broader changes to what is returned?

My gut says this should be handled with the broader considerations of what we return. I don't want to see a bespoke solution here that isn't compatible with future work.

kmasiello · 2023-05-04T15:30:20Z

Additional suggestion and comments after reviewing with users -
check_hash may be better named as check_data_hash to emphasize that metadata is not checked.

Checking metadata for changes is an outstanding ask. A sample scenario is using check_hash = TRUE but wanting to update the metadata to reflect a "current as of" date. You would want to be able to indicate the pin is still fresh, even if the data hasn't changed.

juliasilge · 2023-05-04T16:43:06Z

A sample scenario is using check_hash = TRUE but wanting to update the metadata to reflect a "current as of" date.

Can I make sure I am understanding this? It sounds like users are expressing a desire to:

not fill up their disk space with copies of the same file, and at the same time
have the metadata reflect the pin's freshness, like the last time a write was attempted (info more than just "latest")

This would require a more extensive change than in this PR, will be pretty tough in the near term, and likely would require special handling of different backends (S3 vs. Connect vs. folders vs. ...). As of now, updating the metadata but not the pin contents isn't something straightforward to implement. We could consider how to approach this problem in the future.

I tend to think we should stick with check_hash as the argument name since we will likely think through checking portions of the metadata as well (title, description, tags, and user metadata could all currently change without changing the pin hash).

I tend to think we should still go with the change as scoped here because it lets us offer a solution to the storing-many-copies problem. It does mean folks have to choose between having the metadata reflect the last time the pin contents really changed and the last time a write was attempted; this still seems better than the situation as it is now.

kmasiello · 2023-05-04T17:24:16Z

Agreed. As currently scoped, this helps the main use case tremendously.
If I'm very keen to only store the changed data frame but have a record of the last write attempt, I can bake this logging info into my upstream ETL process.
Similar to the other comment about returning the "wrote / didn't write" status, it will be good to keep ears open to use cases around metadata changes.

hadley

This seems so simple — I love it 😄

I also refreshed my memory of pin_hash(), and I'm confident it's ok to be used in this way. There is some chance that you might get the same hash value with different data values, but it's extremely small. (Or maybe this is better: if you hashed a dataset every day for two years, you're more likely to get hit by a meteor than get a hash collision.)

R/pin-read-write.R

hadley · 2023-05-04T20:18:36Z

R/pin-read-write.R

@@ -111,6 +116,17 @@ pin_write <- function(board, x,
  )
  meta$user <- metadata

+  if (check_hash) {
+    old_hash <- possibly_pin_meta(board, name)$pin_hash


Why do we think this might fail?

This is to let someone use check_hash = TRUE when they write a pin for the first time, before there is any metadata. old_hash returns as NULL here when there isn't any metadata.

Hmmm, I think I'd prefer an explicit pin_exists() check? Is there some reason not to do that?

hadley · 2023-05-04T20:19:04Z

R/pin-read-write.R

+      cli::cli_warn(
+        "The hash of pin {.val {name}} has not changed and will not be stored."
+      )
+      return(invisible(name))


I think that's correct; it should return what pin_write() usually returns.

hadley · 2023-05-04T20:19:51Z

R/pin-read-write.R

@@ -64,6 +64,10 @@ pin_read <- function(board, name, version = NULL, hash = NULL, ...) {
 #'   use the default for `board`
 #' @param tags A character vector of tags for the pin; most important for
 #'   discoverability on shared boards.
+#' @param check_hash Check whether the pin contents are identical to the last


Should we consider defaulting this to TRUE?

Yeah, let's try it. A release is still a little ways out so we can see if it is a problem in an expected way.

Oh actually @hadley the other reason I had not set it to TRUE originally is it adds one additional call to pin_meta(). Do we think this is OK? All the API calls can be especially painful on Connect, but maybe adding yet one more set isn't much marginal difference.

Hmmmm, I think it's probably still worth it. It definitely saves you a lot of time/space in the case where the data hasn't changed. We'll definitely need to promote that change heavily.

NEWS.md

hadley · 2023-05-04T20:20:44Z

R/pin-read-write.R

@@ -64,6 +64,10 @@ pin_read <- function(board, name, version = NULL, hash = NULL, ...) {
 #'   use the default for `board`
 #' @param tags A character vector of tags for the pin; most important for
 #'   discoverability on shared boards.
+#' @param check_hash Check whether the pin contents are identical to the last
+#'   version if one exists (using the hash), and then **do not store** the pin
+#'   again. This argument does not check the pin metadata, only the pin


The wording isn't quite right because the argument isn't checking anything. But I couldn't think of better wording off the top of my head.

What if I change the whole arg to compare_hash?

This would actually be better because there is an internal function already called check_hash(). Take a look and see if you have feedback on the new name/docs.

Co-authored-by: Hadley Wickham <[email protected]>

juliasilge · 2023-05-04T21:31:38Z

tests/testthat/test-board_folder.R

    pin_write(b, 1:5, "x", type = "rds")
    pin_write(b, 1:6, "x", type = "rds")
+    pin_write(b, 1:7, "x", type = "rds")


I did have to change a couple of tests if the default is compare_hash = TRUE

hadley · 2023-05-04T22:17:30Z

R/pin-read-write.R

@@ -64,6 +64,10 @@ pin_read <- function(board, name, version = NULL, hash = NULL, ...) {
 #'   use the default for `board`
 #' @param tags A character vector of tags for the pin; most important for
 #'   discoverability on shared boards.
+#' @param compare_hash Compare the pin contents to the last version if one


I think this is an improvement, but I wondered about something even more direct like write_if_different? Or write_unchanged_data? What do you think?

What about force_identical_write? This one would default to FALSE.

None of these ideas really speak to me (including mine), so just go with your heart 😄

github-actions · 2023-05-20T00:06:12Z

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.

juliasilge added 3 commits April 26, 2023 16:41

New check_hash arg

b30eadd

Add test

5180df4

Update docs

d313fbe

juliasilge mentioned this pull request May 1, 2023

pin_write should return the version as well as the name #592

Open

Clarify docs about no checking of metadata

312171c

juliasilge commented May 1, 2023

View reviewed changes

juliasilge added 2 commits May 1, 2023 11:43

Better warning message

04565eb

Update NEWS

28f0ecd

Update test

856b26c

juliasilge marked this pull request as ready for review May 1, 2023 18:34

juliasilge requested a review from hadley May 1, 2023 18:36

hadley approved these changes May 4, 2023

View reviewed changes

juliasilge and others added 3 commits May 4, 2023 14:43

Apply suggestions from code review

4f219ad

Co-authored-by: Hadley Wickham <[email protected]>

Use cli_inform() instead of warning

7966273

Change arg to compare_hash, default of TRUE

01508aa

juliasilge commented May 4, 2023

View reviewed changes

Update a couple of tests for default of TRUE

d93dab9

hadley reviewed May 4, 2023

View reviewed changes

Switch to force_identical_write, mark as breaking change in NEWS

7503901

juliasilge changed the title ~~New check_hash arg~~ New force_identical_write arg May 5, 2023

juliasilge merged commit cc3c160 into main May 5, 2023

juliasilge deleted the check-hash-arg branch May 5, 2023 18:12

juliasilge mentioned this pull request May 5, 2023

Feature Request: Re-enabling data versioning by hash only #589

Closed

juliasilge mentioned this pull request May 5, 2023

Should pin_hash be used to track changes in the metadata as well as pin contents? #739

Closed

github-actions bot locked and limited conversation to collaborators May 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `force_identical_write` arg #735

New `force_identical_write` arg #735

juliasilge commented Apr 26, 2023

kmasiello commented May 1, 2023

juliasilge commented May 1, 2023 •

edited

Loading

juliasilge May 1, 2023

hadley May 4, 2023

juliasilge commented May 1, 2023 •

edited

Loading

kmasiello commented May 1, 2023

kmasiello commented May 4, 2023

juliasilge commented May 4, 2023

kmasiello commented May 4, 2023

hadley left a comment •

edited

Loading

hadley May 4, 2023

juliasilge May 4, 2023

hadley May 4, 2023 •

edited

Loading

hadley May 4, 2023

hadley May 4, 2023

juliasilge May 4, 2023

juliasilge May 4, 2023

hadley May 4, 2023

hadley May 4, 2023

juliasilge May 4, 2023

juliasilge May 4, 2023

juliasilge May 4, 2023

hadley May 4, 2023

juliasilge May 4, 2023 •

edited

Loading

hadley May 5, 2023

github-actions bot commented May 20, 2023

New force_identical_write arg #735

New force_identical_write arg #735

Conversation

juliasilge commented Apr 26, 2023

kmasiello commented May 1, 2023

juliasilge commented May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge commented May 1, 2023 • edited Loading

kmasiello commented May 1, 2023

kmasiello commented May 4, 2023

juliasilge commented May 4, 2023

kmasiello commented May 4, 2023

hadley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2023

New `force_identical_write` arg #735

New `force_identical_write` arg #735

juliasilge commented May 1, 2023 •

edited

Loading

juliasilge commented May 1, 2023 •

edited

Loading

hadley left a comment •

edited

Loading

hadley May 4, 2023 •

edited

Loading

juliasilge May 4, 2023 •

edited

Loading