NeuroML · pgleeson · Nov 15, 2023 · Nov 8, 2023 · Nov 9, 2023 · Nov 10, 2023
diff --git a/tests/sbml/test_data/test_doc.sbml → examples/test_data/valid_doc.sbml b/tests/sbml/test_data/test_doc.sbml → examples/test_data/valid_doc.sbml
@@ -39,4 +39,4 @@
       </reaction>
     </listOfReactions>
   </model>
-</sbml>
+</sbml>
diff --git a/pyneuroml/pynml.py b/pyneuroml/pynml.py
@@ -126,8 +126,8 @@ def parse_arguments():
         "input_files",
         type=str,
         nargs="*",
-        metavar="<LEMS/NeuroML 2 file(s)>",
-        help="LEMS/NeuroML 2 file(s) to process",
+        metavar="<LEMS/NeuroML 2/SBML file(s)>",
+        help="LEMS/NeuroML 2/SBML file(s) to process",
     )
 
     mut_exc_opts_grp = parser.add_argument_group(
@@ -348,7 +348,12 @@ def parse_arguments():
     mut_exc_opts.add_argument(
         "-validate-sbml",
         action="store_true",
-        help=("Validate SBML file(s)"),
+        help=("Validate SBML file(s), unit consistency failure generates a warning"),
+    )
+    mut_exc_opts.add_argument(
+        "-validate-sbml-units",
+        action="store_true",
+        help=("Validate SBML file(s), unit consistency failure generates an error"),
     )
 
     return parser.parse_args()
@@ -2117,12 +2122,37 @@ def evaluate_arguments(args):
     exit_on_fail = True
 
     # Deal with the SBML validation option which doesn't call run_jneuroml
-    if args.validate_sbml:
-        from pyneuroml.sbml import validate_sbml_files
+    if args.validate_sbml or args.validate_sbml_units:
+        try:
+            from pyneuroml.sbml import validate_sbml_files
+        except Exception:
+            logger.critical("Unable to import pyneuroml.sbml")
+            sys.exit(UNKNOWN_ERR)
 
-        validate_sbml_files(" ".join(args.input_files))
+        if not len(args.input_files) >= 1:
+            logger.critical("No input files specified")
+            sys.exit(ARGUMENT_ERR)
 
-        return True
+        if args.validate_sbml_units:
+            # A failed unit consistency check generates an error
+            strict_units = True
+        else:
+            # A failed unit consistency check generates only a warning
+            strict_units = False
+
+        try:
+            result = validate_sbml_files(args.input_files, strict_units)
+        except Exception as e:
+            logger.critical(f"validate_sbml_files failed with {str(e)}")
+            sys.exit(UNKNOWN_ERR)
+
+        if result:
+            # All files validated ok (with possible warnings but no errors)
+            sys.exit(0)
+
+        # Errors of some kind were found in one or more files
+        logger.error(f"one or more SBML files failed to validate")
+        sys.exit(UNKNOWN_ERR)
 
     # These do not use the shared option where files are supplied
     # They require the file name to be specified after

diff --git a/pyneuroml/sbml/__init__.py b/pyneuroml/sbml/__init__.py
@@ -4,61 +4,91 @@
 """
 
 import os
+import errno
 import libsbml
 from libsbml import SBMLReader
 from typing import List
 
 
-def validate_sbml_files(input_files: str, units_consistency: bool = False):
+def validate_sbml_files(input_files: List[str], strict_units: bool = False) -> bool:
     """
     validate each input file using libsbml.SBMLDocument.checkConsistency
-    input_files is a space separated list of one or more filepaths
+    input_files is a list of one or more filepaths
+    strict_units converts unit consistency warnings into errors
     """
 
-    for file_name in input_files.split():
+    if not len(input_files) >= 1:
+        raise ValueError("No input files specified")
+
+    all_valid = True
+
+    for file_name in input_files:
+        # These checks are already implemented by SBMLReader
+        # But could just be logged along with the other error types rather than causing exceptions
         if not os.path.isfile(file_name):
-            raise OSError(("Could not find SBML file %s" % file_name))
+            raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), file_name)
+
+        if not os.access(file_name, os.R_OK):
+            raise IOError(f"Could not read SBML file {file_name}")
 
         try:
             reader = SBMLReader()
             doc = reader.readSBML(file_name)
         except Exception:
-            raise OSError(("SBMLReader failed to load the file %s" % file_name))
+            # usually errors are logged within the doc object rather than being thrown
+            raise IOError(f"SBMLReader failed trying to open the file {file_name}")
 
-        # set the unit checking, similar for the other settings
-        doc.setConsistencyChecks(
-            libsbml.LIBSBML_CAT_UNITS_CONSISTENCY, units_consistency
-        )
+        # Always check for unit consistency
+        doc.setConsistencyChecks(libsbml.LIBSBML_CAT_UNITS_CONSISTENCY, True)
         doc.checkConsistency()
-        # get errors/warnings
+
+        # Get errors/warnings arising from the file reading or consistency checking calls above
         n_errors: int = doc.getNumErrors()
         errors: List[libsbml.SBMLError] = list()
         warnings: List[libsbml.SBMLError] = list()
+
         for k in range(n_errors):
             error: libsbml.SBMLError = doc.getError(k)
             severity = error.getSeverity()
             if (severity == libsbml.LIBSBML_SEV_ERROR) or (
                 severity == libsbml.LIBSBML_SEV_FATAL
             ):
                 errors.append(error)
+            elif (
+                (strict_units is True)
+                # For error code definitions see
+                # https://github.com/sbmlteam/libsbml/blob/fee56c943ea39b9ac1f8491cac2fc9b3665e368f/src/sbml/SBMLError.h#L528
+                # and sbml.level-3.version-2.core.release-2.pdf page 159
+                and (error.getErrorId() >= 10500)
+                and (error.getErrorId() <= 10599)
+            ):
+                # Treat unit consistency warnings as errors
+                errors.append(error)
             else:
                 warnings.append(error)
+
         # print results
         print("-" * 80)
         print(f"{'validation error(s)':<25}: {len(errors)}")
         print(f"{'validation warning(s)':<25}: {len(warnings)}")
+
         if len(errors) > 0:
+            all_valid = False
             print("--- errors ---")
             for kerr in enumerate(errors):
                 print(f"E{kerr}: {error.getCategoryAsString()} L{error.getLine()}")
                 print(
                     f"[{error.getSeverityAsString()}] {error.getShortMessage()} | {error.getMessage()}"
                 )
+
         if len(warnings) > 0:
             print("--- warnings ---")
             for kwarn in enumerate(warnings):
                 print(f"E{kwarn}: {error.getCategoryAsString()} L{error.getLine()}")
                 print(
                     f"[{error.getSeverityAsString()}] {error.getShortMessage()} | {error.getMessage()}"
                 )
+
         print("-" * 80)
+
+    return all_valid
diff --git a/test-ghactions.sh b/test-ghactions.sh
@@ -70,6 +70,13 @@ pynml LEMS_NML2_Ex9_FN.xml -spineml
 pynml LEMS_NML2_Ex9_FN.xml -sbml
 
 
+echo
+echo "################################################"
+echo "##   Simple SBML validation example"
+
+pynml -validate-sbml test_data/valid_doc.sbml
+pynml -validate-sbml-units test_data/valid_doc.sbml
+
 
 echo
 echo "################################################"

diff --git a/tests/sbml/test_data/inconsistent_units_doc.sbml b/tests/sbml/test_data/inconsistent_units_doc.sbml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<sbml xmlns="http://www.sbml.org/sbml/level3/version1/core" level="3" version="1">
+  <model substanceUnits="mole" timeUnits="second" extentUnits="mole">
+    <listOfUnitDefinitions>
+      <unitDefinition id="per_second">
+        <listOfUnits>
+          <unit kind="second" exponent="-1" scale="0" multiplier="1"/>
+        </listOfUnits>
+      </unitDefinition>
+    </listOfUnitDefinitions>
+    <listOfCompartments>
+      <compartment id="c1" spatialDimensions="3" size="1" units="litre" constant="true"/>
+    </listOfCompartments>
+    <listOfSpecies>
+      <species id="S1" compartment="c1" initialAmount="5" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+      <species id="S2" compartment="c1" initialAmount="0" substanceUnits="gram" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+    </listOfSpecies>
+    <listOfParameters>
+      <parameter id="k" value="1" units="per_second" constant="true"/>
+    </listOfParameters>
+    <listOfReactions>
+      <reaction id="r1" reversible="false" fast="false">
+        <listOfReactants>
+          <speciesReference species="S1" constant="true"/>
+        </listOfReactants>
+        <listOfProducts>
+          <speciesReference species="S2" constant="true"/>
+        </listOfProducts>
+        <kineticLaw>
+          <math xmlns="http://www.w3.org/1998/Math/MathML">
+            <apply>
+              <times/>
+              <ci> k </ci>
+              <ci> S1 </ci>
+              <ci> S2 </ci>
+            </apply>
+          </math>
+        </kineticLaw>
+      </reaction>
+    </listOfReactions>
+  </model>
+</sbml>
diff --git a/tests/sbml/test_data/invalid_doc00.sbml b/tests/sbml/test_data/invalid_doc00.sbml
@@ -0,0 +1,2 @@
+10 PRINT "HELLO"
+20 GOTO 10
diff --git a/tests/sbml/test_data/invalid_doc01.sbml b/tests/sbml/test_data/invalid_doc01.sbml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<sbml xmlns="http://www.sbml.org/sbml/level3/version1/core" level="3" version="1">
+  <model substanceUnits="molerat" timeUnits="second" extentUnits="mole">
+    <listOfUnitDefinitions>
+      <unitDefinition id="per_second">
+        <listOfUnits>
+          <unit kind="second" exponent="-1" scale="0" multiplier="1"/>
+        </listOfUnits>
+      </unitDefinition>
+    </listOfUnitDefinitions>
+    <listOfCompartments>
+      <compartment id="c1" spatialDimensions="3" size="1" units="litre" constant="true"/>
+    </listOfCompartments>
+    <listOfSpecies>
+      <species id="S1" compartment="c1" initialAmount="5" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+      <species id="S2" compartment="c1" initialAmount="0" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+    </listOfSpecies>
+    <listOfParameters>
+      <parameter id="k" value="1" units="per_second" constant="true"/>
+    </listOfParameters>
+    <listOfReactions>
+      <reaction id="r1" reversible="false" fast="false">
+        <listOfReactants>
+          <speciesReference species="S1" constant="true"/>
+        </listOfReactants>
+        <listOfProducts>
+          <speciesReference species="S2" constant="true"/>
+        </listOfProducts>
+        <kineticLaw>
+          <math xmlns="http://www.w3.org/1998/Math/MathML">
+            <apply>
+              <times/>
+              <ci> k </ci>
+              <ci> S1 </ci>
+              <ci> c1 </ci>
+            </apply>
+          </math>
+        </kineticLaw>
+      </reaction>
+    </listOfReactions>
+  </model>
+</sbml>
diff --git a/tests/sbml/test_data/invalid_doc02.sbml b/tests/sbml/test_data/invalid_doc02.sbml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<sbml xmlns="http://www.sbml.org/sbml/level3/version1/core" level="3" version="1">
+  <model substanceUnits="mole" timeUnits="second" extentUnits="mole">
+    <listOfUnitDefinitions>
+      <unitDefinition id="per_second">
+        <listOfUnits>
+          <unit kind="second" exponent="-1" scale="0" multiplier="1"/>
+        </listOfUnits>
+      </unitDefinition>
+    </listOfUnitDefinitions>
+    <listOfCompartments>
+      <compartment id="c1" spatialDimensions="3" size="1" units="litre" constant="true"/>
+    </listOfCompartments>
+    <listOfSpecies>
+      <species id="S1" compartment="c1" initialAmount="5" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+      <species id="S2" compartment="c1" initialAmount="0" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+    </listOfSpecies>
+    <listOfParameters>
+      <parameter id="k" value="1" units="per_second" constant="true"/>
+    </listOfParameters>
+    <listOfReactions>
+      <reaction id="r1" reversible="false" fast="false">
+        <listOfReactants>
+          <speciesReference species="S1" constant="true"/>
+        </listOfReactants>
+        <listOfProducts>
+          <speciesReference species="S2" constant="true"/>
+        </listOfProducts>
+        <kineticLaw>
+          <math xmlns="http://www.w3.org/1998/Math/MathML">
+            <apply>
+              <times/>
+              <ci> k </ci>
+              <ci> S1 </ci>
+              <ci> c1 </ci>
+            </math>
+          </math>
+        </kineticLaw>
+      </reaction>
+    </listOfReactions>
+  </model>
+</sbml>
diff --git a/tests/sbml/test_data/no_read_access.sbml b/tests/sbml/test_data/no_read_access.sbml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<sbml xmlns="http://www.sbml.org/sbml/level3/version1/core" level="3" version="1">
+  <model substanceUnits="molerat" timeUnits="second" extentUnits="mole">
+    <listOfUnitDefinitions>
+      <unitDefinition id="per_second">
+        <listOfUnits>
+          <unit kind="second" exponent="-1" scale="0" multiplier="1"/>
+        </listOfUnits>
+      </unitDefinition>
+    </listOfUnitDefinitions>
+    <listOfCompartments>
+      <compartment id="c1" spatialDimensions="3" size="1" units="litre" constant="true"/>
+    </listOfCompartments>
+    <listOfSpecies>
+      <species id="S1" compartment="c1" initialAmount="5" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+      <species id="S2" compartment="c1" initialAmount="0" substanceUnits="mole" hasOnlySubstanceUnits="false" boundaryCondition="false" constant="false"/>
+    </listOfSpecies>
+    <listOfParameters>
+      <parameter id="k" value="1" units="per_second" constant="true"/>
+    </listOfParameters>
+    <listOfReactions>
+      <reaction id="r1" reversible="false" fast="false">
+        <listOfReactants>
+          <speciesReference species="S1" constant="true"/>
+        </listOfReactants>
+        <listOfProducts>
+          <speciesReference species="S2" constant="true"/>
+        </listOfProducts>
+        <kineticLaw>
+          <math xmlns="http://www.w3.org/1998/Math/MathML">
+            <apply>
+              <times/>
+              <ci> k </ci>
+              <ci> S1 </ci>
+              <ci> c1 </ci>
+            </apply>
+          </math>
+        </kineticLaw>
+      </reaction>
+    </listOfReactions>
+  </model>
+</sbml>