AlgebraicJulia / SyntacticModels.jl

Specifying models with syntax trees
MIT License
2 stars 3 forks source link

wip #13

Open joshday opened 10 months ago

joshday commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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

joshday commented 10 months ago

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 commented 10 months ago

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