Merge pull request #47 from HEPData/yaml-fixes

Fixes for LibYAML issues

Author: GraemeWatt

.github/workflows/ci.yml

@@ -28,15 +28,23 @@ jobs:
       run: |
         pip install --upgrade pip setuptools wheel coveralls
         pip install --ignore-installed -e .[tests]
-    - name: Run tests
+    - name: Run tests without libyaml
       run: |
-        pytest testsuite
+        USE_LIBYAML=False pytest testsuite
+      env:
+        COVERAGE_FILE: '.coverage_nolibyaml'
+    - name: Run tests with libyaml
+      run: |
+        USE_LIBYAML=True pytest testsuite
+      env:
+        COVERAGE_FILE: '.coverage_libyaml'
     - name: Run coveralls
       if: startsWith(matrix.python-version, '3.8')
       env:
         GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         COVERALLS_SERVICE_NAME: github
       run: |
+        coverage combine .coverage_libyaml .coverage_nolibyaml
         coveralls
 
   deploy:

README.rst

@@ -49,17 +49,32 @@ Install from `PyPI <https://pypi.org/project/hepdata-validator/>`_ using ``pip``
    $ pip install --user hepdata-validator
    $ hepdata-validate --help
 
-Install from GitHub (for developers) in a `virtual environment <https://docs.python.org/3/tutorial/venv.html>`_:
+If you would like to use LibYAML, you may need an additional step if running on an M1 Mac, to ensure pyyaml is built
+with the LibYAML bindings. Run the following after installing LibYAML via Homebrew:
+
+.. code:: bash
+
+  $ LDFLAGS="-L$(brew --prefix)/lib" CFLAGS="-I$(brew --prefix)/include" pip install --global-option="--with-libyaml" --force pyyaml
+
+
+Developers
+==========
+Developers should install from GitHub in a `virtual environment <https://docs.python.org/3/tutorial/venv.html>`_:
 
 .. code:: bash
 
    $ git clone https://github.com/HEPData/hepdata-validator
    $ cd hepdata-validator
    $ python3 -m venv ~/venv/hepdata-validator
    $ source ~/venv/hepdata-validator/bin/activate
-   (hepdata-validator) $ pip install --upgrade -e .[tests]
-   (hepdata-validator) $ pytest testsuite
+   (hepdata-validator) $ pip install --upgrade -e ".[tests]"
+
+Tests should be run both with and without LibYAML, as error messages from the different YAML parsers vary:
+
+.. code:: bash
 
+   (hepdata-validator) $ USE_LIBYAML=True pytest testsuite
+   (hepdata-validator) $ USE_LIBYAML=False pytest testsuite
 
 Usage
 -----

hepdata_validator/__init__.py

@@ -32,6 +32,21 @@
 
 from .version import __version__
 
+# We try to load using the CSafeLoader for speed improvements
+YamlLoader = None
+YamlDumper = None
+use_libyaml = os.environ.get('USE_LIBYAML', True)
+if use_libyaml and use_libyaml not in ('False', 'false', 'f', 'F'):
+    try:
+        from yaml import CSafeLoader as YamlLoader
+        from yaml import CSafeDumper as YamlDumper
+    except ImportError:  # pragma: no cover
+        pass
+
+if YamlLoader is None or YamlDumper is None:
+    from yaml import SafeLoader as YamlLoader
+    from yaml import SafeDumper as YamlDumper
+
 __all__ = ('__version__', )
 
 VALID_SCHEMA_VERSIONS = ['1.1.0', '1.0.1', '1.0.0', '0.1.0']

hepdata_validator/data_file_validator.py

@@ -29,13 +29,7 @@
 from packaging import version as packaging_version
 import yaml
 
-# We try to load using the CSafeLoader for speed improvements.
-try:
-    from yaml import CSafeLoader as Loader
-except ImportError: #pragma: no cover
-    from yaml import SafeLoader as Loader #pragma: no cover
-
-from hepdata_validator import Validator, ValidationMessage
+from hepdata_validator import Validator, ValidationMessage, YamlLoader
 from jsonschema import ValidationError
 from jsonschema.exceptions import by_relevance
 
@@ -104,7 +98,7 @@ def validate(self, **kwargs):
             try:
                 # The yaml package support both JSON and YAML
                 with open(file_path, 'r') as df:
-                    data = yaml.load(df, Loader=Loader)
+                    data = yaml.load(df, Loader=YamlLoader)
                     if data is None:
                         self.add_validation_message(ValidationMessage(
                             file=file_path,

hepdata_validator/full_submission_validator.py

@@ -8,20 +8,12 @@
 
 import yaml
 
-from hepdata_validator import Validator, ValidationMessage
+from hepdata_validator import Validator, ValidationMessage, YamlLoader, YamlDumper
 from .schema_resolver import JsonSchemaResolver
 from .schema_downloader import HTTPSchemaDownloader
 from .submission_file_validator import SubmissionFileValidator
 from .data_file_validator import DataFileValidator
 
-# We try to load using the CSafeLoader for speed improvements.
-try:
-    from yaml import CSafeLoader as Loader
-    from yaml import CSafeDumper as Dumper
-except ImportError:  # pragma: no cover
-    from yaml import SafeLoader as Loader
-    from yaml import SafeDumper as Dumper
-
 
 class SchemaType(Enum):
     SUBMISSION = 'submission'
@@ -169,7 +161,7 @@ def validate(self, directory=None, file=None, archive=None):
             # Open the submission.yaml file and load all YAML documents.
             with open(self.submission_file_path, 'r') as submission_file:
                 try:
-                    self.submission_docs = list(yaml.load_all(submission_file, Loader=Loader))
+                    self.submission_docs = list(yaml.load_all(submission_file, Loader=YamlLoader))
                 except yaml.YAMLError as e:
                     self._add_validation_message(
                         file=self.submission_file_path,
@@ -264,7 +256,7 @@ def _create_data_files(self, docs):
                     file_name = os.path.join(self.directory, file_name)
                 with open(file_name, 'w') as data_file:
                     yaml.dump({'independent_variables': doc.pop('independent_variables', None),
-                               'dependent_variables': doc.pop('dependent_variables', None)}, data_file, Dumper=Dumper)
+                               'dependent_variables': doc.pop('dependent_variables', None)}, data_file, Dumper=YamlDumper)
 
     def _check_doc(self, doc):
         # Skip empty YAML documents.
@@ -345,7 +337,7 @@ def _check_doc(self, doc):
 
             # Just try to load YAML data file without validating schema.
             try:
-                contents = yaml.load(open(data_file_path, 'r'), Loader=Loader)
+                contents = yaml.load(open(data_file_path, 'r'), Loader=YamlLoader)
             except (OSError, yaml.YAMLError) as e:
                 problem_type = 'reading' if isinstance(e, OSError) else 'parsing'
                 self._add_validation_message(

hepdata_validator/submission_file_validator.py

@@ -6,13 +6,7 @@
 import yaml
 from yaml.scanner import ScannerError
 
-# We try to load using the CSafeLoader for speed improvements.
-try:
-    from yaml import CSafeLoader as Loader
-except ImportError: #pragma: no cover
-    from yaml import SafeLoader as Loader #pragma: no cover
-
-from hepdata_validator import Validator, ValidationMessage
+from hepdata_validator import Validator, ValidationMessage, YamlLoader
 
 __author__ = 'eamonnmaguire'
 
@@ -71,7 +65,7 @@ def validate(self, **kwargs):
 
             if data is None:
                 data_file_handle = open(file_path, 'r')
-                data = yaml.load_all(data_file_handle, Loader=Loader)
+                data = yaml.load_all(data_file_handle, Loader=YamlLoader)
 
             table_names = []
             table_data_files = []

testsuite/test_data_validator.py

@@ -280,11 +280,12 @@ def test_invalid_parser_yaml_file_v1(validator_v1, data_path, capsys):
 
     assert is_valid is False
     out, err = capsys.readouterr()
-    assert out.strip() == f"""error - There was a problem parsing the file.
+    assert out.strip().startswith(f"""error - There was a problem parsing the file.
 while parsing a flow mapping
-  in "{file}", line 9, column 9
-did not find expected ',' or '}}'
-  in "{file}", line 10, column 5"""
+  in "{file}", line 9, column 9""")
+    # message is different in libyaml and non-libyaml versions but this is in both
+    assert "expected ',' or '}'" in out
+    assert out.strip().endswith(f'in "{file}", line 10, column 5')
 
 
 def test_io_error_yaml_file_v1(validator_v1, data_path, capsys):

testsuite/test_full_submission_validator.py

@@ -222,11 +222,12 @@ def test_invalid_yaml_single_file_gzip(validator_v1, data_path, capsys):
     assert validator_v1.valid_files == {}
     validator_v1.print_errors('1512299_invalid_yaml.yaml')
     out, err = capsys.readouterr()
-    assert out.strip() == """error - There was a problem parsing the file:
+    assert out.strip().startswith("""error - There was a problem parsing the file:
 		while parsing a flow mapping
-		  in "1512299_invalid_yaml.yaml", line 7, column 11
-		did not find expected ',' or '}'
-		  in "1512299_invalid_yaml.yaml", line 8, column 3"""
+		  in "1512299_invalid_yaml.yaml", line 7, column 11""")
+    # message is different in libyaml and non-libyaml versions but this is in both
+    assert "expected ',' or '}'" in out
+    assert out.strip().endswith('in "1512299_invalid_yaml.yaml", line 8, column 3')
 
 
 def test_invalid_single_file_data_file(validator_v1, data_path, capsys):
@@ -266,11 +267,13 @@ def test_invalid_data_directory(validator_v1, data_path, capsys):
     assert errors[expected_file_names[0]][1].message == "Location of 'additional_resources' file '../TestHEPSubmission/figFigure8B.png' should not contain '/'."
     assert errors[expected_file_names[0]][2].message == f"Missing 'additional_resources' file 'figFigure9A.png'."
     assert errors[expected_file_names[1]][0].message == f"Missing data_file 'data3.yaml'."
-    assert errors[expected_file_names[2]][0].message == f"""There was a problem parsing the file:
+    assert errors[expected_file_names[2]][0].message.startswith(f"""There was a problem parsing the file:
 		while parsing a block mapping
-		  in "{dir}/data8.yaml", line 1, column 1
-		did not find expected key
-		  in "{dir}/data8.yaml", line 9, column 3"""
+		  in "{dir}/data8.yaml", line 1, column 1""")
+    # message differs depending on whether libyaml is used
+    assert "did not find expected key" in errors[expected_file_names[2]][0].message or \
+           "expected <block end>, but found '<block mapping start>'" in errors[expected_file_names[2]][0].message
+    assert errors[expected_file_names[2]][0].message.endswith(f'in "{dir}/data8.yaml", line 9, column 3')
     assert errors[expected_file_names[3]][0].message == f"figFigure8B.png is not referenced in the submission."
     assert len(errors[expected_file_names[4]]) == 2
     assert errors[expected_file_names[4]][0].message == f"._data10.yaml is not referenced in the submission."
@@ -304,11 +307,13 @@ def test_invalid_archive(validator_v1, data_path):#, capsys):
     assert errors[expected_file_names[0]][1].message == "Location of 'additional_resources' file '../TestHEPSubmission/figFigure8B.png' should not contain '/'."
     assert errors[expected_file_names[0]][2].message == f"Missing 'additional_resources' file 'figFigure9A.png'."
     assert errors[expected_file_names[1]][0].message == f"Missing data_file 'data3.yaml'."
-    assert errors[expected_file_names[2]][0].message == f"""There was a problem parsing the file:
+    assert errors[expected_file_names[2]][0].message.startswith(f"""There was a problem parsing the file:
 		while parsing a block mapping
-		  in "{dir}/data8.yaml", line 1, column 1
-		did not find expected key
-		  in "{dir}/data8.yaml", line 9, column 3"""
+		  in "{dir}/data8.yaml", line 1, column 1""")
+    # message differs depending on whether libyaml is used
+    assert "did not find expected key" in errors[expected_file_names[2]][0].message or \
+           "expected <block end>, but found '<block mapping start>'" in errors[expected_file_names[2]][0].message
+    assert errors[expected_file_names[2]][0].message.endswith(f'in "{dir}/data8.yaml", line 9, column 3')
     assert errors[expected_file_names[3]][0].message == f"figFigure8B.png is not referenced in the submission."
     assert len(errors[expected_file_names[4]]) == 2
     assert errors[expected_file_names[4]][0].message == f"._data10.yaml is not referenced in the submission."

testsuite/test_submission_validator.py

@@ -1,15 +1,9 @@
 import os
 import pytest
 import yaml
-from hepdata_validator import VALID_SCHEMA_VERSIONS
+from hepdata_validator import VALID_SCHEMA_VERSIONS, YamlLoader
 from hepdata_validator.submission_file_validator import SubmissionFileValidator
 
-# We try to load using the CSafeLoader for speed improvements.
-try:
-    from yaml import CSafeLoader as Loader
-except ImportError:
-    from yaml import SafeLoader as Loader
-
 
 ####################################################
 #                 Tests fixtures                   #
@@ -45,7 +39,7 @@ def test_valid_submission_yaml_v0(validator_v0, data_path):
     file = os.path.join(data_path, 'valid_submission_v0.yaml')
 
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v0.validate(file_path=file, data=yaml_obj)
         validator_v0.print_errors(file)
 
@@ -65,7 +59,7 @@ def test_valid_submission_yaml_v1(validator_v1, data_path):
     file = os.path.join(data_path, 'valid_submission.yaml')
 
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v1.validate(file_path=file, data=yaml_obj)
         validator_v1.print_errors(file)
 
@@ -80,7 +74,7 @@ def test_v0_valid_submission_yaml_v1(validator_v1, data_path):
     file = os.path.join(data_path, 'valid_submission_v0.yaml')
 
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v1.validate(file_path=file, data=yaml_obj)
         validator_v1.print_errors(file)
 
@@ -235,10 +229,11 @@ def test_invalid_parser_submission_yaml_v1(validator_v1, data_path, capsys):
 
     assert is_valid is False
     out, err = capsys.readouterr()
-    assert out.strip() == """error - while parsing a flow mapping
-  in "{0}", line 6, column 5
-did not find expected ',' or '}}'
-  in "{0}", line 7, column 3""".format(file)
+    assert out.strip().startswith(f"""error - while parsing a flow mapping
+  in "{file}", line 6, column 5""")
+    # message is different in libyaml and non-libyaml versions but this is in both
+    assert "expected ',' or '}'" in out
+    assert out.strip().endswith(f'in "{file}", line 7, column 3')
 
 
 def test_io_error_submission_yaml_v1(validator_v1, data_path, capsys):
@@ -284,7 +279,7 @@ def test_data_schema_submission_yaml_v1(validator_v1, data_path):
     file = os.path.join(data_path, 'valid_submission_custom_remote.yaml')
 
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v1.validate(file_path=file, data=yaml_obj)
         assert is_valid is True
         assert not validator_v1.has_errors(file)
@@ -298,7 +293,7 @@ def test_invalid_cmenergies_submission_yaml_v1(validator_v1, data_path, capsys):
     file = os.path.join(data_path, 'invalid_cmenergies.yaml')
 
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v1.validate(file_path=file, data=yaml_obj)
         validator_v1.print_errors(file)
 
@@ -332,7 +327,7 @@ def test_check_for_duplicates(validator_v1):
 def test_submission_with_no_data_tables(validator_v0, validator_v1, data_path, capsys):
     file = os.path.join(data_path, 'valid_file.yaml')
     with open(file, 'r') as submission:
-        yaml_obj = yaml.load_all(submission, Loader=Loader)
+        yaml_obj = yaml.load_all(submission, Loader=YamlLoader)
         is_valid = validator_v1.validate(file_path=file, data=yaml_obj)
         validator_v1.print_errors(file)