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

asJSON() fails on "vctrs_vctr" objects when posterior package is loaded #408

Closed
idmn opened this issue Jan 12, 2023 · 6 comments
Closed

Comments

@idmn
Copy link

idmn commented Jan 12, 2023

Code to reproduce

loadNamespace("posterior") # (>= 1.3.0)
#> <environment: namespace:posterior>

getClassDef("vctrs_vctr")
#> Virtual Class "vctrs_vctr" [package "posterior"]
#> 
#> Slots:
#>             
#> Name:  .Data
#> Class:  list
#> 
#> Extends: 
#> Class "list", from data part
#> Class "vector", by class "list", distance 2
#> 
#> Known Subclasses: "rvar"

x <- structure("a", class = c("vctrs_vctr", "character"))

jsonlite:::asJSON(x)
#> Error: C stack usage  7954376 is too close to the limit

As a consequence (and a real reason I'm reporting this), this makes googledrive::drive_upload() fail when posterior is loaded.

library(posterior)
library(googledrive)
gd_id <- as_id("<url of some GD folder>")
drive_example_local("chicken.jpg") %>%
  drive_upload(
    name = "name-squatter",
    path = gd_id,
    overwrite = TRUE
  )

The above will fail because gd_id is an object with classes: c("drive_id", "vctrs_vctr", "character") and at some point along the way drive_upload() needs to write this object as a json.

I'm not sure whether it's a purely jsonlite bug or is it posterior doing something wrong and unconventional so please let me know if I should create an issue in the posterior repo. But I suppose either way this can be handled in jsonlite?

@jeroen
Copy link
Owner

jeroen commented Jan 14, 2023

Very weird. It seems to run in an infinite where standardGeneric("asJSON") keeps calling itself instead of dispatching.

The problem is probably that the posterior package redefines vctrs_vec to be a list? But I don't understand why this happens.

@idmn
Copy link
Author

idmn commented Jan 14, 2023

Yeah I think so too. When something is declared to be a list, but acts as an atomic vector, jsonlite has problems with it. Rightfully so I guess. Although maybe there is a good way to check for this? Here Hadley writes that is.atomic() is not the best idea, but maybe there's some other approach.

Because clearly here vapply (used in asJSON method for lists) applies as.list to x which in turn converts it to a list where the first element is x. So basically acts as if it's an atomic vector, and this launches the endless recursion.

@lionel-
Copy link

lionel- commented Jan 16, 2023

I took a look and I agree with your diagnostics. Vectors of class c("vctrs_vctr", "character") get dispatched to the the list S4 method of asJSON(). This method rightfully assumes its input is a list and recurses over its elements using vapply(). Since character vectors are type-preserving, this recursion infloops.

"vctrs_vctr" are designed to inherit from any base vector type, including "list" or "character". I'm not sure what's the right incantation of setOldClass() to reflect this.

@mjskay I saw that you added the setOldClass() call to posterior. Do you have an idea how we could do better? Maybe @DavisVaughan could help too.

@mjskay
Copy link

mjskay commented Jan 16, 2023

Looks like you've solved this issue but I thought I'd check on this -

"vctrs_vctr" are designed to inherit from any base vector type, including "list" or "character". I'm not sure what's the right incantation of setOldClass() to reflect this.

@lionel- do you think posterior should be using something like setOldClass(c("rvar", "vctrs_vctr"))? I'll have to plead ignorance on the particulars of S4 in this case.

@lionel-
Copy link

lionel- commented Jan 16, 2023

@mjskay I'm not sure this is better because then getClassDef("vctrs_vctr") reports a character class:

Virtual Class "vctrs_vctr" [in ".GlobalEnv"]

Slots:
                
Name:   .S3Class
Class: character

Extends: "oldClass"

Known Subclasses: "rvar"

Would the following work for you? This way you're not declaring anything about our vctrs classes:

setOldClass("rvar")

Just guessing as I don't know much about S4 either.

@mjskay
Copy link

mjskay commented Jan 17, 2023

@lionel- moving the rvar / vctrs_vctr conversation to {posterior} so the jsonlite folks don't have to listen: stan-dev/posterior#267

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

No branches or pull requests

4 participants