[API Design] dispatch vs verbNoun-verbosity

[Affie]

I moved the following comment here to make the discussion easier on all the bullet point:
We discussed this at length in call today and think we are at parity on "cross-over" between DFG and SDK, if the following notes resonate, then we can close this issue.

Key notes from today's call:

• DFG should only export methods that one would expect to find in the SDK, as to smooth the transition for someone starting in the open-domain and eventually starting to use cost savings from NavAbility.
• DFG should mark as public stable functions that power users can use for more performant code.
• Downstream pre-v1 packages like IIF can special using DFG: BlaBla and reexport (bit of a hack but seems okay before IIF v1 -- we can review then).
• SDK.jl can have more functions than SDK.nonjl -- there is no reason to tie Julia's hands behind it's back because python is bad.
• The "omni intersect signatures" found in all SDKs should be the same to at least 98%, given some wiggle room for language specific quirks like Symbols in Julia and Strings in python.
• SDK.jl depends on DFG types.
• Sorting out good multiple dispatch between DFG and SDK is going to take time, and use of strict verbNoun and aliases for alternate function names are the rules of the road. The combo of multiple dispatch philosophy and verbNoun convention is making the implementation work (considering multiple language SDKs) helluva difficult, but I truly believe these principles will produce a better product/API in the end. Because the conventions by definition reject duplication -- any time there is an argument over something not fitting right, it usually means there is a mistake in the design.
• If multiple dispatch can produce a simpler name in DFG/SDK.jl then the alternates can be implemented by alias so that say SDK.py can have the same name (e.g. merge(::DFG, ::Symbol, ::State) may be good enough for Julia, but not python. So Julia should have the alias mergeState so that Python can also have mergeState). CAVEAT, it is hard to know these signatures before DFG v1.0 is released so there is a workaround, which is to instead implement the verbose mergeState in DFG, and alias the reduction merge. All indications are that this does not block DFG v1.X from moving the implementation to the shortest signature.name and aliasing the more verbose, hence either-or works for DFG v1.0. Just keep moving.
• Particular care must be given to accessors vs. CRUD. Trying to make in-memory DFG harmonize with SDK.nonjl results in design inconsistencies, so solution is to only export composite signatures that allow the full route to be resolved. See next bullet for in-memory accessors. Affie made a good example:

addState(::Var, ::State)  # SDK will not know the route to this data so unusable in SDK but still works in DFG
addState(::FactorGraph, ::Symbol, ::State)  # this allows full data route to be resolved at an SDK level

the first function should only be available to Julia DFG powerusers and should be marked as public when stable, but not exported from DFG. The second function should be the first introduction for new users, so that transition from DIY DFG leads to smoother transition when SDK cost savings become relevant to a new NavAbility customer.

• getters and setters can be tricky (otherwise we wouldn't need merge verb). In example above, there is no way to mark addState as both public and export. So in-memory accessors for DFG should use the variable.state notation rather than getState(:Variable) / getVariableState(variable::Variable). There are cases where dispatch is required, such as variable.state but variable might be a Datalevel 3 (VariableDFG) object. The permissible solution is to overload the Julia getfield method to intercept the call and resolve necessary unpacking (i.e. dispatch) there.
• Unresolved decision for things like getStateDict, how does that go with verb get returning different objects etc. Can resolve that individually. Overall "cross-over" concept is now better captured here.
• (there was one more insight Affie had during the call but was not able to get it down here in the notes -- sorry)

Originally posted by @dehann in #56

[Affie]

• DFG should only export methods that one would expect to find in the SDK, as to smooth the transition for someone starting in the open-domain and eventually starting to use cost savings from NavAbility.
• DFG should mark as public stable functions that power users can use for more performant code.

[Affie]

I agree. See wip of distributed API in https://github.com/NavAbility/NavAbilitySDK.jl/pull/293/files

[Affie]

Functions marked public are also considered stable and if the need is there can become part of the distributed/"cros over" API.

[Affie]

• SDK.jl can have more functions than SDK.nonjl -- there is no reason to tie Julia's hands behind it's back because python is bad.
• The "omni intersect signatures" found in all SDKs should be the same to at least 98%, given some wiggle room for language specific quirks like Symbols in Julia and Strings in python.

[Affie]

• SDK.jl depends on DFG types.

[Affie]

That is correct, it also depends on the new packages IncrementalInferenceTypes and RoMETypes. Any types that need to be serialized should be considered to be split into a lightweight packes to support packing and unpacking for DFG, ie. there will likeley also be a CaesarTypes

[Affie]

• If multiple dispatch can produce a simpler name in DFG/SDK.jl then the alternates can be implemented by alias so that say SDK.py can have the same name (e.g. merge(::DFG, ::Symbol, ::State) may be good enough for Julia, but not python. So Julia should have the alias mergeState so that Python can also have mergeState). CAVEAT, it is hard to know these signatures before DFG v1.0 is released so there is a workaround, which is to instead implement the verbose mergeState in DFG, and alias the reduction merge. All indications are that this does not block DFG v1.X from moving the implementation to the shortest signature.name and aliasing the more verbose, hence either-or works for DFG v1.0. Just keep moving.

[Affie]

This is where we are not on the same page yet... Sure for DFG v1.0 we can start with the shortest unambiguous signature that does not depend on dispatch.

My biggest concern here is that I would like the same verbNoun pattern to be followed as the suggested pattern specifically related to and moving between SDKs:

DFG should only export methods that one would expect to find in the SDK, as to smooth the transition for someone starting in the open-domain and eventually starting to use cost savings from NavAbility.

Take State as an easy case example noun, the distributed/"cross-over" signatures are:

getState(dfg, :x1, :default)
addState!(dfg, :x1, state::State)
mergeState!(dfg, :x1, state::State)
deleteState!(dfg, :x1, :default)

In jula only we can dispatch on ::State therefore the shortest versions are:

getState(dfg, :x1, :default)
add!(dfg, :x1, state::State)
merge!(dfg, :x1, state::State)
deleteState!(dfg, :x1, :default)

• If that is the signatures used in let's say an example using GraphsDFG then a new user that works from the example will find it difficult to use the example as a guide for something in SDK.py.
  So I would sa in this case add! and merge! will not be exported.

Now take the more complex case of the Metadata noun where there are no way to dispatch on the types, (these signitures might contain errors):

# Variable Metadata
getVariableMetadata(dfg::AbstractDFG, variableLabel::Symbol)
addVariableMetadata!(dfg::AbstractDFG, variableLabel::Symbol, metadata::MetadataTypes)
mergeVariableMetadata!(dfg::AbstractDFG, variableLabel::Symbol, metadata::MetadataTypes)
deleteVariableMetadata!(dfg::AbstractDFG, variableLabel::Symbol)

# Factor Metadata
getFactorMetadata(dfg::AbstractDFG, factorLabel::Symbol)
addFactorMetadata!(dfg::AbstractDFG, factorLabel::Symbol, metadata::MetadataTypes)
mergeFactorMetadata!(dfg::AbstractDFG, factorLabel::Symbol, metadata::MetadataTypes)
deleteFactorMetadata!(dfg::AbstractDFG, factorLabel::Symbol)

# Agent Metadata
getAgentMetadata(dfg::AbstractDFG, label::Symbol)
addAgentMetadata!(dfg::AbstractDFG, label::Symbol, metadata::Metadata)
mergeAgentMetadata!(dfg::AbstractDFG, label::Symbol, metadata::Metadata)
deleteAgentMetadata!(dfg::AbstractDFG, label::Symbol)

# Graph Metadata
getGraphMetadata(dfg::AbstractDFG, label::Symbol)
addGraphMetadata!(dfg::AbstractDFG, label::Symbol, metadata::MetadataTypes)
mergeGraphMetadata!(dfg::AbstractDFG, label::Symbol, metadata::MetadataTypes)
deleteGraphMetadata!(dfg::AbstractDFG, label::Symbol)

# Variable Blobentry Metadata
getVariableBlobentryMetadata(dfg::AbstractDFG, variableLabel::Symbol, label::Symbol)
addVariableBlobentryMetadata!(dfg::AbstractDFG, variableLabel::Symbol, label::Symbol, metadata::MetadataTypes)
mergeVariableBlobentryMetadata!(dfg::AbstractDFG, variableLabel::Symbol, label::Symbol, metadata::MetadataTypes)
deleteVariableBlobentryMetadata!(dfg::AbstractDFG, variableLabel::Symbol, label::Symbol)

# Factor Blobentry Metadata
getFactorBlobentryMetadata(dfg::AbstractDFG, factorLabel::Symbol, label::Symbol)
addFactorBlobentryMetadata!(dfg::AbstractDFG, factorLabel::Symbol, label::Symbol, metadata::MetadataTypes)
mergeFactorBlobentryMetadata!(dfg::AbstractDFG, factorLabel::Symbol, label::Symbol, metadata::MetadataTypes)
deleteFactorBlobentryMetadata!(dfg::AbstractDFG, factorLabel::Symbol, label::Symbol)

# Agent Blobentry Metadata
getAgentBlobentryMetadata(dfg::AbstractDFG, agentLabel::Symbol, label::Symbol)
addAgentBlobentryMetadata!(dfg::AbstractDFG, agentLabel::Symbol, label::Symbol, metadata::MetadataTypes)
mergeAgentBlobentryMetadata!(dfg::AbstractDFG, agentLabel::Symbol, label::Symbol, metadata::MetadataTypes)
deleteAgentBlobentryMetadata!(dfg::AbstractDFG, agentLabel::Symbol, label::Symbol)

# Graph Blobentry Metadata
getGraphBlobentryMetadata(dfg::AbstractDFG, entryLabel::Symbol, label::Symbol)
addGraphBlobentryMetadata!(dfg::AbstractDFG, entryLabel::Symbol, label::Symbol, metadata::MetadataTypes)
mergeGraphBlobentryMetadata!(dfg::AbstractDFG, entryLabel::Symbol, label::Symbol, metadata::MetadataTypes)
deleteGraphBlobentryMetadata!(dfg::AbstractDFG, entryLabel::Symbol, label::Symbol)

The same is valid for Blobentry. Now take add! for example, you can have the short version on Variable, Factor, State but need the full version on BlobEntry, Metadata or any type that is not unique.
All languages has rules to help you learn the language, unfortunately most languages then have long lists of exemptions to that rules and I don't want users to need to learn a bunch of rules and exeptions to those rules.

---

Then also from a developer experience. Verbosity help

• Code becomes self documenting.

• having mergeState!, mergeStates!, mergeVariable!, mergeVariables! etc as opposed to only merge! as the implementation makes it easier to find the correct functions and make sense of errors.

• Code is easier to read, as in some places there was just getSolverData(dfg, lbl) (deprecated, but a good example), just reading the line it is impossible to know if lbl was a variable or a factor. Similarly, you might encounter a add!(dfg, label, x), sure julia uses inference to find the type of x, but as a developer it's easier to understand if you say addState!(dfg, label, x). (x here is bad coding style, but just an extreme example)

• Only exporting the full signatures makes it possible for us to use the short versions to implement the "by reference" versions, for example:

addState!(::Var, ::State)  # SDK will not know the route to this data so unusable in SDK but still works in DFG
addState!(::FactorGraph, ::Symbol, ::State)  # this allows full data route to be resolved at an SDK level

we can change addState!(::Var, ::State) to add! I was thinking of simplifying code with:

function _add!(container::AbstractDict{Symbol, <:T}, obj::T) where {T}
    label = getLabel(obj)
    haskey(container, label) && throw(LabelExistsError(string(nameof(T)), label))
    push!(container, label => obj)
    return obj
end

It can maybe be addapted to some middle ground here.

[Affie]

• Particular care must be given to accessors vs. CRUD. Trying to make in-memory DFG harmonize with SDK.nonjl results in design inconsistencies, so solution is to only export composite signatures that allow the full route to be resolved. See next bullet for in-memory accessors. Affie made a good example:

addState(::Var, ::State)  # SDK will not know the route to this data so unusable in SDK but still works in DFG
addState(::FactorGraph, ::Symbol, ::State)  # this allows full data route to be resolved at an SDK level

the first function should only be available to Julia DFG powerusers and should be marked as public when stable, but not exported from DFG. The second function should be the first introduction for new users, so that transition from DIY DFG leads to smoother transition when SDK cost savings become relevant to a new NavAbility customer.

• getters and setters can be tricky (otherwise we wouldn't need merge verb). In example above, there is no way to mark addState as both public and export. So in-memory accessors for DFG should use the variable.state notation rather than getState(:Variable) / getVariableState(variable::Variable). There are cases where dispatch is required, such as variable.state but variable might be a Datalevel 3 (VariableDFG) object. The permissible solution is to overload the Julia getfield method to intercept the call and resolve necessary unpacking (i.e. dispatch) there.
• Unresolved decision for things like getStateDict, how does that go with verb get returning different objects etc. Can resolve that individually. Overall "cross-over" concept is now better captured here.
• (there was one more insight Affie had during the call but was not able to get it down here in the notes -- sorry)

[Affie]

getters and setters can be tricky (otherwise we wouldn't need merge verb). In example above, there is no way to mark addState as both public and export. So in-memory accessors for DFG should use the variable.state notation rather than getState(:Variable) / getVariableState(variable::Variable). There are cases where dispatch is required, such as variable.state but variable might be a Datalevel 3 (VariableDFG) object. The permissible solution is to overload the Julia getfield method to intercept the call and resolve necessary unpacking (i.e. dispatch) there.

I'm not in favour of using/encouraging direct field access outside DFG in general:

• I like using map(getLabel, variables) as apposed to map(v->v.abel, variables)
• Using direct field access means fields should then become part of the stable API and cannot be changed.
• See the style guide: https://docs.julialang.org/en/v1/manual/style-guide/#Prefer-exported-methods-over-direct-field-access

(there was one more insight Affie had during the call but was not able to get it down here in the notes -- sorry)

This might have been that we should rename addState(::Var, ::State) to help make it explicit that modifications are made by reference. see related https://github.com/NavAbility/engineering-and-technology/discussions/198#discussioncomment-13987407

I'm leaning towards just thin wrappers here, but this will take careful consideration. Here is a starting point:

function _get(container::AbstractDict{Symbol, <:T}, label::Symbol) where {T}
    haskey(container, label) || throw(LabelNotFoundError(string(nameof(T)), label))
    return container[label]
end

function _add!(container::AbstractDict{Symbol, <:T}, obj::T) where {T}
    label = getLabel(obj)
    haskey(container, label) && throw(LabelExistsError(string(nameof(T)), label))
    push!(container, label => obj)
    return obj
end

function _merge!(container::AbstractDict{Symbol, <:T}, obj::T) where {T}
    container[getLabel(obj)] = obj
    return 1
end

function _delete!(container::AbstractDict{Symbol, <:T}, label::Symbol) where {T}
    haskey(container, label) && throw(LabelNotFoundError(string(nameof(T)), label))
    delete!(container, label)
    return 1
end

[Affie]

Also worth noting is that I do not see any other in-memory DFG drivers in the near future. These optimizations are therefore not really needed now.
Using _get, _add!, _merge!, and _delete! and the verb-nouns most functions can be autogenerated, but that sometimes makes for harder code to understand and maintain. Don't-overuse-macros

[Affie]

• Unresolved decision for things like getStateDict, how does that go with verb get returning different objects etc. Can resolve that individually. Overall "cross-over" concept is now better captured here.

What if we use the new verb ref here:

• getLabel returns a symbol, therefore by value.
• refState returns a reference to the state dictionary.