FBC3 KeyValuePair annotations + metaid

[pascalaldo]

Hi,

I recently picked up the implementation of SBML L3 FBC v3 in cobrapy (see opencobra/cobrapy#1440). All of the specification implemented so far was nicely available in libsbml, so thank you for that! The only issue I ran into is with KeyValuePair objects:
The issue is with fbc-21501 and fbc-21502 of the spec, which state that KeyValuePair can have sboTerm and metaid attributes, and an annotation subobject. This is indeed implemented in the libsbml code by having KeyValuePair inherit from SBase. However, the code to convert this into XML is not present. I think the issue lies within the functions at FbcSBasePlugin::writeKeyValuePairsAnnotation and KeyValuePair::toXML(). The latter indeed seems to lack any call to export SBase attributes etc., whilst the first lacks the ability to write the attributes metaid and sboTerm, as per fbc-21509 (we're not using fbc-21509 in cobrapy, so I do not really care about that, just noticed it).

The following code illustrates the issue:

import libsbml
from logging import getLogger
logger = getLogger(__name__)

def check(value: int, message: str) -> bool:
    """Check the libsbml return value and prints message if something happened.

    If 'value' is None, prints an error message constructed using
      'message' and then exits with status code 1. If 'value' is an integer,
      it assumes it is a libSBML return status code. If the code value is
      LIBSBML_OPERATION_SUCCESS, returns without further action; if it is not,
      prints an error message constructed using 'message' along with text from
      libSBML explaining the meaning of the code, and exits with status code 1.
    """
    valid = True
    if value is None:
        logger.error(f"Error: LibSBML returned a null value trying to <{message}>.")
        valid = False
    elif isinstance(value, int):
        if value != libsbml.LIBSBML_OPERATION_SUCCESS:
            logger.error(f"Error encountered trying to '{message}'.")
            logger.error(
                f"LibSBML returned error code {str(value)}: "
                f"{libsbml.OperationReturnValue_toString(value).strip()}"
            )
            valid = False

    return valid


TEST_SBML = """<?xml version="1.0" encoding="UTF-8"?>
<sbml xmlns="http://www.sbml.org/sbml/level3/version2/core" xmlns:fbc="http://www.sbml.org/sbml/level3/version1/fbc/version3" level="3" version="2" fbc:required="false">
<model>
    <annotation>
        <listOfKeyValuePairs xmlns="http://sbml.org/fbc/keyvaluepair">
            <keyValuePair metaid="meta_kvp_1" id="kvp_1" key="cobra_test" value="true">
                <annotation>
                    <rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:bqmodel="http://biomodels.net/model-qualifiers/" xmlns:bqbiol="http://biomodels.net/biology-qualifiers/">
                        <rdf:Description rdf:about="#meta_kvp_1">
                            <bqbiol:is>
                                <rdf:Bag>
                                    <rdf:li rdf:resource="http://identifiers.org/bigg.model/e_coli_core" />
                                </rdf:Bag>
                            </bqbiol:is>
                        </rdf:Description>
                    </rdf:RDF>
                </annotation>
            </keyValuePair>
        </listOfKeyValuePairs>
    </annotation>
</model>
</sbml>"""


def test_read():
    print("Reading nested metadata example.")
    doc: libsbml.SBMLDocument = libsbml.readSBMLFromString(TEST_SBML)

    model: "libsbml.Model" = doc.getModel()
    model_fbc: "libsbml.FbcModelPlugin" = model.getPlugin("fbc")

    for kvp in model_fbc.getListOfKeyValuePairs():
        cvterms = kvp.getCVTerms()
        for cv in cvterms:
            print(f"KeyValuePair->CVTerm->Resource[0]: {cv.getResourceURI(0)}")


def test_write():
    print("# Writing nested metadata example.")
    # create document with new fbc version 3 namespace
    sbmlns: libsbml.SBMLNamespaces = libsbml.SBMLNamespaces(3, 2)
    sbmlns.addPkgNamespace("fbc", 3)
    doc: libsbml.SBMLDocument = libsbml.SBMLDocument(sbmlns)
    doc_fbc: libsbml.FbcSBMLDocumentPlugin = doc.getPlugin("fbc")
    doc_fbc.setRequired(False)

    model: libsbml.Model = doc.createModel()
    model_fbc: libsbml.FbcModelPlugin = model.getPlugin("fbc")
    model.setMetaId("meta_model_e_coli_core")

    kvp: libsbml.KeyValuePair = model_fbc.createKeyValuePair()
    kvp.setKey("cobra_test")
    kvp.setValue("true")
    kvp.setId("kvp_1")
    check(kvp.setMetaId("meta_kvp_1"), "Setting metaid")

    cv: "libsbml.CVTerm" = libsbml.CVTerm()
    check(cv.setQualifierType(libsbml.BIOLOGICAL_QUALIFIER), "Set qualifier type")
    check(cv.setBiologicalQualifierType(libsbml.BQB_IS), "Set qualifier")
    check(
        cv.addResource("http://identifiers.org/bigg.model/e_coli_core"), "Add resource"
    )

    # check(model.addCVTerm(cv, newBag=True), "Adding CVTerm to model")
    check(kvp.addCVTerm(cv, newBag=True), "Adding cvterm")
    cvterms_check = kvp.getCVTerms()
    for cv_check in cvterms_check:
        print(f"KeyValuePair->CVTerm->Resource[0]: {cv_check.getResourceURI(0)}")

    sbml_str: str = libsbml.writeSBMLToString(doc)
    print("#####")
    print(sbml_str)
    print("#####")


if __name__ == "__main__":
    test_read()
    test_write()

The output of this is:

Reading nested metadata example.
KeyValuePair->CVTerm->Resource[0]: http://identifiers.org/bigg.model/e_coli_core
# Writing nested metadata example.
KeyValuePair->CVTerm->Resource[0]: http://identifiers.org/bigg.model/e_coli_core
#####
<?xml version="1.0" encoding="UTF-8"?>
<sbml xmlns="http://www.sbml.org/sbml/level3/version2/core" xmlns:fbc="http://www.sbml.org/sbml/level3/version1/fbc/version3" level="3" version="2" fbc:required="false">
  <model metaid="meta_model_e_coli_core">
    <annotation>
      <listOfKeyValuePairs xmlns="http://sbml.org/fbc/keyvaluepair">
        <keyValuePair id="kvp_1" key="cobra_test" value="true"/>
      </listOfKeyValuePairs>
    </annotation>
  </model>
</sbml>

#####

This shows that setting annotations (cvterms) to a KeyValuePair works fine and reading it from an SBML file also works. It's just writing them to xml that is missing.
Hope you can help with this, let me know if you need more info.

[fbergmann]

Thank you for raising this issue, we did not anticipate that someone would annotate the key value pair itself with cvterms. Since the key value pair is the annotation to a specific SBase object, we assumed that CVTerms would be on that SBase object, rather than the KVP. We'll look into that.

[pascalaldo]

Thank you!
And yeah, to be honest, it would be quite a niche feature to use. It's just that it follows from inheriting from SBase, also in our cobrapy code.

[bgoli]

Thanks, interesting issue. @fbergmann we should discuss this at some point.

[fbergmann]

with the commit from yesterday the output of the code above is now:

Reading nested metadata example.
KeyValuePair->CVTerm->Resource[0]: http://identifiers.org/bigg.model/e_coli_core
# Writing nested metadata example.
KeyValuePair->CVTerm->Resource[0]: http://identifiers.org/bigg.model/e_coli_core
#####
<?xml version="1.0" encoding="UTF-8"?>
<sbml xmlns="http://www.sbml.org/sbml/level3/version2/core" xmlns:fbc="http://www.sbml.org/sbml/level3/version1/fbc/version3" level="3" version="2" fbc:required="false">
  <model metaid="meta_model_e_coli_core">
    <annotation>
      <listOfKeyValuePairs xmlns="http://sbml.org/fbc/keyvaluepair">
        <keyValuePair id="kvp_1" key="cobra_test" value="true">
          <annotation>
            <rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:dcterms="http://purl.org/dc/terms/" xmlns:vCard="http://www.w3.org/2001/vcard-rdf/3.0#" xmlns:vCard4="http://www.w3.org/2006/vcard/ns#" xmlns:bqbiol="http://biomodels.net/biology-qualifiers/" xmlns:bqmodel="http://biomodels.net/model-qualifiers/">
              <rdf:Description rdf:about="#meta_kvp_1">
                <bqbiol:is>
                  <rdf:Bag>
                    <rdf:li rdf:resource="http://identifiers.org/bigg.model/e_coli_core"/>
                  </rdf:Bag>
                </bqbiol:is>
              </rdf:Description>
            </rdf:RDF>
          </annotation>
        </keyValuePair>
      </listOfKeyValuePairs>
    </annotation>
  </model>
</sbml>

while I still recommend not to annotate the KVP directly (since after all the model element is the e coli core model, not the kvp), the current behaviour of libsbml needs to change.

[pascalaldo]

Thank you for looking into it and implementing it so quickly. I think the only missing thing is that the metaid is not written to the keyValuePair tag, while the annotation has to refer to it with <rdf:Description rdf:about="#meta_kvp_1">. I quickly tested reading this output, and it seems to work fine, but it should probably also output the metaid for the keyValuePair tag, right?

And about the discussion of whether to have this feature at all: Agreed that it should probably not be the recommended way of annotating anything, and, indeed, in the test code, it does not make sense.

However, it is not really well-defined what can be stored in this type of annotation. So one could very well set a certain property/feature of a modelling species and annotate it with bqm_isDescribedBy->a pubmed resource to cite an article that describes this specific property of the species.

And I think part of the confusion is that (also looking at the discussion around this pull request) it is not stated in the specification that KeyValuePair inherits from SBase, but in the libsbml implementation it does. And the spec also states:

Rules for KeyValuePair object:
fbc-21501: A KeyValuePair object may have the optional SBML Level 3 Core attributes metaid and sboTerm. No other 22
attributes from the SBML Level 3 Core namespaces are permitted on a KeyValuePair. (Reference: SBML 23
Level 3 Version 1 Core, Section 3.2.) 24
fbc-21502: A KeyValuePair object may have the optional SBML Level 3 Core subobjects for notes and annotations. 25
No other elements from the SBML Level 3 Core namespaces are permitted on a KeyValuePair. (Reference: 26
SBML Level 3 Version 1 Core, Section 3.2.)

So I guess it should ideally be implemented as an object that does not inherit from SBase, but does have a lot of its properties (SId, name, sboTerm, annotation, etc.), which will lead to a lot of duplicated code.

[fbergmann]

Thank you for looking into it and implementing it so quickly. I think the only missing thing is that the metaid is not written to the keyValuePair tag, while the annotation has to refer to it with <rdf:Description rdf:about="#meta_kvp_1">. I quickly tested reading this output, and it seems to work fine, but it should probably also output the metaid for the keyValuePair tag, right?

thanks, of course you are right, I've added this to the PR. Thanks!

as for why those rules where there in libSBML to begin with, that was just since that code got automatically generated by deviser.

I don't see the need to change that internal part of the libSBML implementation with the SBase inheritance, it is useful enough. No need of all that code duplication.

Thank you again

[bgoli]

From an interoperability/design perspective having nested KVP's is not ideal. It could only possibly make sense to put a CV term on KVP exclusively in the case that there is no other model property/element to add it to. KVP's with Annotations that contain KVP's are not what we want. However, pragmatically only the top-level KVP's will be implemented so we can go with Franks implementation.

[skeating]

Or we could ask the author of the code (mea culpa) to rewrite it so that KVP is not derived from SBase and not let it have any further attributes/elements but @pascalaldo I'm guessing you have a sensible use case for actually putting annotations on the KVP

[pascalaldo]

@skeating I don't necessarily have a use case. It's just that I tried to implement the spec, which mentioned kvps having annotations is possible. Based on that and the libsbml code, I also made the cobrapy KVP equivalent inherit from the SBase equivalent. I then found the inconsistency of libsbml being able to read this, but not write when testing the code.

That being said, as illustrated in my previous comment, there can be a use case. KVPs are very general and it's not really specified what their exact usecase is, so I can imagine people using it.

But I'm fine with either this implementation or removing it from the spec and from cobrapy.

[fbergmann]

i think it is fine, it is already implemented, it could be useful. and it would involve a lot of work to change. So i'm in favor of just fixing the libSBML behavior (which is done)

[bgoli]

I suppose it's all annotation anyway so I'm okay with going with the fixed libSBML behaviour.