From b8326ad4678b0aec122ba227c3f22cc287272a31 Mon Sep 17 00:00:00 2001 From: Fede Raimondo Date: Thu, 30 Mar 2023 15:13:30 +0200 Subject: [PATCH 1/4] fix WIP --- junifer/api/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/junifer/api/parser.py b/junifer/api/parser.py index 8111f39f6..a7572b41e 100644 --- a/junifer/api/parser.py +++ b/junifer/api/parser.py @@ -71,4 +71,5 @@ def parse_yaml(filepath: Union[str, Path]) -> Dict: logger.info(f"Importing module: {t_module}") importlib.import_module(t_module) + # Compute path for the URI parameter in storage files that are relative return contents -- 2.52.0 From 0f3264d0fd63f5598930cb36d0e5d911c4473644 Mon Sep 17 00:00:00 2001 From: Fede Raimondo Date: Thu, 30 Mar 2023 15:30:44 +0200 Subject: [PATCH 2/4] Compute URI of storage, if relative. --- docs/changes/newsfragments/127.bugfix | 1 + junifer/api/parser.py | 17 ++++++++++ junifer/api/tests/test_parser.py | 47 +++++++++++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 docs/changes/newsfragments/127.bugfix diff --git a/docs/changes/newsfragments/127.bugfix b/docs/changes/newsfragments/127.bugfix new file mode 100644 index 000000000..2391242de --- /dev/null +++ b/docs/changes/newsfragments/127.bugfix @@ -0,0 +1 @@ +Fix a bug in which relative storage URIs will be computed relative to the CWD and not to the location of the YAML file by `Fede Raimondo`_. \ No newline at end of file diff --git a/junifer/api/parser.py b/junifer/api/parser.py index a7572b41e..a56807247 100644 --- a/junifer/api/parser.py +++ b/junifer/api/parser.py @@ -72,4 +72,21 @@ def parse_yaml(filepath: Union[str, Path]) -> Dict: importlib.import_module(t_module) # Compute path for the URI parameter in storage files that are relative + # This is a tricky thing that appeared in #127. The problem is that + # the path in the URI parameter is relative to YAML file, not to the + # current working directory. If we leave it as is in the contents + # dict, then it will be used later in the ``build`` function as is, + # which will be computed relative to the current working directory. + # The solution is to compute the absolute path and replace the + # relative path in the contents dict with the absolute path. + + # Check if the storage file is defined + if "storage" in contents: + if "uri" in contents["storage"]: + # Check if the storage file is relative + uri_path = Path(contents["storage"]["uri"]) + if not uri_path.is_absolute(): + # Compute the absolute path + contents["storage"]["uri"] = str( + (filepath.parent / uri_path).resolve()) return contents diff --git a/junifer/api/tests/test_parser.py b/junifer/api/tests/test_parser.py index 6585a1809..005451e55 100644 --- a/junifer/api/tests/test_parser.py +++ b/junifer/api/tests/test_parser.py @@ -135,3 +135,50 @@ def test_parse_yaml_absolute_path(tmp_path: Path) -> None: # Check test file parse_yaml(yaml_fname) + + +def test_parse_storage_uri_relative(tmp_path: Path) -> None: + """Test YAML parsing with storage and relative URI. + + Parameters + ---------- + tmp_path : pathlib.Path + The path to the test directory. + + """ + fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" + fname.write_text( + "foo: bar\nwith: numpy\nstorage:\n uri: test.db\n" + ) + + contents = parse_yaml(fname) + assert "foo" in contents + assert contents["foo"] == "bar" + assert "storage" in contents + assert "uri" in contents["storage"] + assert contents["storage"]["uri"] == str(tmp_path / "test.db") + + fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" + fname.write_text( + "foo: bar\nwith: numpy\nstorage:\n uri: ../another/test.db\n" + ) + + contents = parse_yaml(fname) + assert "foo" in contents + assert contents["foo"] == "bar" + assert "storage" in contents + assert "uri" in contents["storage"] + assert contents["storage"]["uri"] == str( + (tmp_path / "../another/test.db").resolve()) + + fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" + fname.write_text( + "foo: bar\nwith: numpy\nstorage:\n uri: /absolute/test.db\n" + ) + + contents = parse_yaml(fname) + assert "foo" in contents + assert contents["foo"] == "bar" + assert "storage" in contents + assert "uri" in contents["storage"] + assert contents["storage"]["uri"] == "/absolute/test.db" -- 2.52.0 From 6f4708776107185591cfa75a899c6ad0a46e6350 Mon Sep 17 00:00:00 2001 From: Synchon Mandal Date: Thu, 30 Mar 2023 16:38:56 +0200 Subject: [PATCH 3/4] chore: black --- junifer/api/parser.py | 3 ++- junifer/api/tests/test_parser.py | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/junifer/api/parser.py b/junifer/api/parser.py index a56807247..8e96b365e 100644 --- a/junifer/api/parser.py +++ b/junifer/api/parser.py @@ -88,5 +88,6 @@ def parse_yaml(filepath: Union[str, Path]) -> Dict: if not uri_path.is_absolute(): # Compute the absolute path contents["storage"]["uri"] = str( - (filepath.parent / uri_path).resolve()) + (filepath.parent / uri_path).resolve() + ) return contents diff --git a/junifer/api/tests/test_parser.py b/junifer/api/tests/test_parser.py index 005451e55..8c87128d4 100644 --- a/junifer/api/tests/test_parser.py +++ b/junifer/api/tests/test_parser.py @@ -147,9 +147,7 @@ def test_parse_storage_uri_relative(tmp_path: Path) -> None: """ fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" - fname.write_text( - "foo: bar\nwith: numpy\nstorage:\n uri: test.db\n" - ) + fname.write_text("foo: bar\nwith: numpy\nstorage:\n uri: test.db\n") contents = parse_yaml(fname) assert "foo" in contents @@ -169,7 +167,8 @@ def test_parse_storage_uri_relative(tmp_path: Path) -> None: assert "storage" in contents assert "uri" in contents["storage"] assert contents["storage"]["uri"] == str( - (tmp_path / "../another/test.db").resolve()) + (tmp_path / "../another/test.db").resolve() + ) fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" fname.write_text( -- 2.52.0 From ae3f975af81555b62b4b92e46cc8c18ac869ac4b Mon Sep 17 00:00:00 2001 From: Fede Raimondo Date: Thu, 30 Mar 2023 18:27:46 +0200 Subject: [PATCH 4/4] Increase coverage --- junifer/api/tests/test_parser.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/junifer/api/tests/test_parser.py b/junifer/api/tests/test_parser.py index 8c87128d4..bf5bcb58d 100644 --- a/junifer/api/tests/test_parser.py +++ b/junifer/api/tests/test_parser.py @@ -77,6 +77,28 @@ def test_parse_yaml_failure_with_multi_module_autoload(tmp_path: Path) -> None: parse_yaml(fname) +def test_parse_yaml_with_wrong_path(tmp_path: Path) -> None: + """Test YAML parsing with wrong paths in with. + + Parameters + ---------- + tmp_path : pathlib.Path + The path to the test directory. + + """ + t_tmp_path = tmp_path / "test_relative_with" + # Write yaml that includes a relative path + yaml_path = t_tmp_path / "yamls" + yaml_path.mkdir(exist_ok=True, parents=True) + yaml_fname = yaml_path / "test_parse_yaml_wrong_path.yaml" + + yaml_fname.write_text("foo: bar\nwith:\n - missingt.py\n - scipy\n") + + # Check test file + with pytest.raises(ValueError, match="does not exist"): + parse_yaml(yaml_fname) + + def test_parse_yaml_relative_path(tmp_path: Path) -> None: """Test YAML parsing with relative paths in with. @@ -181,3 +203,12 @@ def test_parse_storage_uri_relative(tmp_path: Path) -> None: assert "storage" in contents assert "uri" in contents["storage"] assert contents["storage"]["uri"] == "/absolute/test.db" + + # Just to trick coverage + fname = tmp_path / "test_parse_yaml_with_storage_uri.yaml" + fname.write_text("foo: bar\nwith: numpy\nstorage:\n kind: SomeStorage\n") + + contents = parse_yaml(fname) + assert "foo" in contents + assert contents["foo"] == "bar" + assert "storage" in contents -- 2.52.0