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

wip #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

wip #13

wants to merge 2 commits into from

Conversation

joshday
Copy link
Collaborator

@joshday joshday commented Aug 28, 2023

Got the example at https://algebraicjulia.github.io/SyntacticModels.jl/dev/generated/uwd_examples/ working

julia> u
{ R(x:X, y:Y)
  S(y:Y, z:Z)
  T(z:Z, y:Y, u) } where {x:X, z:Z}

julia> str = JSON3.write(u)
"{\"_type\":\"UWDExpr\",\"context\":[{\"_type\":\"Typed\",\"var\":\"x\",\"type\":\"X\"},{\"_type\":\"Typed\",\"var\":\"z\",\"type\":\"Z\"}],\"statements\":[{\"_type\":\"Statement\",\"relation\":\"R\",\"variables\":[{\"_type\":\"Typed\",\"var\":\"x\",\"type\":\"X\"},{\"_type\":\"Typed\",\"var\":\"y\",\"type\":\"Y\"}]},{\"_type\":\"Statement\",\"relation\":\"S\",\"variables\":[{\"_type\":\"Typed\",\"var\":\"y\",\"type\":\"Y\"},{\"_type\":\"Typed\",\"var\":\"z\",\"type\":\"Z\"}]},{\"_type\":\"Statement\",\"relation\":\"T\",\"variables\":[{\"_type\":\"Typed\",\"var\":\"z\",\"type\":\"Z\"},{\"_type\":\"Typed\",\"var\":\"y\",\"type\":\"Y\"},{\"_type\":\"Untyped\",\"var\":\"u\"}]}]}"

julia> u2 = JSON3.read(str, AbstractTerm)
{ R(x:X, y:Y)
  S(y:Y, z:Z)
  T(z:Z, y:Y, u) } where {x:X, z:Z}

The cool thing is that the code I've added (Moved to SyntacticModels.jl from SyntacticModelsBase.jl) should make every struct created via MLStyle work out of the box. You don't need to add StructTypes boilerplate anymore.

The bummer is that I had to use @eval to do it. It's possible to do all the StructTypes stuff without using eval except for MLStyle-generated structs that only have one field.


My editor deleted whitespace at the end of lines...making the diff artificially bigger.

@jpfairbanks
Copy link
Member

@joshday, what else do you want to get in here before merging? This eval isn't going to cause world age issues, right? My heuristic for that is "is this eval going to be in a loop any time soon?"

@joshday
Copy link
Collaborator Author

joshday commented Aug 29, 2023

Well I haven't actually run the tests yet to see if I broke anything...

@joshday joshday marked this pull request as ready for review August 29, 2023 18:03
@joshday
Copy link
Collaborator Author

joshday commented Aug 29, 2023

No world age issues with eval.

You will be missing a necessary method for JSON3.read-ing a type if you define an AbstractTerm outside of this package, though.

jpfairbanks added a commit that referenced this pull request Aug 30, 2023
@jpfairbanks
Copy link
Member

I tested this and found a bug where the constructor doesn't drop the :_type field on reading the inputs. I tried to fix this by changing the constructor you defined with @eval, but it isn't the method being called inside JSON3.read

I did this:

for T in concrete_subtypes(AbstractTerm)
    @eval function $(parentmodule(T)).$(T.name.name)(x::NamedTuple)
        fields = filter(x -> x != :_type, fieldnames($T))
        args = collect(x[fields])
        @show args
        $(parentmodule(T)).$(T.name.name)(args...)
    end
end

But the @show call isn't producing output and the constructor is still getting a NamedTuple.

It looks like JSON3 is invoking construct directly rather than constructing a type with T(x...)

  Stacktrace:
    [1] construct(T::Type, args::NamedTuple{(:_type, :name, :schema, :description, :schema_name, :model_version), Tuple{Symbol, Vararg{String, 5}}}; kw::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
      @ StructTypes ~/.julia/packages/StructTypes/AK4aM/src/StructTypes.jl:327
    [2] construct(T::Type, args::NamedTuple{(:_type, :name, :schema, :description, :schema_name, :model_version), Tuple{Symbol, Vararg{String, 5}}})
      @ StructTypes ~/.julia/packages/StructTypes/AK4aM/src/StructTypes.jl:327
    [3] #read#44
      @ ~/.julia/packages/JSON3/L8Yfy/src/structs.jl:435 [inlined]
    [4] read(::StructTypes.CustomStruct, buf::Base.CodeUnits{UInt8, String}, pos::Int64, len::Int64, b::UInt8, ::Type{Header})
      @ JSON3 ~/.julia/packages/JSON3/L8Yfy/src/structs.jl:432
    [5] (::JSON3.StructClosure{Base.CodeUnits{UInt8, String}, NamedTuple{(), Tuple{}}})(i::Int64, nm::Symbol, TT::Type; kw::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
      @ JSON3 ~/.julia/packages/JSON3/L8Yfy/src/structs.jl:557

I also deleted a call to Dict(m) in core.jl because we don't need that anymore with the new features you wrote. I pushed my changes to a branch on origin (instead of your fork).

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