Use module in DFGVariable softtype serialize/deserialize

[GearsAD]

No description provided.

[GearsAD]

@Affie

[Affie]

I upgraded my branch and I think I ran into this error. It used to work fine but now:

┌ Error: Unable to deserialize soft type RoME.Pose2(3, String[], (:Euclid, :Euclid, :Circular))
└ @ DistributedFactorGraphs ~/.julia/dev/DistributedFactorGraphs/src/services/DFGVariable.jl:72
channel = ┌ Error: UndefVarError: Pose2 not defined
│ Stacktrace:
│  [1] unpack(::CloudGraphsDFG{IncrementalInference.SolverParams}, ::PackedVariableNodeData) at /home/johan/.julia/dev/DistributedFactorGraphs/src/services/DFGVariable.jl:69
│  [2] iterate at /home/johan/.julia/dev/DistributedFactorGraphs/src/CloudGraphsDFG/services/CloudGraphsDFG.jl:194 [inlined]
│  [3] collect(::Base.Generator{Base.ValueIterator{Dict{String,PackedVariableNodeData}},getfield(DistributedFactorGraphs, Symbol("##222#223")){CloudGraphsDFG{IncrementalInference.SolverParams}}}) at ./array.jl:606
│  [4] map at ./abstractarray.jl:2099 [inlined]
│  [5] getVariable(::CloudGraphsDFG{IncrementalInference.SolverParams}, ::Int64) at /home/johan/.julia/dev/DistributedFactorGraphs/src/CloudGraphsDFG/services/CloudGraphsDFG.jl:194
│  [6] getVariable(::CloudGraphsDFG{IncrementalInference.SolverParams}, ::Symbol) at /home/johan/.julia/dev/DistributedFactorGraphs/src/CloudGraphsDFG/services/CloudGraphsDFG.jl:228
│  [7] #220 at /home/johan/.julia/dev/DistributedFactorGraphs/src/CloudGraphsDFG/services/CloudGraphsDFG.jl:175 [inlined]
│  [8] iterate at ./generator.jl:47 [inlined]
│  [9] _collect(::Array{Symbol,1}, ::Base.Generator{Array{Symbol,1},getfield(DistributedFactorGraphs, Symbol("##220#221")){CloudGraphsDFG{IncrementalInference.SolverParams}}}, ::Base.EltypeUnknown, ::Base.HasShape{1}) at ./array.jl:619
│  [10] map at ./array.jl:548 [inlined]
│  [11] addFactor!(::CloudGraphsDFG{IncrementalInference.SolverParams}, ::Array{Symbol,1}, ::DFGFactor{IncrementalInference.CommonConvWrapper{IncrementalInference.Prior{Distributions.MvNormal{Float64,PDMats.PDMat{Float64,Array{Float64,2}},Array{Float64,1}}}},Symbol}) at /home/johan/.julia/dev/DistributedFactorGraphs/src/CloudGraphsDFG/services/CloudGraphsDFG.jl:175
│  [12] addNodeFactor!(::Dict{String,MirroredDFG}, ::SurveyBotLCMTypes.node_factor_t) at /home/johan/.julia/dev/Colosseum/src/helpers.jl:118
│  [13] #main#159(::String, ::String, ::Bool, ::Bool, ::Bool, ::typeof(main)) at /home/johan/.julia/dev/Colosseum/src/Colosseum.jl:392
│  [14] #main at ./none:0 [inlined]
│  [15] macro expansion at /home/johan/.julia/dev/Colosseum/src/controlgui.jl:22 [inlined]
│  [16] (::getfield(Colosseum, Symbol("##11#14")){Base.Iterators.Pairs{Symbol,Any,Tuple{Symbol,Symbol,Symbol},NamedTuple{(:mongoip, :enableBigDataDB, :enableGraphDB),Tuple{String,Bool,Bool}}},String})() at ./task.jl:268
└ @ DistributedFactorGraphs ~/.julia/dev/DistributedFactorGraphs/src/services/DFGVariable.jl:76

[Affie]

Ahh, I see its hard-coded to Main. So can get it to work.
This is likely to become a problem for users if they only do using Caesar, then RoME is not in Main

[dehann]

Just want to know if this was resolved for v0.5.0 or if its still upcoming? If anyone knows, please update the milestone on this issue.

[GearsAD]

I don't think this is resolved, but I'm having trouble reproducing it. Below I import Caesar and DFG but not RoME, and it successfully serializes and deserializes:

using DistributedFactorGraphs
using Caesar
using Test
# using Logging
# logger = SimpleLogger(stdout, Logging.Debug)
# global_logger(logger)

dfg = GraphsDFG{NoSolverParams}()

# Same graph as iifInterfaceTests.jl
numNodes = 10
#change ready and solvable for x7,x8 for improved tests on x7x8f1
verts = map(n -> addVariable!(dfg, Symbol("x$n"), Pose2, labels = [:POSE]), 1:numNodes)
facts = map(n -> addFactor!(dfg, [verts[n], verts[n+1]], Pose2Pose2(MvNormal([10.0;0;pi/3], Matrix(Diagonal([0.1;0.1;0.1].^2))))), 1:(numNodes-1))

# Save and load the graph to test.
saveFolder = "/tmp/fileDFG"
saveDFG(dfg, saveFolder)

copyDfg = DistributedFactorGraphs._getDuplicatedEmptyDFG(dfg)
retDFG = loadDFG(saveFolder, Main, copyDfg)

# Seems fine here.
@test solverData(getVariable(retDFG, :x1)).softtype isa RoME.Pose2

Although, we did have issues with this in the past. We introduced the setSerializationModule call because of it. Are you initializing everything inside a module @Affie?

[Affie]

I'll try and reproduce and add it to a test.

[dehann]

I added this to v0.9.0 if thats okay?

[Affie]

I think a decision was made for softtype to always be declared in Main? In that case, this will not be an issue.
However, it will probably come back to haunt us later. Say, Point2 is declared in Main form another module. Then you would have RoME.Point2 and AnotherModule.Point2 with Point2 not existing in Main

[GearsAD]

I came across this a few days ago - not importing say RoME - and then trying to retrieve a graph with Pose2. In that case the error is cryptic.

Overall our assumption is as @Affie said, that there's one Pose2 in main, and it has to be loaded into main. Otherwise, kaboom. If we're ok with that for now, this is resolved.

A bit of cleanup would be to improve the error message.

[Affie]

The easiest solution I can think of is to always include the module. So RoME.Point2. That way it's not needed in Main and the error message can be very specific:

"module RoME not found to deserialise Point2, try using RoME"

[dehann]

The Julia Visualization libraries (mostly from GeometryTypes) has a collision with Point2. So yeah ... I agree with using RoME.Point2 wherever we can until we decide what to do there.

To be fair, this has always been a problem. I have just been using RoME. whenever it came up.

[Affie]

Ok, I'll update the issue. We can perhaps do it with the other softtype changes JuliaRobotics/IncrementalInference.jl#783

[Affie]

We want to be able to find Point2 (for example) from destabilization even if it has a conflicting name in another module (GeometryTypes in this example)

[dehann]

This is likely to become a problem for users if they only do using Caesar, then RoME is not in Main

This might be due to a missing export in Caesar. I might have added a full reexport in Caesar of all RoME functions which resulted in the error going away (as was mentioned above). Not 100% sure, but i did do the RoME reexport in Caesar during this issue timeframe.

[Affie]

just some ideas so far:

# when not imported to main
modulestring = "Dates"
typestring = "DateTime"
m = Base.root_module(Base.__toplevel__, Symbol(modulestring));
typ = getfield(m, Symbol(typestring));
typ(m.now())

# when still using Main
import Dates
modulestring = "Dates"
typestring = "DateTime"
m = getfield(Main, Symbol(modulestring));
typ = getfield(m, Symbol(typestring));
typ(mod.now())

[dehann]

that there's one Pose2 in main, and it has to be loaded into main. Otherwise, kaboom. If we're ok with that for now, this is resolved.

Sorry never answered this. Yes, that was the design decision a long time back. The only way to discover the types from IIF is to "outsource" where the types and converters are built, and the only way to do that is through the Main context. So, not a case of if we are ok with that, it's a case of we actively decided to do so.

The reasoning is that users can define their own types live, or in their own repos and not have to include the variable/factor definitions into the IncrementalInference library like some of the other SLAM packages out there.

[Affie]

Main has too many issues and is not the only way. I suggest using the parent module (eg. RoME) and not Main.

[dehann]

There is an absolute cardinal requirement for the following to work, and it's non-negotiable:

using IncrementalInference

fg = initfg()
addVariable!(fg, :x0, MultivariateEuclid{3})
addFactor!(fg, [:x0], PartialPrior(...))

# oops, i need that other factor and variable
import IncrementalInference: getSample

@defVariable MyVar 5 SomeManifold

# add the new variable type
addVariable!(fg, :x1, MyVar)

struct MyFactor <: AbstractRelativeFactor
...
end

getSample(::MyFactor) = ...
(::MyFactor)(...) = ....

# add the new factor
addFactor!(fg, [:x0; :x1], MyFactor(...))

# ...
solveTree!(fg)

# oh wait I need to save it
import Base: convert

struct PackedMyFactor <: PackedInfererenceType
  ...
end

convert(::Type{PackedMyFactor}, obj::MyFactor) = ...
convert(::Type{MyFactor}, obj::PackedMyFactor) = ...

saveDFG(fg, "mynewfg")

# happiness
## Just remeber to `fg_ = loadDFG("mynewfg")` will require MyVar and MyFactor to be defined again first.
### This might be a good time to copy paste your development code into a file (doesn't have to be a module) for later reuse.

The only way to do this is via Main?

[dehann]

I suggest using the parent module

If I understand correctly ... I will very strongly oppose this.

[dehann]

Main should be the primary place where discovery on dispatch for serialization occurs.

[Affie]

Best I can think of is to implement it and test. I can only say that main failed me in the 2 cases already. With no workaround in case of a conflict.

[dehann]

We do need to resolve the conflicts, if you referring to something like this #157 (comment)

[dehann]

just to make sure it's said somewhere, the project needs of not having a parent module are far greater. We will resolve the name conflict issue in due course. It's much easier to just wrap stuff in a module, and we want the users to have that freedom. It's part of what differentiates us from other SLAM libraries -- so its super super super super important. Julia has this great dispatch and type inference capability, and it would be such a waste if we drop back to only Polymorphic OO patterns. I cannot stress this enough!

[Affie]

https://docs.julialang.org/en/v1/base/base/#Base.parentmodule

[Affie]

I think I understand your requirement. But don't see how this will influence it at all. It's just the addition of the parent module in the serialization of the softtype. Once it's deserialized it will be exactly the same.

[Affie]

I'm making some progress. Testing with:

using DistributedFactorGraphs
using IncrementalInference
fg = initfg()
DFG.@defVariable Point2 2 (:Euclid, :Euclid) 
addVariable!(fg, :x0, Point2)
saveDFG(fg, "mynewfg")
loadfg = initfg()
loadDFG!(loadfg, "mynewfg")

using RoME
# oops now we have a conflict
# WARNING: using RoME.Point2 in module Main conflicts with an existing identifier./
fg = initfg()
addVariable!(fg, :x0, Main.Point2)
addVariable!(fg, :x1, RoME.Point2)
saveDFG(fg, "mynewfg")
loadfg = initfg()
loadDFG!(loadfg, "mynewfg")

First part works, have to resolve:

ERROR: LoadError: MethodError: Cannot `convert` an object of type 
  VariableNodeData{Point2} to an object of type 
  VariableNodeData{RoME.Point2}

EDIT: that was just because the soft type is serialized in 2 places.... now seems to work well.

[dehann]

From SLACK:

Best to test with something like :PoseSpecial definition. Here is an example factors defined only in Main.
https://github.com/JuliaRobotics/IncrementalInference.jl/blob/master/test/testNumericRootGenericRandomized.jl

you can compare with this example:
https://github.com/JuliaRobotics/IncrementalInference.jl/blob/master/test/TestModuleFunctions.jl

I havent tested with Variables defined only in Main

also, to load a graph depending on something like :PoseSpecial will require the user to reload those types and functions into the Main scope -- and that is fine