Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] fixing context generator for any_of with range #1898

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

djarecka
Copy link
Contributor

fixies #1897

I've tried to fix the context generator, but I'm actually not sure if my approach is correct. Should slot_def["@type"] = "@id" be added if any of the ranges from any_of are in self.schema.classes?

Copy link

codecov bot commented Feb 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b6ca53) 80.13% compared to head (dd9567d) 80.13%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1898   +/-   ##
=======================================
  Coverage   80.13%   80.13%           
=======================================
  Files         100      100           
  Lines       11476    11504   +28     
  Branches     2956     2904   -52     
=======================================
+ Hits         9196     9219   +23     
+ Misses       1734     1732    -2     
- Partials      546      553    +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -150,7 +150,8 @@ def visit_slot(self, aliased_slot_name: str, slot: SlotDefinition) -> None:
else:
slot_def = {}
if not slot.usage_slot_name:
if slot.range in self.schema.classes:
any_of_ranges = [any_of_el["range"] for any_of_el in slot.any_of]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this definitely work? All metamodel elements should be accessed as objects, e.g any_of_el.range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it worked, but I will change it to any_of_el.range

@cmungall
Copy link
Member

As a next step for this PR, I recommend taking a look at:

def test_slot_any_of(framework, data_name, value, is_valid, use_any_type, use_default_range):
"""
Tests behavior of any_of at the slot level.
This test creates a test schema with a slot S1 whose values
are either an integer or an inlined instance of class D.
any_of is a special case of boolean metaslot,
as it cleanly maps to many language constructs (e.g. Union in Python),
and it is often possible to normalize complex combinations to any_of form.
TODO: resolve issues around combining any_of with default ranges or
asserted ranges, see https://github.com/linkml/linkml/issues/1483
:param framework: generator to test
:param data_name: unique identifier for the test data instance
:param value: data value to use in the instance
:param is_valid: whether the test instance is expected to be valid
:param use_any_type: if True, assert additional range using linkml:Any
:param use_default_range: if True, the default range will be included in addition to any_of.
:return:
"""
expected_json_schema = {"s1": {"anyOf": [{"$ref": "#/$defs/D"}, {"type": "integer"}]}}
if use_default_range and not use_any_type:
# TODO: undesired behavior, see https://github.com/linkml/linkml/issues/1483
expected_json_schema["s1"]["type"] = "string"
if use_any_type:
# TODO: undesired behavior, see https://github.com/linkml/linkml/issues/1483
expected_json_schema["s1"]["$ref"] = "#/$defs/Any"
classes = {
CLASS_D: {
"attributes": {
SLOT_S2: {
"range": "string",
},
},
},
CLASS_C: {
"attributes": {
SLOT_S1: {
"any_of": [
{
"range": CLASS_D,
},
{"range": "integer"},
],
"_mappings": {
PYDANTIC: f"{SLOT_S1}: Optional[Union[D, int]]",
JSON_SCHEMA: expected_json_schema,
},
},
},
},
}
if use_default_range:
default_range = "string"
else:
default_range = None
if use_any_type:
classes[CLASS_ANY] = {
"class_uri": "linkml:Any",
}
classes[CLASS_C]["attributes"][SLOT_S1]["range"] = CLASS_ANY

and adding something here:

"_mappings": {
PYDANTIC: f"{SLOT_S1}: Optional[Union[D, int]]",
JSON_SCHEMA: expected_json_schema,

to test the jsonld context output

But let us know if you have issues or don't have time for that and we can take on the PR, thanks!

@djarecka
Copy link
Contributor Author

djarecka commented Feb 13, 2024

I've added a check for JOSNOLD_CONTEXT. I believe the result should be the same in any combination of use_default_range and use_any_type, am I right?

FYI: I must say that it was a bit confusing to me when pytest was failing only for one combination of data_name,value,is_valid, it took me some time to realize that the checks from _mappings are only checked for one combination and for the following tests this part is skipped

@djarecka
Copy link
Contributor Author

@cmungall - I've been using this branch for one of my project, and I have problems when I have any_of with range either str or some class, e.g.:

    any_of:
    - range: string
    - range: langString

In this specific case, langString is langString: class_uri: rdf:langString (but I believe it could be anything).

If in the context I have @type: @id and the data is a simple string, I got an error: Message: Node <http://localhost:8000/examples/activities/items/item1> does not conform to one or more shapes in [ sh:datatype xsd:string ] , [ sh:datatype rdf:langString ]

Not sure if this is a common case to have range either string or class, and if there is a way that this PR should be changed?

@cmungall cmungall merged commit c5b6b8c into linkml:main Feb 15, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants