Standardize CRUD try catch logic

[dehann]

Error handling inside a CRUD function could be divided into two categories (do or dont do try catch inside CRUD) -- also see similar Midori model here:

• Type 1: Functional or syntactic errors
• Type 2: 'not found' / duplicate / invalid value errors / cannot execute

Repeat example from #134

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 same as var

Only do try catch on CRUD Type 2 errors?

Example DB, bad comms, etc. Similar discussion for internal retries. Type 2 errors would then also return @assert !flag if internal try catch or retries expired.

[Affie]

I think the question becomes: where are the exceptions caught and thrown?

[dehann]

Personal preference is user is responsible for Type 1 errors, even if it is a code problem inside the Caesar framework. Reason: else any error in the code is never caught, its always just "the code fails softly and I dont know where its broken".

However, if all the code is working right, but something physical is resulting in the error -- such as trying to communicating with a DB which is not active, then it makes sense to come back with a message that DB host not reachable -- which is better than an obscure code error. This is what I mean by Type 2 errors. But still log the full obscure error stack somewhere, but any try catch will obviously grab all type 1 errors inside as well. This was a real problem for me a couple of months ago, when trying to debug through layers of try catch statements -- makes it really difficult.

I think Type 2 try catch only works once we know all Type 1 errors below have already been found and resolved. Hence my general avoidance of wrapping try catch statements in the middle somewhere. Think @GearsAD may feel different on this ( and he has the experience ). We had not resolved this decision in the past before.

At minimum we should decide on:

• any potential try catch statements are "as close to the top/user" as possible? OR
• minimize the footprint of try-catch for Type 1 errors by placing them as low down (covering minimum code) as possible?

[dehann]

try catch as low down as possible for known Type 2 errors seems like a viable way to go?

[Affie]

Ok, I tend to agree with you. However I would say it is only applicable to new errors thrown and should not be wrapped in try catch at all.
Take the addVariable example
It has error protection for checking if the variable exists. But throws a new exception. It might be better to just return false with a warning or optional error. The aim then becomes to get the code to not error at all with type 1 errors and put in checks for type 2.

[dehann]

Should we split Type 2 errors further, as either:

• a) something unforseen like DB is down; or
• b) user unforced error, such as delete a variable that is not present?

[dehann]

Upstream,

• julep: "chain of custody" error handling JuliaLang/julia#7026
• RFC: Uncatchable FatalException. Separating bugs from recoverable errors. JuliaLang/julia#15514
• Support catching specific exception types JuliaLang/julia#23418

[dehann]

Scenario:

• addVariable fail on cloud due to bad comms.
• retry Standardize Graph CRUD retry behavior #144
• after several retries a new more serious comms failure occurs,
• thus you need to know why addVariable failed,
• and must track how many times it failed

Suggestion is reason why failed should be in general DFG Caesar code, other errors occur outside dfg.

[Affie]

From an user perspective:

• there should be a @warning/@error/exception, that is human understandable, output to a log/console/etc.

From an automated system perspective:

• error/faults should be distinguishable for severity and
• we need to know where it came from.

[dehann]

I see some of the packages are starting to use specialized exception types for more informative failures -- we are not alone :-)

[dehann]

Comment mistake, taking this over from #182

Sam said (FROM SLACK):

I tried with returning nothing and it causes a lot of problems with derived functions / there’s a discussion on it in the docs
Eg getEstimate(dfg, :x1) uses getVariable. What if the variable doesn’t exist?
The error pattern is already in light graphs so I used that, it solved those issues
I don’t feel strongly about tag operations, really, it just feels like bicycle shedding, whatever you guys want and let’s move on
So, whichever functions you guys want with a list - I just went with the set operators as you suggested that

Personally i prefer hard errors over silent failures. I'm scared returning nothing will create more confusion. Know we spoke about this at length in the past. I vote it should error and user can deal with it in a try catch statement. I personally don't think we should follow a pattern where try catches get built way down (unless they have really useful error messages). But that is the same as throwing a typed exception. The Julia auto generated errors, like :x1 does not exist in labelDict is pretty good already. Punchline, I vote go with allowing errors to propagation up for standard CRUD.

[dehann]

Converging towards decision that graph CRUD should propagate errors and not ::Nothing -- Can we make that the decision and close this?

[dehann]

In time we can add Typed Exceptions as resources allow, but not presume ::Nothing when something didn't work in the "early days".

[Affie]

As long as these hold

From an automated system perspective:

• error/faults should be distinguishable for severity and
• we need to know where it came from.

[dehann]

Sam liked Exceptions over nothing (Slack).

Done, thanks.

[GearsAD]

Personally I believe that errors and typed exceptions should work in Julia, and that we should fail early. I'm hoping this is possible.

If we return nothing in some cases but not all, then functions that make use of inner functions will have strange behavior. Consider getSubgraph(... [:a, :b, :doesntexist]) - if we return nothing or an error, when getVariable is called for :doesntexist the code will have to check for nothing, and then there is another decision on how to handle that. Does getSubgraph return partial graphs if you ask for something that doesn't exist?. That is the root of further issues, e.g. parameters that provide different behavior for different circumstances.

So the code become complex very quickly. Rather - my opinion - fail early and let the user know asap. If we can't, that's ok, but let's try keep things simple if at all possible.