JuliaRobotics
Standardize DFG Graph CRUD return types
FROM SLACK
Currently the behavior of GraphsDFG and CloudDFG is somewhat different. We want it consistent, but i would not want the cloud breaking julia, but rather the ability to retry
I like the cloud graphs addVariable in that it does not error, but not the automatic update if it exists. I would propose the behavior is to @error on exists and return false.
Update does not seem to change the graph structure in in-memory types which might be a bug? It errors on GraphsDFG but creates with @warn on CloudDFG. Create with @warn or @error seems logical
Hi Johan consistent definitely good, cloud @error with false a good start. The rerun false might be under question at some point since initially addVariable used to return the variable object. Maybe escalate the in memory to also be @error. Probably need a hard decision addVariable return type. Should limit to just one type I think.The retry can work well, although there is a discussion on whether user or driver has that responsibility. If it’s small then maybe a keyword argument to disable retry?Not sure on the return type for delete, that could perhaps be nice if the DFGVariable is returned, or empty if failed?
On retry i would say user responsibility, auto retry can maybe be implemented in RoME. What I meant was, rather return a false or nothing with an @error or @warn than throwing an exception that stops julia.
Agree not to throw exception. I’d suggest that that there is only one return type from the function for performance, but somehow indicates success or failure.
On addVariable!: IIF returns the variable, DFG true/false. What is preferred. I would think if the function name is the same it should have the same behavior.
definitely only one return type, but a Nothing Union eg. Union{Nothing, DFGVariable} might also be acceptable as the performance is not impacted that much since julia 0.7
Right, union types thing... theres something I’m not sure about yet. There is a macro type_warn which shows type instability which is slow. Do return Union in functions skip dynamic type inference — will be great if that’s the case. Sounds like 0.7 was able to do that..? At some point I’ll try the type_warn thing to see if Union return is stable, having a hard time understanding how that would be the case though. As long as things are type stable I think it would be good?
Its used in julia’s code base, eg findfirst. I kind of remember reading that small type unions were now fast, but that might have been in containers.
Oh cool, okay thanks. Does this not also solve the dense adjacency matrix problem — ie Should use Union with isbits types like Missing. Not to replace sparse. I mean when going between sparse and dense representations.It does look like references are bad. So a Union{Missing, DFGVariable} would still be bad? But a Union{Missing, Bool} would be good... if I read that right. It’s immutable types that the compiler can optimize.
looks like Union{Missing, T} and Union{Nothing, T} would then be optimized in the same way? Preference as to which type? At the moment we're using a lot of Union{Nothing, T}
General design pattern should be exception throwing - standard across languages - if we use the return checking approach we force the user to do the work. I get that Julia isn't good with exception handling yet (for some reason, hasn't stuck in Julia) , so let's go with the return code approach. Sound like a plan?
just a consideration: as we introduce functions that talk to databases etc., we will end up trapping the potential exceptions in the addVariable function, and returning true/false, which may make it more difficult for users to debug why it failed (does the variable already exists? is it a comms issue? etc.)
E.g. using the return true/false approach. If this works we can make the driver changes: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/pull/136
Hi, I think the point of that blog is Union nothing is bad but Union Missing is good. Not 100% sure but at least Union Missing is good.I think try catch exception handling is good. Julia does the job, don’t think there is an issue with that. My previous hold up with try statements was precisely the same issue of a graceful failure or one sentence error message but without reporting the call stack. I’d prefer to use try catch but then just log the stack trace and message properly so we can debug more easily. Try catch should not be used for duplicate adds or delete a missing value, right?So sounds like 2 questions then, if I have this right and feel free to disagree:
- What is return types for graph cruds — considering missings? Suggestion is Union{Missing, <: isbits}...
- Missing is not an exception error but other errors should be caught internally vi try catch with full stack logs? Currently only some of the DB code has this style...
My question on IIF/RoME got missed, so repeating: On addVariable!: IIF returns the variable, DFG true/false. What is preferred. I would think if the function name is the same it should have the same behavior. I currently use the IIF/RoME variable that is returned. The options for addVariable!:
- Inconstant behavior; IIF return variable, DFG true/false or true exception.
- return true/false, use
addVariable!thengetVariable!-> I think this will have a big impact downstream. - return variable everywhere, but question becomes how to deal with failure.
My understanding of the blog is two parts:
- functions: any 'small unions' are acceptable
- arrays: only bit types.
returning nothing will automatically break the users code if a dfg variable is expected.
perhaps a flag that either throws exception or @error with nothing return?
https://docs.julialang.org/en/v1/manual/style-guide/index.html#Don't-overuse-try-catch-1
My question on IIF/RoME got missed, so repeating: On addVariable!: IIF returns the variable, DFG true/false. What is preferred. I would think if the function name is the same it should have the same behavior. I currently use the IIF/RoME variable that is returned. The options for addVariable!:
- Inconstant behavior; IIF return variable, DFG true/false or true exception.
- return true/false, use addVariable! then getVariable! -> I think this will have a big impact downstream.
- return variable everywhere, but question becomes how to deal with failure.
~~HI Please continue this discussion in the issue thread~~. Question did not get missed sorry -- answer is behavior should be the same and we now need to make a design decision.
I am personally not a big fan of returning union types yet, but chewing on it.
Hi @GearsAD , do you have strong preference on return type for graph CRUD -- think we going to vote on this to resolve? My feeling is we should return DFGVariable/DFGFactor type, but that leaves the issue of what to do when error occurs.
- I don't know if Union{Missing, <:Node} is the the most performant and can create type instability, since it is not a 'small union'.
- Error handling inside the function is tangential issue, but there are two categories (do or dont do try catch inside CRUD):
- Type 1: Functional or syntactic errors
- Type 2: 'not found' / duplicate / invalid value errors / cannot execute
Alternative Suggestion for CRUD Return
How about always returning a Tuple{Bool, <:DFGNode}. The bool flag states success and the second position is the relevant response. ie
fg = initfg()
flag, var = addVariable!(fg, :x1, Pose2)
@assert flag
flag, var__ = addVariable!(fg, :x1, Pose2)
@assert !flag
# var__ should be empty container, but specifically in this case could also be previous var
Only do try catch on CRUD Type 2 errors?
xref #144
Summary of discussion with @GearsAD (Please feel free to update if I misunderstood something):
-add...! adds.
-update...! updates, if it exists, adds (with @warn) if not.
-detete...!
-get...!
All throws exception on fail or returns variable/factor on success. (to fit with RoME/IIF)
Forgot about the flag. Its need will depend on where the error handling is done. I think it will be most usefull in the external dfg types (cloud, file, etc) for example:
function addVariable!(dfg::CloudGraphsDFG, variable::DFGVariable)::Bool
if exists(dfg, variable)
if getErrorHandlingFlag(dfg) #returns dfg.throwExceptionOnError
error("Variable '$(variable.label)' already exists in the factor graph")
else
@error "Variable '$(variable.label)' already exists in the factor graph"
# preverably with a stack trace, but since you can set throwExeptionOnError true it should be easy to debug
end
return false
end
...
...
return true
end
With the return type pending this decision
small union: This optimization applies to all Unions of a small number of types, whether they are bits types or not. “Small” is defined by the MAX_UNION_SPLITTING constant, which is currently set to 4
right, so i'm guessing DFGVariable or DFGFactor would exceed MAX_UNION_SPLITTING?
All throws exception on fail or returns variable/factor on success. (to fit with RoME/IIF)
RoME/IIF can change as needed. Starting a new thread for that here #144
My understanding Union{Nothing, Any} -> bad. Union{Nothing, DFGVariable} -> ok, spit in 2.