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

Feat manifest compat #178

Merged
merged 8 commits into from
Dec 20, 2022
Merged

Feat manifest compat #178

merged 8 commits into from
Dec 20, 2022

Conversation

machow
Copy link
Collaborator

@machow machow commented Dec 16, 2022

Addresses the first piece of #174, by ensuring boards with a _pins.yaml manifest do not attempt to treat it like a pin.

  • Copied over tests/testthat/pin-board from R pins
  • Added tests for some pin_read/exists/write/search cases against that board.
  • I noticed that pins in that board have a tags field in their metadata, which causes pins-python to error 😬. I added the tags field to pins.meta.Meta to address. (but alternative suggestion below).

tags and backwards compatibility

It seems like there might be two approaches to support for pins written after tags are implemented:

  • Provide compatibility in the next pins-python version. Previous versions of pins-python will error because they don't expect a tags field.
  • Bump to api_version=2 when adding tags. Previous versions of pins-python will error saying "Unsupported api_version: 2".

It seems easier to steer people to upgrade with the second choice?!

should pins-python just read all fields of any metadata file?

Currently, pins-python has a schema it expects for api_version=1 metadata. If it encounters additional fields, should it not raise an error, but instead just shove them somewhere? (e.g. pins parses a few fields out of the pins v0 metadata, and then chucks all the original data into a original_fields property.)

@machow
Copy link
Collaborator Author

machow commented Dec 16, 2022

I'm not sure how important this is, but I can't upload the example data pin-board to rsconnect, because its pin names are less than 3 characters 😭. But also, it's not really needed, since we can't add a manifest there anyway

@juliasilge
Copy link
Member

juliasilge commented Dec 17, 2022

Hmmmm, in R, we can read new pins written with tags with an old version of pins from before tags were added. With pins 1.0.0 (I made sure to delete my cache for this pin, etc):

library(pins)
b <- board_rsconnect()
#> Connecting to RSC 2022.11.0 at <https://colorado.posit.co/rsc>
pin_read(b, "julia.silge/great-numbers")
#>  [1]  1  2  3  4  5  6  7  8  9 10
pin_meta(b, "julia.silge/great-numbers")
#> List of 11
#>  $ file       : chr "great-numbers.rds"
#>  $ file_size  : 'fs_bytes' int 61
#>  $ pin_hash   : chr "4d1eb93dc2188114"
#>  $ type       : chr "rds"
#>  $ title      : chr "julia.silge/great-numbers: a pinned integer vector"
#>  $ description: NULL
#>  $ tags       : chr [1:2] "i-love-integers" "numbers-are-fun"
#>  $ created    : POSIXct[1:1], format: "2022-11-21 14:09:00"
#>  $ api_version: num 1
#>  $ user       : list()
#>  $ local      :List of 4
#>   ..$ dir       : 'fs_path' chr "~/Library/Caches/pins/rsc-e62371cfd77db754024f9c5ed3556a73/cc8ef31c-d6b3-4e0a-be8e-7992efffaee0/66084"
#>   ..$ url       : chr "https://colorado.posit.co/rsc/content/cc8ef31c-d6b3-4e0a-be8e-7992efffaee0/_rev66084/"
#>   ..$ version   : chr "66084"
#>   ..$ content_id: chr "cc8ef31c-d6b3-4e0a-be8e-7992efffaee0"

Created on 2022-12-16 with reprex v2.0.2

I think this is because of how read_meta() (the function that reads the downloaded YAML) works. What do you think about having similar behavior in Python? I think that means I would lean "yes" to:

should pins-python just read all fields of any metadata file?

This doesn't solve the problem of people with current older version of pins for Python but seems like a good change for moving forward.

My inclination is that this is not a very significant change to mark as v2 of the pins API. For context, the change from 0 to 1 was the very different, from the legacy boards to what we are calling "modern" boards. Different functions, different interface entirely. Especially because it is backwards compatible both ways in R (old write + new read, new read + old write) I don't see the need for bumping the pins API version again.

Copy link
Member

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

I think this looks good. 👍 These are the important tests IMO to make sure a board is usable by a Python user once a manifest file has been added.

@machow
Copy link
Collaborator Author

machow commented Dec 19, 2022

Thanks! I totally missed that R pins just reads the whole file into its metadata list when writing py pins 😭. I've modified py pins Meta data class, so it just stores all unknown fields in an _unknown_fields field.

Some details:

  • Users can still access any unknown fields using e.g. meta.some_unknown_field_name
  • Meta.to_dict() currently doesn't dump out unknown fields, so methods like pin_versions won't show them in its DataFrame output. (this can always change later, but wanted to make sure at least the "read any metadata" piece works!)

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.

2 participants