Repository: glotzerlab/signac
Pull request #666 (2022-01-21 19:51:49)

Raise error on underspecified path for import/export and view
<!-- Provide a general summary of your changes in the Title above -->

## Description
<!-- Describe your changes in detail -->

Just the bug fix.

There is some duplicate code, but that will be refactored in the rest of #642.

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->

see #642 for a description

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [ ] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [ ] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [ ] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [ ] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [ ] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.

<!-- Example for a changelog entry: `Fix issue with launching rockets to the moon (#101, #212).` -->


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
> Merging [#666](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (eb7d423) into [master](https://codecov.io/gh/glotzerlab/signac/commit/8176ba98a3c8f15649f0978d891d7cc41962e8a5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (8176ba9) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/666/graphs/tree.svg?width=650&height=150&src=pr&token=FghbjTyzgM&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)

```diff
@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   78.40%   78.43%   +0.03%     
==========================================
  Files          65       65              
  Lines        7112     7122      +10     
  Branches     1561     1564       +3     
==========================================
+ Hits         5576     5586      +10     
  Misses       1226     1226              
  Partials      310      310              
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Coverage Δ | |
|---|---|---|
| [signac/contrib/linked\_view.py](https://codecov.io/gh/glotzerlab/signac/pull/666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvbGlua2VkX3ZpZXcucHk=) | `83.91% <ø> (ø)` | |
| [signac/contrib/import\_export.py](https://codecov.io/gh/glotzerlab/signac/pull/666/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvaW1wb3J0X2V4cG9ydC5weQ==) | `78.47% <100.00%> (+0.58%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Last update [8176ba9...eb7d423](https://codecov.io/gh/glotzerlab/signac/pull/666?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).


Patch:
diff --git a/changelog.txt b/changelog.txt
index 544c9912..4cd5a22a 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -29,6 +29,7 @@ Fixed
 +++++
 
  - ``H5Store.mode`` returns the file mode (#607).
+ - User-provided path functions now raise an error if not unique (#666).
 
 [1.7.0] -- 2021-06-08
 ---------------------
diff --git a/signac/contrib/import_export.py b/signac/contrib/import_export.py
index ea1cfdbf..fda1cdf8 100644
--- a/signac/contrib/import_export.py
+++ b/signac/contrib/import_export.py
@@ -11,7 +11,7 @@ import re
 import shutil
 import tarfile
 import zipfile
-from collections import OrderedDict
+from collections import Counter, OrderedDict
 from contextlib import closing, contextmanager
 from string import Formatter
 from tempfile import TemporaryDirectory
@@ -149,6 +149,40 @@ class _SchemaPathEvaluationError(RuntimeError):
     pass
 
 
+def _check_path_function_unique(jobs, path_spec, path_function):
+    """Validate that path function specifies a 1-1 mapping.
+
+    Parameters
+    ----------
+    jobs : iterable of :class:`~signac.contrib.job.Job`
+        A sequence of jobs (instances of :class:`~signac.contrib.job.Job`).
+    path_spec : str
+        The path string that generated the path_function. Displayed in the
+        error message.
+    path_function : callable
+        A callable path generating function.
+
+    Raises
+    ------
+    RuntimeError
+        If paths generated with given path function are not unique.
+
+    """
+    job_paths = Counter(path_function(job) for job in jobs)
+    duplicates = {path for path, count in job_paths.items() if count > 1}
+    if len(duplicates) > 0:
+        # Log paths generated more than once
+        for path in duplicates:
+            logger.debug(f"Generated path '{path}' is not unique.")
+        raise RuntimeError(
+            f"The path specification '{path_spec}' would result in duplicate "
+            "paths. See the debug log for the list of duplicate paths. The "
+            "easiest way to fix this is to append the job id to the path "
+            "specification like "
+            f"'{os.path.join(str(path_spec), 'id', '{job.id}')}'."
+        )
+
+
 def _make_path_function(jobs, path):
     """Generate a path function for jobs or use ``path`` if it is callable.
 
@@ -194,7 +228,7 @@ def _make_path_function(jobs, path):
             return str(job.id)
 
     elif isinstance(path, str):
-        # Detect keys that are already provided as part of the path specifier and
+        # Detect keys that are already provided as part of the path specifier
         # and should therefore be excluded from the 'auto'-part.
         exclude_keys = [x[1] for x in Formatter().parse(path)]
 
@@ -241,6 +275,9 @@ def _make_path_function(jobs, path):
             except Exception as error:
                 raise _SchemaPathEvaluationError(error)
 
+        # Check that the user-specified path generates a 1-1 mapping
+        _check_path_function_unique(jobs, path_spec=path, path_function=path_function)
+
     else:
         raise ValueError(
             "The path argument must either be `None`, `False`, or of type `str`."
@@ -297,21 +334,18 @@ def _export_jobs(jobs, path, copytree):
     ------
     RuntimeError
         If paths generated with given path function are not unique.
-
     """
     # Transform the path argument into a callable if necessary.
     if callable(path):
         path_function = path
+        _check_path_function_unique(jobs, path_spec=path, path_function=path_function)
     else:
         path_function = _make_path_function(jobs, path)
+        # path_function is checked for uniqueness inside _make_path_function
 
     # Determine export path for each job.
     paths = {job.workspace(): path_function(job) for job in jobs}
 
-    # Check whether the mapped paths are unique.
-    if len(set(paths.values())) != len(paths):
-        raise RuntimeError("Paths generated with given path function are not unique!")
-
     # Check leaf/node consistency
     _check_directory_structure_validity(paths.values())
 
diff --git a/signac/contrib/linked_view.py b/signac/contrib/linked_view.py
index 6a591a74..a30bd5d9 100644
--- a/signac/contrib/linked_view.py
+++ b/signac/contrib/linked_view.py
@@ -109,8 +109,8 @@ def create_linked_view(project, prefix=None, job_ids=None, index=None, path=None
             links["./job"] = job.workspace()
         assert len(links) < 2
     _check_directory_structure_validity(links.keys())
-
     _update_view(prefix, links)
+
     return links
 
 


Test patch:
diff --git a/tests/test_project.py b/tests/test_project.py
index 50da1e40..724b122f 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -2003,6 +2003,12 @@ class TestLinkedViewProject(TestProjectBase):
                     sp = {"a": a, "d": {"b": b, "c": c}}
                     self.project.open_job(sp).init()
 
+        # Should error if user-provided path doesn't make 1-1 mapping
+        with pytest.raises(RuntimeError):
+            self.project.create_linked_view(
+                prefix=view_prefix, path=os.path.join("a", "{a}")
+            )
+
         self.project.create_linked_view(
             prefix=view_prefix, path="a/{a}/d.c/{d.c}/{{auto}}"
         )
@@ -2230,6 +2236,23 @@ class TestLinkedViewProject(TestProjectBase):
             with pytest.raises(RuntimeError):
                 self.project.create_linked_view(prefix=view_prefix)
 
+    @pytest.mark.skipif(WINDOWS, reason="Linked views unsupported on Windows.")
+    def test_create_linked_view_duplicate_paths(self):
+        view_prefix = os.path.join(self._tmp_pr, "view")
+        a_vals = range(2)
+        b_vals = range(3, 8)
+        for a in a_vals:
+            for b in b_vals:
+                sp = {"a": a, "b": b}
+                self.project.open_job(sp).init()
+
+        # An error should be raised if the user-provided path function doesn't
+        # make a 1-1 mapping.
+        with pytest.raises(RuntimeError):
+            self.project.create_linked_view(
+                prefix=view_prefix, path=os.path.join("a", "{a}")
+            )
+
 
 class UpdateCacheAfterInitJob(signac.contrib.job.Job):
     def init(self, *args, **kwargs):
diff --git a/tests/test_shell.py b/tests/test_shell.py
index d2eaaeed..d4791bc8 100644
--- a/tests/test_shell.py
+++ b/tests/test_shell.py
@@ -237,6 +237,24 @@ class TestBasicShell:
                 "view/a/{}/job".format(sp["a"])
             ) == os.path.realpath(project.open_job(sp).workspace())
 
+    @pytest.mark.skipif(WINDOWS, reason="Symbolic links are unsupported on Windows.")
+    def test_view_incomplete_path_spec(self):
+        self.call("python -m signac init my_project".split())
+        project = signac.Project()
+        sps = [{"a": i} for i in range(3)]
+        for sp in sps:
+            project.open_job(sp).init()
+        os.mkdir("view")
+
+        # An error should be raised if the user-provided path function
+        # doesn't make a 1-1 mapping.
+        err = self.call(
+            "python -m signac view view non_unique".split(),
+            error=True,
+            raise_error=False,
+        )
+        assert "duplicate paths" in err
+
     def test_find(self):
         self.call("python -m signac init my_project".split())
         project = signac.Project()