Skip to content

Conversation

@ticapix
Copy link

@ticapix ticapix commented Jan 1, 2025

Proposal to address linkml/linkml#2475

Without this, it's impossible to convert a Python Instance of a LinkML gen-python class to RDF without loosing language and datatype information.

@ticapix
Copy link
Author

ticapix commented Jan 2, 2025

of course, this PR is broken if we consider that only rdf:langString can have a language tag. https://www.w3.org/TR/rdf12-concepts/#section-Graph-Literal

However, it would still fix the missing datatype tag during conversion.

Any thoughts ?

@sierra-moxon
Copy link
Member

@turbomam @ptgolden - if you have a moment, can you please review this RDFlib linkml PR?

@amc-corey-cox
Copy link
Contributor

@ticapix I have moved the work for this PR from the contributor fork to the linkml-runtime branch https://github.com/linkml/linkml-runtime/pull/new/add_rdf_lang_type_support . These branches will be lifted over in the merge to the mono-repo so you won't lose you work. You will need to open a new PR against the linkml monorepo after the move. I'd prefer to get this in but if we can't please open a new PR when you resume work. Thanks!

Copy link

@ptgolden ptgolden left a comment

Choose a reason for hiding this comment

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

The core changes in yamlutils.py to serialize langstrings looks solid. The biggest change needed is how the test is constructed. I gave my suggestion on how I would handle it, but it'd probably be good to check with people who have stronger opinions on how tests in here are presented.

from jsonasobj2 import JsonObj, JsonObjTypes, as_dict, as_json, items
from rdflib import Graph, URIRef
from jsonasobj2 import JsonObj, as_json, as_dict, JsonObjTypes, items
import jsonasobj2

Choose a reason for hiding this comment

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

This import is unused

from typing import (
Any,
ClassVar,
Dict,

Choose a reason for hiding this comment

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

Since support for Python 3.8 was dropped earlier this year, these imports should be removed, and plain old dict and list should be used.

Choose a reason for hiding this comment

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

Both of these entries should be removed-- html5lib should be added to the [dependencies] section in pyproject.toml.

Choose a reason for hiding this comment

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

I'm not sure about the strategy here to define a schema inline-- if this test were merged, the definition would potentially have to be kept up to date with changes in behavior of linkml runtime. (There's already one here, as dataclasses_init_fn_with_kwargs has been deprecated).

There's already a Person schema defined in tests. Can that be reused here to define a test case? (See these lines).

It might be good to add a new test case to that file instead of adding a new file here. (Although others may have a different opinion). There's another test there that is doing something very similar to what this test is doing:

# see https://github.com/linkml/linkml/issues/429
SCHEMA_429 = INPUT_PATH / "personinfo_test_issue_429.yaml"
DATA_429 = INPUT_PATH / "example_personinfo_test_issue_429_data.yaml"
OUT_429 = OUTPUT_PATH / "example_personinfo_test_issue_429_data.ttl"

...

@pytest.fixture
def issue_429_graph():
    """Create RDF graph for issue 429 testing."""
    view = SchemaView(str(SCHEMA_429))
    container = yaml_loader.load(str(DATA_429), target_class=Container_429)
    rdflib_dumper.dump(container, schemaview=view, to_file=str(OUT_429))
    g = Graph()
    g.parse(str(OUT_429), format="ttl")
    return g

@amc-corey-cox
Copy link
Contributor

@ticapix I have moved the work on this PR into the repo branch https://github.com/linkml/linkml-runtime/tree/add_rdf_lang_type_support . 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.

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.

4 participants