Repository: glotzerlab/signac
Pull request #816 (2022-09-18 19:51:56)

Warn users to migrate when old schema is detected and enable migration CLI
<!-- Provide a general summary of your changes in the title above. -->

## Description
<!-- Describe your changes in detail. -->
<!-- Please indicate if the changes may break existing functionality. -->
This PR adds a check in project initialization to ensure that projects with old schemas are detected when signac fails to find a project with the current schema. The resulting check provides the user a meaningful error about how to proceed with updating their schema using our migration code. This PR also exposes a CLI for migration to complement the Python interface, providing a simpler and friendlier approach to one-off migration that is recommended by the error.

## 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. -->
Necessary to for projects how to migrate to v2.

## Checklist:
<!-- This checklist must be complete before merging the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [Contributing Guidelines](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [Contributor Agreement](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.
- [x] The [package documentation](https://github.com/glotzerlab/signac/tree/master/doc) and [framework documentation](https://docs.signac.io/) in [signac-docs](https://github.com/glotzerlab/signac-docs) are up to date with these changes.
- [x] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added any related issue and pull request numbers for future reference.


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/816?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
> Merging [#816](https://codecov.io/gh/glotzerlab/signac/pull/816?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (fbf1225) into [next](https://codecov.io/gh/glotzerlab/signac/commit/52ff01754cb9189ef67bedc664ce683cee4a75e0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (52ff017) will **decrease** coverage by `9.13%`.
> The diff coverage is `77.89%`.

```diff
@@            Coverage Diff             @@
##             next     #816      +/-   ##
==========================================
- Coverage   86.32%   77.19%   -9.14%     
==========================================
  Files          51       53       +2     
  Lines        4687     4713      +26     
  Branches     1022     1016       -6     
==========================================
- Hits         4046     3638     -408     
- Misses        456      900     +444     
+ Partials      185      175      -10     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/816?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Coverage Δ | |
|---|---|---|
| [signac/\_synced\_collections/\_\_init\_\_.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvX19pbml0X18ucHk=) | `100.00% <ø> (ø)` | |
| [signac/\_synced\_collections/\_caching.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvX2NhY2hpbmcucHk=) | `0.00% <0.00%> (ø)` | |
| [signac/\_synced\_collections/backends/\_\_init\_\_.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvX19pbml0X18ucHk=) | `100.00% <ø> (ø)` | |
| [...\_synced\_collections/backends/collection\_mongodb.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl9tb25nb2RiLnB5) | `52.27% <ø> (ø)` | |
| [...c/\_synced\_collections/backends/collection\_redis.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl9yZWRpcy5weQ==) | `65.62% <ø> (ø)` | |
| [...ac/\_synced\_collections/backends/collection\_zarr.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl96YXJyLnB5) | `54.00% <ø> (ø)` | |
| [signac/\_synced\_collections/buffers/\_\_init\_\_.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYnVmZmVycy9fX2luaXRfXy5weQ==) | `100.00% <ø> (ø)` | |
| [...\_collections/buffers/memory\_buffered\_collection.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYnVmZmVycy9tZW1vcnlfYnVmZmVyZWRfY29sbGVjdGlvbi5weQ==) | `100.00% <ø> (ø)` | |
| [signac/\_synced\_collections/data\_types/\_\_init\_\_.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvZGF0YV90eXBlcy9fX2luaXRfXy5weQ==) | `100.00% <ø> (ø)` | |
| [signac/\_synced\_collections/data\_types/attr\_dict.py](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvZGF0YV90eXBlcy9hdHRyX2RpY3QucHk=) | `100.00% <ø> (ø)` | |
| ... and [70 more](https://codecov.io/gh/glotzerlab/signac/pull/816/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)

vyasr: > I haven't tested this locally but I'm happy with the code in this PR! I'll test it with v1 projects on the `next` branch sometime in the near future.

I'll hold off on merging to let you try a test or two in case you catch bugs that I missed in my testing.
bdice: @vyasr Maybe we should wait until I rebase `next` and then rebase this PR?
vyasr: > @vyasr Maybe we should wait until I rebase `next` and then rebase this PR?

Yes that was my plan. I was also hoping to get one or two additional testers aside from myself, but I think once the rebase is done if nobody has done additional testing at that point we can just go ahead and merge this and let people test it on the main branch.
vyasr: @bdice @mikemhenry if one of you would like to test this locally that would be great, otherwise I think we can go ahead and just merge this.
vyasr: I'm going to go ahead and merge this. People can test it out on next.

Patch:
diff --git a/signac/__main__.py b/signac/__main__.py
index a450e534..576512d7 100644
--- a/signac/__main__.py
+++ b/signac/__main__.py
@@ -634,35 +634,36 @@ def main_update_cache(args):
         _print_err(f"Updated cache (size={n}).")
 
 
-# UNCOMMENT THE FOLLOWING BLOCK WHEN THE FIRST MIGRATION IS INTRODUCED.
-# def main_migrate(args):
-#     "Migrate the project's schema to the current schema version."
-#     from .contrib.migration import apply_migrations, _get_config_schema_version
-#     from packaging import version
-#     from .version import SCHEMA_VERSION
-#     project = get_project(_ignore_schema_version=True)
-#
-#     root = args.root_directory if args.root_directory else os.getcwd()
-#
-#     schema_version = version.parse(SCHEMA_VERSION)
-#     config_schema_version = _get_config_schema_version(root, schema_version)
-#
-#     if config_schema_version > schema_version:
-#         _print_err(
-#             "The schema version of the project ({}) is newer than the schema "
-#             "version supported by signac version {}: {}. Try updating signac.".format(
-#                 config_schema_version, __version__, schema_version))
-#     elif config_schema_version == schema_version:
-#         _print_err(
-#             "The schema version of the project ({}) is up to date. "
-#             "Nothing to do.".format(config_schema_version))
-#     elif args.yes or _query_yes_no(
-#         "Do you want to migrate this project's schema version from '{}' to '{}'? "
-#         "WARNING: THIS PROCESS IS IRREVERSIBLE!".format(
-#             config_schema_version, schema_version), 'no'):
-#         apply_migrations(root)
-#
-#
+def main_migrate(args):
+    """Migrate the project's schema to the current schema version."""
+    from .contrib.migration import _get_config_schema_version, apply_migrations
+    from .version import SCHEMA_VERSION
+
+    root = args.root_directory if args.root_directory else os.getcwd()
+
+    schema_version = int(SCHEMA_VERSION)
+    config_schema_version = _get_config_schema_version(root, schema_version)
+
+    if config_schema_version > schema_version:
+        _print_err(
+            f"The schema version of the project ({config_schema_version}) is "
+            "newer than the schema version {schema_version} supported by "
+            f"signac version {__version__}. Try updating signac."
+        )
+    elif config_schema_version == schema_version:
+        _print_err(
+            f"The schema version of the project ({config_schema_version}) is "
+            "up to date. Nothing to do."
+        )
+    elif args.yes or _query_yes_no(
+        "Do you want to migrate this project's schema version from "
+        f"'{config_schema_version}' to '{schema_version}'? WARNING: THIS "
+        "PROCESS IS IRREVERSIBLE!",
+        "no",
+    ):
+        apply_migrations(root)
+
+
 def verify_config(cfg, preserve_errors=True):
     """Verify provided configuration."""
     verification = cfg.verify(preserve_errors=preserve_errors)
@@ -1573,19 +1574,25 @@ def main():
     parser_verify = config_subparsers.add_parser("verify")
     parser_verify.set_defaults(func=main_config_verify)
 
-    # UNCOMMENT THE FOLLOWING BLOCK WHEN THE FIRST MIGRATION IS INTRODUCED.
-    # parser_migrate = subparsers.add_parser(
-    #     'migrate',
-    #     description="Irreversibly migrate this project's schema version to the "
-    #                 "supported version.")
-    # parser_migrate.add_argument(
-    #     "-r",
-    #     "--root-directory",
-    #     type=str,
-    #     default='',
-    #     help="The path to the project.",
-    # )
-    # parser_migrate.set_defaults(func=main_migrate)
+    parser_migrate = subparsers.add_parser(
+        "migrate",
+        description="Irreversibly migrate this project's schema version to the "
+        "supported version.",
+    )
+    parser_migrate.add_argument(
+        "-r",
+        "--root-directory",
+        type=str,
+        default="",
+        help="The path to the project.",
+    )
+    parser_migrate.add_argument(
+        "-y",
+        "--yes",
+        action="store_true",
+        help="Do not ask for confirmation.",
+    )
+    parser_migrate.set_defaults(func=main_migrate)
 
     # This is a hack, as argparse itself does not
     # allow to parse only --version without any
diff --git a/signac/common/config.py b/signac/common/config.py
index 2a1663c3..10d7be40 100644
--- a/signac/common/config.py
+++ b/signac/common/config.py
@@ -17,14 +17,47 @@ logger = logging.getLogger(__name__)
 PROJECT_CONFIG_FN = os.path.join(".signac", "config")
 USER_CONFIG_FN = os.path.expanduser(os.path.join("~", ".signacrc"))
 
-# TODO: Consider making this entire module internal and removing all its
-# functions from the public API.
-
 
 def _get_project_config_fn(path):
     return os.path.abspath(os.path.join(path, PROJECT_CONFIG_FN))
 
 
+def _raise_if_older_schema(root):
+    """Raise if an older schema version is detected at the search path.
+
+    Parameters
+    ----------
+    root : str
+        Directory to check schema for.
+
+    Raises
+    ------
+    IncompatibleSchemaVersion
+        If the project uses an older schema version that requires migration.
+    """
+    from ..contrib.errors import IncompatibleSchemaVersion
+    from ..contrib.migration import _get_config_schema_version
+    from ..version import SCHEMA_VERSION, __version__
+
+    schema_version = int(SCHEMA_VERSION)
+
+    try:
+        schema_version = _get_config_schema_version(root, schema_version)
+        assert schema_version != int(SCHEMA_VERSION), (
+            "Migration schema loader succeeded in loading a config file "
+            "where normal loader failed. This indicates an internal "
+            "error, please contact the signac developers."
+        )
+        raise IncompatibleSchemaVersion(
+            "The signac schema version used by this project is "
+            f"{schema_version}, but signac {__version__} requires "
+            f"schema version {SCHEMA_VERSION}. Try running python -m "
+            "signac migrate."
+        )
+    except RuntimeError:
+        pass
+
+
 def _locate_config_dir(search_path):
     """Locates directory containing a signac configuration file in a directory hierarchy.
 
@@ -38,10 +71,24 @@ def _locate_config_dir(search_path):
     str or None
         The directory containing the configuration file if one is found, otherwise None.
     """
+    orig_search_path = search_path
     search_path = os.path.abspath(search_path)
     while True:
         if os.path.isfile(_get_project_config_fn(search_path)):
             return search_path
+        if (up := os.path.dirname(search_path)) == search_path:
+            break
+        else:
+            search_path = up
+
+    logger.debug(
+        "Reached filesystem root, no config found. Checking whether a "
+        "project created with an older signac schema may be found."
+    )
+
+    search_path = os.path.abspath(orig_search_path)
+    while True:
+        _raise_if_older_schema(search_path)
         if (up := os.path.dirname(search_path)) == search_path:
             logger.debug("Reached filesystem root, no config found.")
             return None
diff --git a/signac/contrib/migration/v0_to_v1.py b/signac/contrib/migration/v0_to_v1.py
index 3ee6634b..f2ed52c2 100644
--- a/signac/contrib/migration/v0_to_v1.py
+++ b/signac/contrib/migration/v0_to_v1.py
@@ -19,9 +19,12 @@ workspace_dir = string(default='workspace')
 
 
 def _load_config_v1(root_directory):
-    cfg = configobj.ConfigObj(
-        os.path.join(root_directory, "signac.rc"), configspec=_cfg.split("\n")
-    )
+    config_fn = os.path.join(root_directory, "signac.rc")
+    if not os.path.isfile(config_fn):
+        raise RuntimeError(
+            f"The directory {root_directory} does not contain a config file."
+        )
+    cfg = configobj.ConfigObj(config_fn, configspec=_cfg.split("\n"))
     validator = configobj.validate.Validator()
     if cfg.validate(validator) is not True:
         raise RuntimeError(
diff --git a/signac/contrib/migration/v1_to_v2.py b/signac/contrib/migration/v1_to_v2.py
index 39942c5f..6f55f616 100644
--- a/signac/contrib/migration/v1_to_v2.py
+++ b/signac/contrib/migration/v1_to_v2.py
@@ -29,9 +29,12 @@ schema_version = string(default='0')
 
 
 def _load_config_v2(root_directory):
-    cfg = configobj.ConfigObj(
-        os.path.join(root_directory, ".signac", "config"), configspec=_cfg.split("\n")
-    )
+    config_fn = os.path.join(root_directory, ".signac", "config")
+    if not os.path.isfile(config_fn):
+        raise RuntimeError(
+            f"The directory {root_directory} does not contain a config file."
+        )
+    cfg = configobj.ConfigObj(config_fn, configspec=_cfg.split("\n"))
     validator = configobj.validate.Validator()
     if cfg.validate(validator) is not True:
         raise RuntimeError(
diff --git a/signac/contrib/project.py b/signac/contrib/project.py
index a8e49e6c..3ba72d6c 100644
--- a/signac/contrib/project.py
+++ b/signac/contrib/project.py
@@ -25,6 +25,7 @@ from ..common.config import (
     _get_project_config_fn,
     _load_config,
     _locate_config_dir,
+    _raise_if_older_schema,
     _read_config_file,
 )
 from ..core.h5store import H5StoreManager
@@ -104,6 +105,7 @@ class Project:
         if path is None:
             path = os.getcwd()
         if not os.path.isfile(_get_project_config_fn(path)):
+            _raise_if_older_schema(path)
             raise LookupError(
                 f"Unable to find project at path '{os.path.abspath(path)}'."
             )
@@ -1435,13 +1437,6 @@ class Project:
         -------
         :class:`~signac.Project`
             Initialized project, an instance of :class:`~signac.Project`.
-
-        Raises
-        ------
-        RuntimeError
-            If the project path already contains a conflicting project
-            configuration.
-
         """
         if path is None:
             path = os.getcwd()


Test patch:
diff --git a/tests/test_project.py b/tests/test_project.py
index 34f0098d..c2e56cbb 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -2326,6 +2326,41 @@ class TestProjectSchema(TestProjectBase):
         assert len(migrations) == 0
 
 
+def _initialize_v1_project(dirname, with_workspace=True):
+    # Create v1 config file.
+    cfg_fn = os.path.join(dirname, "signac.rc")
+    workspace_dir = "workspace_dir"
+    with open(cfg_fn, "w") as f:
+        f.write(
+            textwrap.dedent(
+                f"""\
+                project = project
+                workspace_dir = {workspace_dir}
+                schema_version = 0"""
+            )
+        )
+
+    # Create a custom workspace
+    os.makedirs(os.path.join(dirname, workspace_dir))
+    if with_workspace:
+        os.makedirs(os.path.join(dirname, "workspace"))
+
+    # Create a shell history file.
+    history_fn = os.path.join(dirname, ".signac_shell_history")
+    with open(history_fn, "w") as f:
+        f.write("print(project)")
+
+    # Create a statepoint cache. Note that this cache does not
+    # correspond to actual statepoints since we don't currently have
+    # any in this project, but that's fine for migration testing.
+    sp_cache = os.path.join(dirname, ".signac_sp_cache.json.gz")
+    sp = {"a": 1}
+    with gzip.open(sp_cache, "wb") as f:
+        f.write(json.dumps({calc_id(sp): sp}).encode())
+
+    return cfg_fn
+
+
 class TestSchemaMigration:
     @pytest.mark.parametrize("implicit_version", [True, False])
     @pytest.mark.parametrize("workspace_exists", [True, False])
@@ -2333,36 +2368,7 @@ class TestSchemaMigration:
         from signac.contrib.migration import apply_migrations
 
         with TemporaryDirectory() as dirname:
-            # Create v1 config file.
-            cfg_fn = os.path.join(dirname, "signac.rc")
-            workspace_dir = "workspace_dir"
-            with open(cfg_fn, "w") as f:
-                f.write(
-                    textwrap.dedent(
-                        f"""\
-                        project = project
-                        workspace_dir = {workspace_dir}
-                        schema_version = 0"""
-                    )
-                )
-
-            # Create a custom workspace
-            os.makedirs(os.path.join(dirname, workspace_dir))
-            if workspace_exists:
-                os.makedirs(os.path.join(dirname, "workspace"))
-
-            # Create a shell history file.
-            history_fn = os.path.join(dirname, ".signac_shell_history")
-            with open(history_fn, "w") as f:
-                f.write("print(project)")
-
-            # Create a statepoint cache. Note that this cache does not
-            # correspond to actual statepoints since we don't currently have
-            # any in this project, but that's fine for migration testing.
-            history_fn = os.path.join(dirname, ".signac_sp_cache.json.gz")
-            sp = {"a": 1}
-            with gzip.open(history_fn, "wb") as f:
-                f.write(json.dumps({calc_id(sp): sp}).encode())
+            cfg_fn = _initialize_v1_project(dirname, workspace_exists)
 
             # If no schema version is present in the config it is equivalent to
             # version 0, so we test both explicit and implicit versions.
@@ -2394,6 +2400,17 @@ class TestSchemaMigration:
             assert os.path.isfile(project.fn(os.sep.join((".signac", "shell_history"))))
             assert os.path.isfile(project.fn(Project.FN_CACHE))
 
+    def test_project_init_old_schema(self):
+        with TemporaryDirectory() as dirname:
+            _initialize_v1_project(dirname)
+
+            # Initializing a project should detect the incompatible schema.
+            with pytest.raises(IncompatibleSchemaVersion):
+                signac.get_project(dirname)
+
+            with pytest.raises(IncompatibleSchemaVersion):
+                signac.Project(dirname)
+
 
 class TestProjectPickling(TestProjectBase):
     def test_pickle_project_empty(self):
diff --git a/tests/test_shell.py b/tests/test_shell.py
index 18848828..efa70676 100644
--- a/tests/test_shell.py
+++ b/tests/test_shell.py
@@ -9,6 +9,7 @@ import sys
 from tempfile import TemporaryDirectory
 
 import pytest
+from test_project import _initialize_v1_project
 
 import signac
 from signac.common import config
@@ -810,3 +811,12 @@ class TestBasicShell:
 
         err = self.call("python -m signac update-cache".split(), error=True)
         assert "Cache is up to date" in err
+
+    def test_migrate_v1_to_v2(self):
+        dirname = self.tmpdir.name
+        _initialize_v1_project(dirname, False)
+        self.call("python -m signac migrate --yes".split())
+        assert not os.path.isfile(os.path.join(dirname, "signac.rc"))
+        assert not os.path.isdir(os.path.join(dirname, "workspace_dir"))
+        assert os.path.isdir(os.path.join(dirname, ".signac"))
+        assert os.path.isdir(os.path.join(dirname, "workspace"))