Add latest changes from master branch

Merge branch 'master' into 466-esm1p5-cice-startdate-fix

payu/branch.py

@@ -13,7 +13,7 @@
 from typing import Optional
 import shutil
 
-from ruamel.yaml import YAML, CommentedMap
+from ruamel.yaml import YAML, CommentedMap, constructor
 import git
 
 from payu.fsops import read_config, DEFAULT_CONFIG_FNAME, list_archive_dirs
@@ -67,8 +67,18 @@ def add_restart_to_config(restart_path: Path, config_path: Path) -> None:
     restart in archive"""
 
     # Default ruamel yaml preserves comments and multiline strings
-    yaml = YAML()
-    config = yaml.load(config_path)
+    try:
+        yaml = YAML()
+        config = yaml.load(config_path)
+    except constructor.DuplicateKeyError as e:
+        # PyYaml which is used to read config allows duplicate keys
+        # These will get removed when the configuration file is modified
+        warnings.warn(
+            "Removing any subsequent duplicate keys from config.yaml"
+        )
+        yaml = YAML()
+        yaml.allow_duplicate_keys = True
+        config = yaml.load(config_path)
 
     # Add in restart path
     config['restart'] = str(restart_path)

payu/cli.py

@@ -15,6 +15,7 @@
 import shlex
 import subprocess
 import sys
+import warnings
 
 import payu
 import payu.envmod as envmod
@@ -23,10 +24,17 @@
 from payu.schedulers import index as scheduler_index
 import payu.subcommands
 
-
 # Default configuration
 DEFAULT_CONFIG = 'config.yaml'
 
+# Force warnings.warn() to omit the source code line in the message
+formatwarning_orig = warnings.formatwarning
+warnings.formatwarning = (
+    lambda message, category, filename, lineno, line=None: (
+        formatwarning_orig(message, category, filename, lineno, line='')
+    )
+)
+
 
 def parse():
     """Parse the command line inputs and execute the subcommand."""

payu/envmod.py

@@ -159,24 +159,8 @@ def setup_user_modules(user_modules, user_modulepaths):
     previous_path = os.environ.get('PATH', '')
 
     for modulefile in user_modules:
-        # Check module exists and there is not multiple available
-        output = run_module_cmd("avail --terse", modulefile).stderr
-
-        # Extract out the modulefiles available - strip out lines like:
-        # /apps/Modules/modulefiles:
-        modules = [line for line in output.strip().splitlines()
-                   if not (line.startswith('/') and line.endswith(':'))]
-
-        # Modules are used for finding model executable paths - so check
-        # for unique module
-        if len(modules) > 1:
-            raise ValueError(
-                f"There are multiple modules available for {modulefile}:\n" +
-                f"{output}\n{MULTIPLE_MODULES_HELP}")
-        elif len(modules) == 0:
-            raise ValueError(
-                f"Module is not found: {modulefile}\n{MODULE_NOT_FOUND_HELP}"
-            )
+        # Check modulefile exists and is unique or has an exact match
+        check_modulefile(modulefile)
 
         # Load module
         module('load', modulefile)
@@ -191,6 +175,31 @@ def setup_user_modules(user_modules, user_modulepaths):
     return (loaded_modules, paths)
 
 
+def check_modulefile(modulefile: str) -> None:
+    """Given a modulefile, check if modulefile exists, and there is
+    a unique modulefile available - e.g. if it's version is specified"""
+    output = run_module_cmd("avail --terse", modulefile).stderr
+
+    # Extract out the modulefiles available - strip out lines like:
+    # /apps/Modules/modulefiles:
+    modules_avail = [line for line in output.strip().splitlines()
+                     if not (line.startswith('/') and line.endswith(':'))]
+
+    # Remove () from end of modulefiles if they exist, e.g. (default)
+    modules_avail = [mod.rsplit('(', 1)[0] for mod in modules_avail]
+
+    # Modules are used for finding model executable paths - so check
+    # for unique module, or an exact match for the modulefile name
+    if len(modules_avail) > 1 and modules_avail.count(modulefile) != 1:
+        raise ValueError(
+            f"There are multiple modules available for {modulefile}:\n" +
+            f"{output}\n{MULTIPLE_MODULES_HELP}")
+    elif len(modules_avail) == 0:
+        raise ValueError(
+            f"Module is not found: {modulefile}\n{MODULE_NOT_FOUND_HELP}"
+        )
+
+
 def run_module_cmd(subcommand, *args):
     """Wrapper around subprocess module command that captures output"""
     modulecmd = f"{os.environ['MODULESHOME']}/bin/modulecmd bash"

payu/fsops.py

@@ -17,6 +17,7 @@
 import shlex
 import subprocess
 from typing import Union, List
+import warnings
 
 # Extensions
 import yaml
@@ -62,6 +63,27 @@ def movetree(src, dst, symlinks=False):
     shutil.rmtree(src)
 
 
+class DuplicateKeyWarnLoader(yaml.SafeLoader):
+    def construct_mapping(self, node, deep=False):
+        """Add warning for duplicate keys in yaml file, as currently
+        PyYAML overwrites duplicate keys even though in YAML, keys
+        are meant to be unique
+        """
+        mapping = {}
+        for key_node, value_node in node.value:
+            key = self.construct_object(key_node, deep=deep)
+            value = self.construct_object(value_node, deep=deep)
+            if key in mapping:
+                warnings.warn(
+                    "Duplicate key found in config.yaml: "
+                    f"key '{key}' with value '{value}'. "
+                    f"This overwrites the original value: '{mapping[key]}'"
+                )
+            mapping[key] = value
+
+        return super().construct_mapping(node, deep)
+
+
 def read_config(config_fname=None):
     """Parse input configuration file and return a config dict."""
 
@@ -70,7 +92,7 @@ def read_config(config_fname=None):
 
     try:
         with open(config_fname, 'r') as config_file:
-            config = yaml.safe_load(config_file)
+            config = yaml.load(config_file, Loader=DuplicateKeyWarnLoader)
 
         # NOTE: A YAML file with no content returns `None`
         if config is None:

payu/runlog.py

@@ -12,6 +12,7 @@
 import shlex
 import subprocess
 import sys
+import warnings
 
 # Third party
 import requests
@@ -95,9 +96,18 @@ def commit(self):
         print(cmd)
         try:
             subprocess.check_call(shlex.split(cmd), stdout=f_null,
-                                  cwd=self.expt.control_path)
+                                  stderr=f_null, cwd=self.expt.control_path)
         except subprocess.CalledProcessError:
-            print('TODO: Check if commit is unchanged')
+            # Attempt commit without signing commits
+            cmd = f'git commit --no-gpg-sign -am "{commit_msg}"'
+            print(cmd)
+            try:
+                subprocess.check_call(shlex.split(cmd),
+                                      stdout=f_null,
+                                      cwd=self.expt.control_path)
+                warnings.warn("Runlog commit was commited without git signing")
+            except subprocess.CalledProcessError:
+                warnings.warn("Error occured when attempting to commit runlog")
 
         # Save the commit hash
         self.expt.run_id = commit_hash(self.expt.control_path)

test/test_envmod.py

@@ -0,0 +1,105 @@
+import pytest
+from unittest.mock import patch
+
+from payu.envmod import check_modulefile
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_unique(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0
+"""
+    # Test runs without an error
+    check_modulefile('test-module/1.0.0')
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_without_version(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0
+test-module/2.0.0
+test-module/3.0.1
+"""
+
+    # Expect an error raised
+    with pytest.raises(ValueError) as exc_info:
+        check_modulefile('test-module')
+        exc_info.value.startswith(
+            "There are multiple modules available for test-module"
+        )
+
+    # Mock module avail command use debug in name
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0
+test-module/1.0.0-debug
+"""
+
+    # Expect an error raised
+    with pytest.raises(ValueError) as exc_info:
+        check_modulefile('test-module')
+        exc_info.value.startswith(
+            "There are multiple modules available for test-module"
+        )
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_exact_match(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0
+test-module/1.0.0-debug
+"""
+
+    # Test runs without an error
+    check_modulefile('test-module/1.0.0')
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_exact_match_with_symbolic_version(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0(default)
+test-module/1.0.0-debug
+"""
+
+    # Test runs without an error
+    check_modulefile('test-module/1.0.0')
+
+    # Rerun test with another symbolic version/alias other than default
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0(some_symbolic_name_or_alias)
+test-module/1.0.0-debug
+"""
+
+    # Test runs without an error
+    check_modulefile('test-module/1.0.0')
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_multiple_modules(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = """/path/to/modulefiles:
+test-module/1.0.0
+/another/module/path:
+test-module/1.0.0
+"""
+
+    # Expect an error raised
+    with pytest.raises(ValueError) as exc_info:
+        check_modulefile('test-module/1.0.0')
+        exc_info.value.startswith(
+            "There are multiple modules available for test-module"
+        )
+
+
+@patch('payu.envmod.run_module_cmd')
+def test_check_modulefile_no_modules_found(mock_module_cmd):
+    # Mock module avail command
+    mock_module_cmd.return_value.stderr = ""
+
+    # Expect an error raised
+    with pytest.raises(ValueError) as exc_info:
+        check_modulefile('test-module/1.0.0')
+        exc_info.value.startswith("Module is not found: test-module")

test/test_payu.py

@@ -1,6 +1,7 @@
 from io import StringIO
 import os
 from pathlib import Path
+import payu.branch
 import pytest
 import shutil
 import stat
@@ -432,3 +433,34 @@ def test_run_userscript_command(tmp_path):
 ])
 def test_needs_shell(command, expected):
     assert payu.fsops.needs_subprocess_shell(command) == expected
+
+
+def test_read_config_yaml_duplicate_key():
+    """The PyYAML library is used for reading config.yaml, but use ruamel yaml
+    is used in when modifying config.yaml as part of payu checkout
+    (ruamel is used to preserve comments and multi-line strings).
+    This led to bug #441, where pyyaml allowed duplicate keys but
+    ruamel.library raises an error
+    """
+    # Create a yaml file with a duplicate key
+    config_content = """
+pbs_flags: value1
+pbs_flags: value2
+"""
+    config_path = tmpdir / "config.yaml"
+    with config_path.open("w") as file:
+        file.write(config_content)
+
+    # Test read config passes without an error but a warning is raised
+    warn_msg = "Duplicate key found in config.yaml: key 'pbs_flags' with "
+    warn_msg += "value 'value2'. This overwrites the original value: 'value1'"
+    with pytest.warns(UserWarning, match=warn_msg):
+        payu.fsops.read_config(config_path)
+
+    restart_path = tmpdir / "restarts"
+
+    # Test add restart to config.yaml does not fail with an error, but
+    # still raises another warning that duplicates keys will be deleted
+    warn_msg = "Removing any subsequent duplicate keys from config.yaml"
+    with pytest.warns(UserWarning, match=warn_msg):
+        payu.branch.add_restart_to_config(restart_path, config_path)