Raise an error when two classes have the same ambiguous attribute

[vincentkelleher]

Closes linkml/linkml#2403

[cmungall]

Can we extend the test to also make sure that no premature exceptions are raised when using schemaview to cycle through all slots and do a lookup (or to ensure that randomAttribute is not returned on get_slots without attributes=True)

[sneakers-the-rat]

in my opinion the problem is the ambiguity of get_slot returning attributes at all, and there is actually nothing wrong with the schema in this case - both attributes can be defined within the context of independent classes without expectation that they represent the same thing. i think i said this in the other issue but i think there are 3 dispositions for an attribute

• attribute defined only within current class (no parents): the current case - the attribute is only defined within the context of the class and does not have independent meaning otherwise
• attribute defined in a subclass and a parent class: the attribute is only defined within the class lineage, and Subclass.attribute has an implicit is_a relationship with ParentClass.attribute
• attribute explicitly inherits from a slot via an explicit is_a and extends it: attribute defined within a class is independent with an inheritance relationship to the slot, which does have independent meaning outside the class.

So I think the error message is misleading, or at least not how linkml should behave (in my opinion) - imo it should say something more like "Multiple attributes with the same name found in classes: {list of class names where attr is defined}. Access the attribute from the class directly using induced_slot(slot_name, class_name)"

[sneakers-the-rat]

would be great to start making more specific exception types in these packages - super useful for downstream programming. so idk like

class AmbiguityError(ValueError):
    pass

class AmbiguousNameError(AmbiguityError):
    pass

and then one would both catch ValueError's as well as having a more specific catch for that type of exception if someone wanted to (rather than needing to catch ValueError and then check the message text to know what kind of ValueError was raised)

[vincentkelleher]

Thank you for your feedback 🙏

If I try to summarize everything before coding a solution:

1. @cmungall you are asking me to add a test where I would look for an actual slot (which I can add to ImportantSecondClass for example) but with the attributes=False parameter to make sure my change doesn't interfere with usual slot collection ?
2. @sneakers-the-rat I kind of understand your point with certain cases of ambiguous attributes being possible as they are not supposed to be considered as slots which would be normal for the Python generator for example as attributes of a class are isolated from the outside world. The issue is that we will have two different behaviors in generators such as the Python/Java/... generators and the JSON-LD Context/SHACL/... generators. Maybe we should consider having separate methods for slots and attributes but that has massive impacts in existing code 🤔
3. @sneakers-the-rat I totally agree with more specific exceptions, I just didn't want to force my coding habits on others 😇

[sneakers-the-rat]

The issue is that we will have two different behaviors in generators such as the Python/Java/... generators and the JSON-LD Context/SHACL/... generators.

Again writing from my opinion here and dont mean to hold this up - imo all the RDF generators should give URIs like {prefix}{class_name}.{attribute_name} to attributes and then use RDFs/OWL terms to indicate relationships, so e.g. if you had two example attrs and a slot, they would be like

http://example.com/ClassA.example
http://example.com/ClassB.example
http://example.com/example

My comment here was just about the error message wording tho - it's a little unclear about what the problem is and what needs to be done: so it could say something like directed at a schema author "these attributes will be ambiguous in RDF generators and you may need to rename them or restructure the schema" or directed at a generator developer/direct user of the schema "to access these attributes use induced_slot"

[vincentkelleher]

@sneakers-the-rat I think I got your point 👍

In that case we can keep the fact that it throws an exception when ambiguous attributes are found except I will write a clearer error message to push developers/ontologists to either fix the schema or use another method.

If ever your attribute prefixing solution comes to fruition, we can remove this exception altogether.

Is that okay for you ?

[vincentkelleher]

@cmungall @sneakers-the-rat I've just updated the pull-request with your suggestions 😇

[sierra-moxon]

@vincentkelleher - I don't have permission to resolve conflicts on this PR; can you check into it?

[amc-corey-cox]

@vincentkelleher I have moved the work on this PR into the repo branch github.com/linkml/linkml-runtime/tree/raise_error_for_ambiguous_attributes. If you would like to continue this work please re-open the PR on the mono-repo after the merge from that branch. We'll lift it over for you so it should take less effort. This branch will be moved over to the linkml repo with a similar name.

[vincentkelleher]

Hi @amc-corey-cox,

Thanks for notifying me 👍

I won't be working on this PR as I'm not part of Gaia-X anymore which needed this change implemented.
I sadly don't have any time to allocate to LinkML right now.