No clear default for `inlined_as_list`

[Silvanoc]

Describe the bug

No default is neither documented nor implemented for inlined_as_list. In the end it gets the default Python value None. The robust conditional <value> is not True will make it equivalent to False, but the not so robust <value> == False would make that None equivalent to True 🤯

Neither the schema loader nor the viewer provide such a default.

Since the documentation says that "inlining as a dictionary [...] is only possible if the range of the slot is a class that has an identifier slot", we should:

1. Specify that the default depends on the attributes of the range class.
2. Fix the implementation so that inlined_as_list gets a default value.
   • If the class does not have an identifier, then inlined: True and inlined_as_list: True.
   • The question is what happens if inlined: True (explicitly set), it has an identifier and no value has been specified for inlined_as_list. Should we have by default set inlined_as_list: False whenever the range classes have an identifier?

Version of LinkML you are using v1.9.2

Please provide a schema (and if applicable, a data file) that replicates the issue

[alexbrdn]

If I am allowed to voice an opinion, I'd say that the default for inlined_as_list should always be True, as that's the safer option. One can assume that a list of objects always has clear semantics. With dictionaries, there's always the question of what semantics, if any, does the key have.

[tfliss]

The question is what happens if inlined: True (explicitly set), it has an identifier and no value has been specified for inlined_as_list. Should we have by default set inlined_as_list: False whenever the range classes have an identifier?

My understanding is when id is present, inlined==True, inlined_as_list==None, then multivalued needs to be checked to distinguish between inlining as a list of instance structs and inlining as a single valued object struct.

Below is the table I came up for the Pandera generator to pass the inlining compliance tests that is relevant to this issue. The calculation of the form key addresses the defaults. Both inlined and inlined_as_list can be treated as None defaults to false for the lookup. @Silvanoc Thanks, I realized based on this I can remove the commented line in my PR.

Note that this calculates derived decision flags rather than then driving the generator off the inlined_as_list flag. It avoids confusion when interpreting the precedence of the various flags, given that inlined_as_list takes precedence over inlined, but not multivalued (need to confirm). 'precedence' might not be the right term, this table was the best way I could find to summarize the documentation and tests.

    internal_inlined_form_key = ((slot.inlined is True), (slot.inlined_as_list is True), is_multivalued)
    INTERNAL_INLINED_FORM = {
        (False, False, False): FORM_FOREIGN_KEY,
        (False, False, True): FORM_MULTIVALUED_FOREIGN_KEY,
        (False, True, False): FORM_INLINED_LIST_DICT,
        (False, True, True): FORM_INLINED_LIST_DICT,
        (True, False, False): FORM_INLINED_DICT,
        (True, False, True): FORM_INLINED_COLLECTION_DICT,
        (True, None, True): FORM_INLINED_DICT,   # <-- no longer needed b/c the lookup uses "if inlined_as_list is True"
        (True, True, False): FORM_INLINED_DICT,    # <-- will need to double-check this line against the inlined compliance test.
        (True, True, True): FORM_INLINED_LIST_DICT,
    }
    internal_inlined_form = self.INTERNAL_INLINED_FORM.get(
        internal_inlined_form_key, SlotGeneratorMixinBase.FORM_ERROR
    )

If the class does not have an identifier, then inlined: True and inlined_as_list: True.

This may also need to account for multivalued taking precedence when inlined_as_list is unspecified/None

[Silvanoc]

Let me try to document all combinations, clearly documented/specified values and compatibilities and unspecified ones.

Index	Range class has identifier	Specified inlined	Specified inlined_as_list		inlined value	inlined_as_list value	Comment
1	True	True	True		True	True	✅ Class with identifier inlined as list
2	True	True	False		True	False	✅ Class with identifier inlined as dictionary
3	True	True	None		True	True❓	❓ No default specified on how to inline classes with identifier, any needed?
4	True	False	True		❗	❗	🚨 Incompatible values, raise error! No inlining format where no inlining!
5	True	False	False		❗	❗	🚨 Incompatible values, raise error! No inlining format where no inlining!
6	True	False	None		False	None	✅ References to class with identifier
7	True	None	True		True❓	True❓	⚠️ Does the specification of an inlining format imply inlining? Should inlined be automatically become True? To be clarified
8	True	None	False		True❓	False❓	⚠️ Does the specification of an inlining format imply inlining? Should inlined be automatically become True? To be clarified
9	True	None	None		False❓	None❓	❓ Any default needed? Any possible combination allowed? To be clarified
10	False	True	True		True	True	✅ Class without identifier inlined as list
11	False	True	False		❗	❗	🚨 Incompatible values, raise error! No inlining as dictionary possible for classes without identifier!
12	False	True	None		True	True	👀 A class without identifier can only be inlined as list, automatically set it!
13	False	False	True		❓	❓	🚨 Incompatible values, raise error! Inlining required for classes without identifier!
14	False	False	False		❓	❓	🚨 Incompatible values, raise error! Inlining required for classes without identifier!
15	False	False	None		❓	❓	🚨 Incompatible values, raise error! Inlining required for classes without identifier!
16	False	None	True		True	True	👀 Since class without identifer, it MUST be inlined, set it automatically. inlined_as_list: True is fine.
17	False	None	False		❓	❓	🚨 Incompatible values, raise error! Classes without identifier MUST be inlined as list!
18	False	None	None		True	True	👀 Since class without identifer, it MUST be inlined as list, set them automatically.

[Silvanoc]

If I am allowed to voice an opinion, I'd say that the default for inlined_as_list should always be True, as that's the safer option. One can assume that a list of objects always has clear semantics. With dictionaries, there's always the question of what semantics, if any, does the key have.

@alexbrdn according that comment, you would expect in row 3 to get inlined_as_list: True automatically set. Any opinion on the others?

[Silvanoc]

My understanding is when id is present, inlined==True, inlined_as_list==None, then multivalued needs to be checked to distinguish between inlining as a list of instance structs and inlining as a single valued object struct.

@tfliss I have to admit, that up-to-now I've only considered inlining for multivalued attributes 🤷 Therefore the previous table only covers multivalued attributes. A similar table might be needed for single-valued attributes... At least for multivalued attributes you seem to agree with @alexbrdn on how to handle row 3.

Below is the table I came up for the Pandera generator to pass the inlining compliance tests that is relevant to this issue. The calculation of the form key addresses the defaults. Both inlined and inlined_as_list can be treated as None defaults to false for the lookup. @Silvanoc Thanks, I realized based on this I can remove the commented line in my PR.

Note that this calculates derived decision flags rather than then driving the generator off the inlined_as_list flag. It avoids confusion when interpreting the precedence of the various flags, given that inlined_as_list takes precedence over inlined, but not multivalued (need to confirm). 'precedence' might not be the right term, this table was the best way I could find to summarize the documentation and tests.

IMO the value None for inlined and inlined_as_list cannot be assumed to mean False, but rather "unspecified". The question is if "unspecified" means "free to choose something senseful". If that is the case, then rows 7, 8, 16 and 17 should IMO set inlined: True to make any sense. With that interpretation there is no need to have some values overriding others, either they are incompatible (and raise an error), or one value is unspecified. In the latest case, there are cases where its value can be inferred from others (e.g. inlining: True when inlining_as_list: True/False.

[alexbrdn]

@Silvanoc, here's my opinion on the question points:

• line 3: inlined_as_list=True
• line 7: True, True
• line 8: True, False
• line 9: no inlining, just a list of identifiers
• lines 13, 14, 15, 17: raise error

[Silvanoc]

@Silvanoc, here's my opinion on the question points:

* line 3: inlined_as_list=True

* line 7: True, True

* line 8: True, False

* line 9: no inlining, just a list of identifiers

* lines 13, 14, 15, 17: raise error

@alexbrdn since your expectations correspond mine, I have added them as proposal. line 9 "no inlining" would mean inlined: False.

[Silvanoc]

Attention! ⚠️

The Python generator is applying some defaults that might become inconsistent with the proposal in the table! (see for example here). Changes those implicit defaults applied on the Python generator might not be that easy due to the wide user base of that generator...

Additionally, the tests are showing that the Python generator is applying a different default from what @alexbrdn is proposing. See for example, how the function _normalize_inlined_as_dict is being used for annotations when the schema is not specifying any value for inlined_as_list.

[alexbrdn]

I'll happily admit that my suggestion for table line 3 is just what seems to make sense to me. If some existing tests disagree - let them win. I'm mostly interested in line 18 - that one is a must, IMO.

[Silvanoc]

There are some compliance tests checking the different combinations of "inlining"... But they don't consider the value None, that might appear as long as the SchemaLoader doesn't take care of defaulting None to True or False. Additionally illegal combinations are not being checked either.

[dalito]

I was also thinking (but had no time to study) if we could exclude None as additional allowed value to reduce complexity.