refactor(models): distinguish source repo via prefix (#202)

Add a prefix to model names identifying which repository the model is from (i.e., what "kind" of model it is):

- example/... example models from https://github.com/MODFLOW-ORG/modflow6-examples
- test/... test models from https://github.com/MODFLOW-ORG/modflow6-testmodels (only mf6)
- large/... models from https://github.com/MODFLOW-ORG/modflow6-largetestmodels

This will allow more easily selecting subsets of models by the repository they are from as we do in MF6 autotests.

I considered adding an "mf6" prefix to all these (useful if we want to expose older models, e.g. from the mf5to6 folder in the test models repo). I also considered making the prefix identical to the repository name. Not sure which way is best, this will do for now. Anyone reading this with an opinion, please make it known.

Also, remove the auto-generated model functions (doesn't play nice with "namespaced" i.e. "/"-delimited model names), tweak the registry introspection functions for consistency, and update some docstrings.

Author: wpbonelli

autotest/test_models.py

@@ -21,28 +21,35 @@ def test_registry():
     assert any(registry), "Registry is empty"
 
 
-@pytest.mark.parametrize("model_name, files", MODELMAP.items())
+@pytest.mark.parametrize(
+    "model_name, files", MODELMAP.items(), ids=list(MODELMAP.keys())
+)
 def test_models(model_name, files):
-    model_names = models.list_models()
+    model_names = list(models.get_models().keys())
     assert model_name in model_names, f"Model {model_name} not found in model map"
     assert files == models.MODELMAP[model_name], (
         f"Files for model {model_name} do not match"
     )
-    assert hasattr(models, model_name), (
-        f"Function {model_name} not found in models module"
-    )
     if "mf6" in model_name:
         assert any(Path(f).name == "mfsim.nam" for f in files)
 
 
-@pytest.mark.parametrize("example_name, model_names", models.get_examples().items())
+@pytest.mark.parametrize(
+    "example_name, model_names",
+    models.get_examples().items(),
+    ids=list(models.get_examples().keys()),
+)
 def test_get_examples(example_name, model_names):
     assert example_name in models.EXAMPLES
     for model_name in model_names:
         assert model_name in models.MODELMAP
 
 
-@pytest.mark.parametrize("model_name, files", list(islice(MODELMAP.items(), TAKE)))
+@pytest.mark.parametrize(
+    "model_name, files",
+    list(islice(MODELMAP.items(), TAKE)),
+    ids=list(MODELMAP.keys())[:TAKE],
+)
 def test_copy_to(model_name, files, tmp_path):
     workspace = models.copy_to(tmp_path, model_name, verbose=True)
     assert workspace.exists(), f"Model {model_name} was not copied to {tmp_path}"
@@ -54,21 +61,3 @@ def test_copy_to(model_name, files, tmp_path):
     )
     if "mf6" in model_name:
         assert any(Path(f).name == "mfsim.nam" for f in files)
-
-
-@pytest.mark.parametrize("model_name, files", list(islice(MODELMAP.items(), TAKE)))
-def test_generated_functions_return_files(model_name, files):
-    model_function = getattr(models, model_name)
-    fetched_files = model_function()
-    assert isinstance(fetched_files, list), (
-        f"Function {model_name} did not return a list"
-    )
-    assert len(fetched_files) == len(files), (
-        f"Function {model_name} did not return the correct number of files"
-    )
-    if "mf6" in model_name:
-        assert any(Path(f).name == "mfsim.nam" for f in files)
-    for fetched_file in fetched_files:
-        assert Path(fetched_file).exists(), (
-            f"Fetched file {fetched_file} does not exist"
-        )

docs/md/models.md

@@ -1,15 +1,13 @@
 # Models API
 
-The `modflow_devtools.models` module provides programmatic access to MODFLOW 6 example models via [Pooch](https://www.fatiando.org/pooch/latest/index.html).
-
-When the module is imported, it checks for the existence of the registry in models files. If they are found, it loads the registry and dynamically generates functions for each model, attaching them to the module namespace.
-
-Each function returns a list of files. Example usage:
+The `modflow_devtools.models` module provides programmatic access to MODFLOW 6 example models via [Pooch](https://www.fatiando.org/pooch/latest/index.html). Example usage:
 
 ```python
 import modflow_devtools.models as models
+from flopy.mf6 import MFSimulation
 
-files = models.some_model()
+workspace = models.copy_to("some/path", "some_model")
+sim = MFSimulation.load(sim_ws=workspace)
 ```
 
 ## Developers
@@ -24,7 +22,7 @@ The script can be executed with `python -m modflow_devtools.make_registry`. It a
 For example, to create a registry of models in the MF6 examples and test models repositories, assuming each is checked out next to this project:
 
 ```shell
-python -m modflow_devtools.make_registry ../modflow6-examples/examples --url https://github.com/MODFLOW-ORG/modflow6-examples/releases/download/current/mf6examples.zip
-python -m modflow_devtools.make_registry ../modflow6-testmodels --append --url https://github.com/MODFLOW-ORG/modflow6-testmodels/raw/master
-python -m modflow_devtools.make_registry ../modflow6-largetestmodels --append --url https://github.com/MODFLOW-ORG/modflow6-largetestmodels/raw/master
+python -m modflow_devtools.make_registry ../modflow6-examples/examples --url https://github.com/MODFLOW-ORG/modflow6-examples/releases/download/current/mf6examples.zip --prefix example
+python -m modflow_devtools.make_registry ../modflow6-testmodels/mf6 --append --url https://github.com/MODFLOW-ORG/modflow6-testmodels/raw/master/mf6 --prefix test
+python -m modflow_devtools.make_registry ../modflow6-largetestmodels --append --url https://github.com/MODFLOW-ORG/modflow6-largetestmodels/raw/master --prefix large
 ```

modflow_devtools/make_registry.py

@@ -32,6 +32,7 @@ def _sha256(path: Path) -> str:
 def write_registry(
     path: str | PathLike,
     url: str,
+    prefix: str = "",
     append: bool = False,
 ):
     path = Path(path).expanduser().absolute()
@@ -47,18 +48,16 @@ def write_registry(
 
     model_paths = get_model_paths(path)
     for model_path in model_paths:
-        # TODO: the renaming is only necessary because we're attaching auto-
-        # generated functions to the models module. If we can live without,
-        # and get by with a single function that takes model name as an arg,
-        # then the model names could correspond directly to directory names.
         model_path = model_path.expanduser().absolute()
         rel_path = model_path.relative_to(path)
-        model_name = "/".join(rel_path.parts)
+        parts = [prefix, *list(rel_path.parts)] if prefix else list(rel_path.parts)
+        model_name = "/".join(parts)
         modelmap[model_name] = []
         if is_zip:
-            if rel_path.parts[0] not in examples:
-                examples[rel_path.parts[0]] = []
-            examples[rel_path.parts[0]].append(model_name)
+            name = rel_path.parts[0]
+            if name not in examples:
+                examples[name] = []
+            examples[name].append(model_name)
         for p in model_path.rglob("*"):
             if not p.is_file() or any(e in p.name for e in exclude):
                 continue
@@ -95,21 +94,23 @@ def drop_none_or_empty(path, key, value):
 
 
 if __name__ == "__main__":
-    parser = argparse.ArgumentParser(description="Make a registry of example models.")
+    parser = argparse.ArgumentParser(description="Make a registry of models.")
     parser.add_argument("path")
     parser.add_argument(
         "--append",
         "-a",
         action="store_true",
         help="Append instead of overwriting.",
     )
+    parser.add_argument(
+        "--prefix", "-p", type=str, help="Prefix for models.", default=""
+    )
     parser.add_argument(
         "--url",
         "-u",
         type=str,
-        help="Base URL for example models.",
+        help="Base URL for models.",
+        default=BASE_URL,
     )
     args = parser.parse_args()
-    path = Path(args.path)
-    url = args.url if args.url else BASE_URL
-    write_registry(path=path, url=url, append=args.append)
+    write_registry(path=args.path, url=args.url, prefix=args.prefix, append=args.append)

modflow_devtools/models.py

@@ -81,9 +81,7 @@ def _fetch_zip(zip_name):
     with pkg_resources.open_binary(REGISTRY_ANCHOR, MODELMAP_FILE_NAME) as models_file:
         MODELMAP = tomli.load(models_file)
         for model_name, files in MODELMAP.items():
-            fetch = _fetch(model_name, files)
-            FETCHERS[model_name] = fetch
-            globals()[model_name] = fetch
+            FETCHERS[model_name] = _fetch(model_name, files)
 except:  # noqa: E722
     warn(
         f"No model mapping file '{MODELMAP_FILE_NAME}' "
@@ -104,21 +102,35 @@ def _fetch_zip(zip_name):
 
 
 def get_examples() -> dict[str, list[str]]:
+    """Get a map of example names to example scenarios."""
     return EXAMPLES
 
 
 def get_registry() -> dict[str, str]:
+    """
+    Get a map of file names to URLs. Note that this mapping
+    contains no information on which files belong to which
+    models. For that information, use `get_models()`.
+    """
     return REGISTRY
 
 
-def list_models() -> list[str]:
-    return list(MODELMAP.keys())
+def get_models() -> dict[str, str]:
+    """Get a map of model names to input files."""
+    return MODELMAP
 
 
 def copy_to(
     workspace: str | PathLike, model_name: str, verbose: bool = False
 ) -> Path | None:
-    if not any(files := FETCHERS[model_name]()):
+    """
+    Copy the model's input files to the given workspace.
+    The workspace will be created if it does not exist.
+    """
+
+    if (fetch := FETCHERS.get(model_name, None)) is None:
+        raise ValueError(f"Model '{model_name}' not in registry")
+    if not any(files := fetch()):
         return None
     # create the workspace if needed
     workspace = Path(workspace).expanduser().absolute()