Use custom code for RDF (de)serialisation

[gouttegd]

This PR implements option ”A” mentioned in #628. That is, it replaces the LinkML-based code for serialising/deserialising to/from RDF by a completely custom code that implements the new SSSOM/RDF spec exactly as we want it.

All the new code is contained within the new sssom.rdf_internal module, which as the name implies is not really supposed to be used outside of SSSOM-Py (though of course people can still do whatever they want!). The public interface to read from RDF is still the from_sssom_rdf method in sssom.parsers, and the public interface to write to RDF is still the to_rdf_graph method in sssom.writers. Both methods are rewritten to use the MappingSetRDFConverter class of the sssom.rdf_internal module, instead of methods from the LinkML runtime.

closes #628

[cthoyt]

@gouttegd there are a few parts to the control flow I think could be improved, do you mind If I push some updates directly to your branch? otherwise I'll PR it to your branch

[cthoyt]

I consider passthrough=True is a bandage for end-users of curies who have very poorly formatted data and Just Want It To Work™. I think we should more explicitly handle what happens when the value either 1) isn't a valid URI or 2) can't be compressed for some reason based on the converter. I would prefer to throw exceptions in both scenarios, which could get caught by code that calls this and decide whether to be strict or not about swallowing them or bubbling them up

[gouttegd]

I principle I agree, and in fact I initially did not use passthrough=True, but then this caused the test suite to fail, because some of the very sets that we use in our own test suite apparently contain poorly formatted data – which was accepted without errors or warnings by the current RDF parser and writer.

I think the new code should be, at least initially, as tolerant as the old one. If the new code refuses a set that the old one let pass, I’d consider that a bug of the new code (and this is why I had to use passthrough=True).

I wouldn’t be opposed to making the code more strict, but that would belong to another PR IMHO.

[gouttegd]

I wouldn’t be opposed to making the code more strict, but that would belong to another PR IMHO.

To be clear: I would in fact be very much in favour of making the code more strict. But:

• This would break existing code or sets and must therefore be considered carefully (I know SSSOM-Py is still in 0.x and therefore we are allowed to break things as much as we want; doesn’t mean we should do it lightly).
• This would require changes to more than just the RDF parser/writer.

Two reasons why this would belong to a distinct, dedicated PR.

[matentzn]

I agree, better handle in dedicated PR

[cthoyt]

this can be a module-level variable

[gouttegd]

I like to avoid module-level variable if possible. Such a variable is initialised as soon as the module is loaded.

Suppose you are doing this:

from sssom.parsers import parse_sssom_table
from sssom.writers import write_json

msdf = parse_sssom_table("myset.sssom.tsv")
write_json(msdf, "myset.sssom.json")

Since you are loading the sssom.parsers module, you are also loading the sssom.rdf_internal module, and initialising any module-level variables in that module – even though you never even intended to do anything related to RDF.

[cthoyt]

I don't think that it's a legitimate concern given it's a small dictionary with elements that are already interpreted on import time anyway, but if you're insistent, then you can always wrap in a functionan call like:

from functools import lru_cache

@lru_cache
def _get_my_data():
     return {}

The point being that this class could just be the create function all by itself

[gouttegd]

do you mind If I push some updates directly to your branch?

Err, as a matter of fact I do mind. I’d like to see your proposed updates before they end up on my branch. Sorry, but distributed revision control systems like Git were invented for a reason after all. :)

[cthoyt]

I think this function should either be module level, or a classmethod that constructs a MappingSetRDFConverter() inside it

[gouttegd]

This would make the MappingSetRDFConverter class a “one-shot“ class: you create an instance to convert one graph into a MSDF (or one MSDF into a graph), and then it is spent, if you want to convert another object you must create another instance.

Yes, for the sssom.parsers.sssom_from_rdf and sssom.writers.to_rdf_graph functions within SSSOM-Py this is all we need, but I’d rather keep the MappingSetRDFConverter class reusable for multiple conversions for those who might need that.

No objection to add a module-level msdf_from_rdf function that does nothing but create an instance and immediately call the msdf_from_rdf method on it (that’s basically what sssom.parsers.sssom_from_rdf is already doing), as a convenience for those who don’t want to bother creating an instance themselves.

[matentzn]

Sorry it took so long to look at this.

I have reviewed mostly everything apart from src/sssom/rdf_internal.py (I scanned it from an architectural viewpoint). This all looks clean, well designed and performant!

[matentzn]

I agree, better handle in dedicated PR

[matentzn]

I assume you want a sep. PR for this. IN the meantime, maybe logging.warning()?

[gouttegd]

I assume you want a sep. PR for this

Yes.

First because extension slots are completely unsupported throughout SSSOM-Py anyway, so not supporting them in RDF import/export is not a big deal.

Second (and most importantly), since extension slots are currently completely unsupported, I think we need to discuss first about how we are going to support them in the rest of SSSOM-Py – in particular, how they are going to be represented within a MSDF.

For now, I am assuming that extension slots will be represented in the most direct way possible:

• a foo extension slot at the mapping set level will be represented as a foo key in the metadata part of the MSDF;
• a bar extension slot at the mapping level will be represented as a bar column in the data frame part of the MSDF.

I don’t think it needs to be more complicated than that, and it is consistent with the way the other (non-extension) slots are represented.

Still, this needs discussion, if only because this will require updating (or, in fact, completely rewriting from scratch) the existing parsers and writers (apart from the RDF ones) so that they can support extensions – for now, they cannot do that since they rely heavily on the LinkML’s dumpers and loaders, which do not allow the presence of unpexpected slots.

[cthoyt]

I've made a PR onto this PR gouttegd#3 that improves the control flow

[gouttegd]

I've made a PR onto this PR gouttegd#3 that improves the control flow

Commenting here to keep the discussion in a single place.

Your PR “improves” the control flow by making the MappingSetRDFConverter class in effect not reusable (unless for sets that all use the same prefix map), thereby negating one of the main interests of that class.

The class is designed so that you can do something like this:

from rdflib import Graph
from sssom.rdf_internal import MappingSetRDFConverter

# Creating a converter object once and for all
converter = MappingSetRDFConverter()

# Extracting a set from a graph
msdf1 = converter.msdf_from_rdf(Graph().parse("my-set-1.ttl"))

# Extracting another set from another graph
msdf2 = converter.msdf_from_rdf(Graph().parse("my-set-2.ttl"))

# Extracting yet another set from yet another graph
msdf3 = converter.msdf_from_rdf(Graph().parse("my-set-3.ttl"))

# and so on...

and likewise for RDF export (msdf_to_rdf).

The “improved” version requires instead to create a whole new converter for every single set to extract:

g1 = Graph().parse("my-set-1.ttl")
msdf1 = MappingSetRDFConverter(Converter.from_rdflib(g1)).msdf_from_rdf(g1)

g2 = Graph().parse("my-set-2.ttl")
msdf2 = MappingSetRDFConverter(Converter.from_rdflib(g2)).msdf_from_rdf(g2)

# and so on...

And then there is no longer any real point in having a MappingSetRDFConverter class to begin with...

At the very least, such a design would absolutely need a clear warning in the documentation of the MappingSetRDFConverter constructor that any new instance can be used to (de)serialize several mapping sets if and only if all the sets share the same prefix map.

[matentzn]

@gouttegd from your perspective this is ready to merge right?

@cthoyt

• re control flow change you suggested in Update control flow gouttegd/sssom-py#3, since they are not in line with the orginal vision behind this PR here, you will have to file an issue (sorry, I know this sounds like unnecessary extra work) that explains why you believe its needed and the current design is not enough
• any other observations? I would like to merge asap

[gouttegd]

from your perspective this is ready to merge right?

Assuming there is agreement on the overall design, yes.

If there is no agreement on the design: I personally think supporting the RDF serialisation as described in the spec is more important than the exact design used to achieve that aim. I am not wed to my initial design and could consider changing it, but not for the design proposed in gouttegd#3, at least not in its current state.

[matentzn]

I have considered the design again; its unfortunate we lost @cthoyt on this one, but I agree with keeping the serialiser class reusable across transformations rather than tying the constructor to a specific converter. I am not a entirely shure about the ConverterProvider, but then again I am the worst coder among us, so my judgment counts for little.

THANK YOU @gouttegd for pushing this, I would like to move forward with this whenever you are ready!