From 820072034f68c111ca91325d14dad8fdc9d42abf Mon Sep 17 00:00:00 2001 From: guillaumepichon Date: Tue, 5 Aug 2025 09:58:23 +0200 Subject: [PATCH 1/3] Removing ruamel.yaml in github workflow. --- .github/workflows/tests.yml | 1 - pyproject.toml | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 92ef0f4b..637a65f7 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -32,7 +32,6 @@ jobs: pip install accelerator-toolbox pip install pyaml pip install flake8 pytest - pip install ruamel.yaml if [ -f requirements.txt ]; then pip install -r requirements.txt; fi - name: Lint with flake8 run: | diff --git a/pyproject.toml b/pyproject.toml index e8eb3fbb..4a779c55 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,8 +44,7 @@ dependencies = [ "scipy>=1.7.3", "accelerator-toolbox>=0.6.1", "PyYAML>=6.0.2", - "pydantic>=2.11.7", - "ruamel.yaml" + "pydantic>=2.11.7" ] [project.optional-dependencies] From f1774634de83dd7204cfb285cb61720a8dab4070 Mon Sep 17 00:00:00 2001 From: guillaumepichon Date: Tue, 5 Aug 2025 10:30:33 +0200 Subject: [PATCH 2/3] Check of cycles in configuration files. --- pyaml/configuration/fileloader.py | 54 ++++++++++++++++++------------- tests/config/bad_conf_cycles.json | 32 ++++++++++++++++++ tests/config/bad_conf_cycles.yml | 26 +++++++++++++++ tests/test_factory.py | 26 +++++++++++---- 4 files changed, 109 insertions(+), 29 deletions(-) create mode 100644 tests/config/bad_conf_cycles.json create mode 100644 tests/config/bad_conf_cycles.yml diff --git a/pyaml/configuration/fileloader.py b/pyaml/configuration/fileloader.py index 34af0a9c..e08e5703 100644 --- a/pyaml/configuration/fileloader.py +++ b/pyaml/configuration/fileloader.py @@ -15,43 +15,49 @@ logger = logging.getLogger(__name__) -#TODO -#Implement cycle detection in case of wrong yaml/json link that creates a cycle +accepted_suffixes = [".yaml", ".yml", ".json"] -def load(filename:str) -> Union[dict,list]: +def load(filename:str, paths_stack:list=None) -> Union[dict,list]: """Load recursively a configuration setup""" if filename.endswith(".yaml") or filename.endswith(".yml"): - l = YAMLLoader(filename) + l = YAMLLoader(filename, paths_stack) elif filename.endswith(".json"): - l = JSONLoader(filename) + l = JSONLoader(filename, paths_stack) else: raise PyAMLException(f"{filename} File format not supported (only .yaml .yml or .json)") - return l.load(filename) + return l.load() + +# Expand condition +def hasToExpand(value): + return isinstance(value,str) and any(value.endswith(suffix) for suffix in accepted_suffixes) + # Loader base class (nested files expansion) class Loader: - def __init__(self, filename:str): - self.suffixes = [] + def __init__(self, filename:str, parent_path_stack:list): self.path:Path = get_root_folder() / filename + self.files_stack = [] + if parent_path_stack: + if any(self.path.samefile(parent_path) for parent_path in parent_path_stack): + raise PyAMLException(f"A cycle has been detected: {parent_path_stack}") + self.files_stack.extend(parent_path_stack) + self.files_stack.append(self.path) - # Expand condition - def hasToExpand(self,value): - return isinstance(value,str) and any(value.endswith(suffix) for suffix in self.suffixes) # Recursively expand a dict def expand_dict(self,d:dict): for key, value in d.items(): - if self.hasToExpand(value): - d[key] = load(value) + if hasToExpand(value): + d[key] = load(value, self.files_stack) else: self.expand(value) # Recursively expand a list def expand_list(self,l:list): for idx,value in enumerate(l): - if self.hasToExpand(value): - l[idx] = load(value) + if hasToExpand(value): + l[idx] = load(value, self.files_stack) else: self.expand(value) @@ -65,7 +71,7 @@ def expand(self,obj: Union[dict,list]): # Load a file def load(self) -> Union[dict,list]: - raise Exception(str(self.path) + ": load() method not implemented") + raise PyAMLException(str(self.path) + ": load() method not implemented") class SafeLineLoader(SafeLoader): @@ -93,22 +99,24 @@ def construct_mapping(self, node, deep=False): # YAML loader class YAMLLoader(Loader): - - def load(self,fileName:str) -> Union[dict,list]: - self.path:Path = get_root_folder() / fileName - self.suffixes = [".yaml",".yml"] + def __init__(self, filename: str, parent_paths_stack:list): + super().__init__(filename, parent_paths_stack) + + def load(self) -> Union[dict,list]: + logger.log(logging.DEBUG, f"Loading YAML file '{self.path}'") with open(self.path) as file: try: return self.expand(yaml.load(file,Loader=SafeLineLoader)) except yaml.YAMLError as e: - raise Exception(self.path + ": " + str(e)) + raise PyAMLException(str(self.path) + ": " + str(e)) from e # JSON loader class JSONLoader(Loader): + def __init__(self, filename: str, parent_paths_stack:list): + super().__init__(filename, parent_paths_stack) - def load(self,fileName:str) -> Union[dict,list]: + def load(self) -> Union[dict,list]: logger.log(logging.DEBUG, f"Loading JSON file '{self.path}'") - self.suffixes = [".json"] with open(self.path) as file: try: return self.expand(json.load(file)) diff --git a/tests/config/bad_conf_cycles.json b/tests/config/bad_conf_cycles.json new file mode 100644 index 00000000..2cd7dd31 --- /dev/null +++ b/tests/config/bad_conf_cycles.json @@ -0,0 +1,32 @@ +{ + "type": "pyaml.pyaml", + "instruments":[ + { + "type": "pyaml.instrument", + "name": "sr", + "energy": 6e9, + "simulators": ["../config/bad_conf_cycles.json"], + "data_folder": "/data/store", + "arrays": [ + { + "type": "pyaml.arrays.hcorrector", + "name": "HCORR", + "elements": [ + "SH1A-C01-H", + "SH1A-C02-H" + ] + }, + { + "type": "pyaml.arrays.vcorrector", + "name": "VCORR", + "elements": ["SH1A-C01-V", "SH1A-C02-V"] + } + ], + "devices": [ + "sr/quadrupoles/QF1AC01.yaml", + "sr/correctors/SH1AC01.yaml", + "sr/correctors/SH1AC02.yaml" + ] + } + ] +} \ No newline at end of file diff --git a/tests/config/bad_conf_cycles.yml b/tests/config/bad_conf_cycles.yml new file mode 100644 index 00000000..89acd7e4 --- /dev/null +++ b/tests/config/bad_conf_cycles.yml @@ -0,0 +1,26 @@ +type: pyaml.pyaml +instruments: + - type: pyaml.instrument + name: sr + energy: 6e9 + simulators: + - type: pyaml.lattice.simulator + lattice: sr/lattices/ebs.mat + name: design + data_folder: /data/store + arrays: + - type: pyaml.arrays.hcorrector + name: HCORR + elements: + - SH1A-C01-H + - SH1A-C02-H + - type: pyaml.arrays.vcorrector + name: VCORR + elements: + - SH1A-C01-V + - SH1A-C02-V + devices: + - ../config/bad_conf_cycles.yml # Cycle here + - sr/quadrupoles/QF1AC01.yaml + - sr/correctors/SH1AC01.yaml + - sr/correctors/SH1AC02.yaml diff --git a/tests/test_factory.py b/tests/test_factory.py index f77ab58b..09f32b76 100644 --- a/tests/test_factory.py +++ b/tests/test_factory.py @@ -1,5 +1,5 @@ import pytest -from pyaml import PyAMLConfigException +from pyaml import PyAMLConfigException, PyAMLException from pyaml.configuration.factory import Factory from pyaml.pyaml import PyAML, pyaml from tests.conftest import MockElement @@ -28,9 +28,23 @@ def test_factory_with_custom_strategy(): assert obj.name == "custom_injected" -def test_error_location(): - +@pytest.mark.parametrize("test_file", [ + "tests/config/bad_conf.yml", +]) +def test_error_location(test_file): with pytest.raises(PyAMLConfigException) as exc: - ml: PyAML = pyaml("tests/config/bad_conf.yml") - - assert "at line 7, column 9" in str(exc.value) + ml: PyAML = pyaml(test_file) + print(str(exc.value)) + test_file_names = test_file.split("/") + test_file_name = test_file_names[len(test_file_names)-1] + assert f"{test_file_name} at line 7, column 9" in str(exc.value) + +@pytest.mark.parametrize("test_file", [ + "tests/config/bad_conf_cycles.yml", + "tests/config/bad_conf_cycles.json", +]) +def test_error_cycles(test_file): + with pytest.raises(PyAMLException) as exc: + ml: PyAML = pyaml(test_file) + + assert "A cycle has been detected" in str(exc.value) From a3e70f784ff3da097523f748cb3bf39b3cb86caf Mon Sep 17 00:00:00 2001 From: guillaumepichon Date: Tue, 5 Aug 2025 16:20:03 +0200 Subject: [PATCH 3/3] More explicit exception message --- pyaml/configuration/fileloader.py | 34 ++++++++++++++++++++++++------- tests/test_factory.py | 4 +++- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/pyaml/configuration/fileloader.py b/pyaml/configuration/fileloader.py index e08e5703..7d902699 100644 --- a/pyaml/configuration/fileloader.py +++ b/pyaml/configuration/fileloader.py @@ -17,6 +17,14 @@ accepted_suffixes = [".yaml", ".yml", ".json"] +class PyAMLConfigCyclingException(PyAMLException): + + def __init__(self, error_filename:str, path_stack:list[Path]): + self.error_filename = error_filename + parent_file_stack = [parent_path.name for parent_path in path_stack] + super().__init__(f"Circular file inclusion of {error_filename}. File list before reaching it: {parent_file_stack}") + pass + def load(filename:str, paths_stack:list=None) -> Union[dict,list]: """Load recursively a configuration setup""" if filename.endswith(".yaml") or filename.endswith(".yml"): @@ -35,12 +43,12 @@ def hasToExpand(value): # Loader base class (nested files expansion) class Loader: - def __init__(self, filename:str, parent_path_stack:list): + def __init__(self, filename:str, parent_path_stack:list[Path]): self.path:Path = get_root_folder() / filename - self.files_stack = [] + self.files_stack:list[Path] = [] if parent_path_stack: if any(self.path.samefile(parent_path) for parent_path in parent_path_stack): - raise PyAMLException(f"A cycle has been detected: {parent_path_stack}") + raise PyAMLConfigCyclingException(filename, parent_path_stack) self.files_stack.extend(parent_path_stack) self.files_stack.append(self.path) @@ -48,10 +56,22 @@ def __init__(self, filename:str, parent_path_stack:list): # Recursively expand a dict def expand_dict(self,d:dict): for key, value in d.items(): - if hasToExpand(value): - d[key] = load(value, self.files_stack) - else: - self.expand(value) + try: + if hasToExpand(value): + d[key] = load(value, self.files_stack) + else: + self.expand(value) + except PyAMLConfigCyclingException as pyaml_ex: + location = d.pop('__location__', None) + field_locations = d.pop('__fieldlocations__', None) + location_str = "" + if location: + file, line, col = location + if field_locations and key in field_locations: + location = field_locations[key] + file, line, col = location + location_str = f" in {file} at line {line}, column {col}" + raise PyAMLException(f"Circular file inclusion of {pyaml_ex.error_filename}{location_str}") from pyaml_ex # Recursively expand a list def expand_list(self,l:list): diff --git a/tests/test_factory.py b/tests/test_factory.py index 09f32b76..78cc926e 100644 --- a/tests/test_factory.py +++ b/tests/test_factory.py @@ -47,4 +47,6 @@ def test_error_cycles(test_file): with pytest.raises(PyAMLException) as exc: ml: PyAML = pyaml(test_file) - assert "A cycle has been detected" in str(exc.value) + assert "Circular file inclusion of " in str(exc.value) + if not test_file.endswith(".json"): + assert "at line 23" in str(exc.value)