Clean up environment before each test

conftest.py

@@ -411,6 +411,12 @@ def db_cluster(fileutils, wlmutils, request):
     exp.stop(db)
 
 
+@pytest.fixture(scope="function", autouse=True)
+def environment_cleanup(monkeypatch):
+    monkeypatch.delenv("SSDB", raising=False)
+    monkeypatch.delenv("SSKEYIN", raising=False)
+    monkeypatch.delenv("SSKEYOUT", raising=False)
+
 @pytest.fixture
 def dbutils():
     return DBUtils

doc/changelog.rst

@@ -22,6 +22,7 @@ This section details changes made in the development branch that have not yet be
 
 Description
 
+- Fix test suite behavior with environment variables
 - Update ML data loaders to make use of SmartRedis's aggregation lists
 - Drop support for Ray
 - Allow for models to be launched independently as batch jobs
@@ -32,6 +33,9 @@ Description
 
 Detailed Notes
 
+- Running some tests could result in some SmartSim-specific environment variables to be set. Such environment variables are now reset
+  after each test execution. Also, a warning for environment variable usage in Slurm was added, to make the user aware in case an environment
+  variable will not be assigned the desired value with `--export`. (PR270_)
 - The PyTorch and TensorFlow data loaders were update to make use of aggregation lists. This breaks their API, but makes them easier to use. (PR264_)
 - The support for Ray was dropped, as its most recent versions caused problems when deployed through SmartSim.
   We plan to release a separate add-on library to accomplish the same results. If
@@ -51,6 +55,7 @@ Detailed Notes
 - The release of RedisAI 1.2.7 allows us to update support for recent versions of PyTorch, Tensorflow, and ONNX (PR234_)
 - Make installation of correct Torch backend more reliable according to instruction from PyTorch
 
+.. _PR270: https://github.com/CrayLabs/SmartSim/pull/270
 .. _PR264: https://github.com/CrayLabs/SmartSim/pull/264
 .. _PR263: https://github.com/CrayLabs/SmartSim/pull/263
 .. _PR258: https://github.com/CrayLabs/SmartSim/pull/258

smartsim/settings/slurmSettings.py

@@ -25,11 +25,15 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import datetime
+import os
 from typing import List, Tuple
 
 from ..error import SSUnsupportedError
+from ..log import get_logger
 from .base import BatchSettings, RunSettings
 
+logger = get_logger(__name__)
+
 
 class SrunSettings(RunSettings):
     def __init__(
@@ -277,12 +281,30 @@ def format_run_args(self):
                     opts += ["=".join((prefix + opt, str(value)))]
         return opts
 
+    def check_env_vars(self):
+        """Warn a user trying to set a variable which is set in the environment
+
+        Given Slurm's env var precedence, trying to export a variable which is already
+        present in the environment will not work.
+        """
+        for k, v in self.env_vars.items():
+            if "," not in str(v):
+                # If a variable is defined, it will take precedence over --export
+                # we warn the user
+                preexisting_var = os.environ.get(k, None)
+                if preexisting_var is not None:
+                    msg = f"Variable {k} is set to {preexisting_var} in current environment. "
+                    msg += f"If the job is running in an interactive allocation, the value {v} will not be set. "
+                    msg += "Please consider removing the variable from the environment and re-run the experiment."
+                    logger.warning(msg)
+
     def format_env_vars(self):
         """Build bash compatible environment variable string for Slurm
 
         :returns: the formatted string of environment variables
         :rtype: list[str]
         """
+        self.check_env_vars()
         return [f"{k}={v}" for k, v in self.env_vars.items() if "," not in str(v)]
 
     def format_comma_sep_env_vars(self) -> Tuple[str, List[str]]:
@@ -295,7 +317,7 @@ def format_comma_sep_env_vars(self) -> Tuple[str, List[str]]:
         :returns: the formatted string of environment variables
         :rtype: tuple[str, list[str]]
         """
-
+        self.check_env_vars()
         exportable_env, compound_env, key_only = [], [], []
 
         for k, v in self.env_vars.items():

tests/backends/test_dataloader.py

@@ -155,7 +155,7 @@ def train_tf(generator):
 @pytest.mark.skipif(not shouldrun_tf, reason="Test needs TensorFlow to run")
 def test_tf_dataloaders(fileutils, wlmutils):
     test_dir = fileutils.make_test_dir()
-    exp = Experiment("test_tf_dataloaders", test_dir)
+    exp = Experiment("test_tf_dataloaders", test_dir, launcher=wlmutils.get_test_launcher())
     orc: Orchestrator = wlmutils.get_orchestrator()
     exp.generate(orc)
     exp.start(orc)
@@ -318,7 +318,7 @@ def test_data_info_repr():
 )
 def test_wrong_dataloaders(fileutils, wlmutils):
     test_dir = fileutils.make_test_dir()
-    exp = Experiment("test-wrong-dataloaders", exp_path=test_dir)
+    exp = Experiment("test-wrong-dataloaders", exp_path=test_dir, launcher=wlmutils.get_test_launcher())
     orc = wlmutils.get_orchestrator()
     exp.generate(orc)
     exp.start(orc)

tests/test_batch_settings.py

@@ -26,6 +26,7 @@
 
 
 import pytest
+
 from smartsim.settings import BsubBatchSettings, QsubBatchSettings, SbatchSettings
 from smartsim.settings.settings import create_batch_settings
 
@@ -87,12 +88,12 @@ def test_create_bsub():
 
 
 @pytest.mark.parametrize(
-    "batch_args", 
+    "batch_args",
     [
         pytest.param({"core_isolation": None}, id="null batch arg"),
         pytest.param({"core_isolation": "abc"}, id="valued batch arg"),
         pytest.param({"core_isolation": None, "xyz": "pqr"}, id="multi batch arg"),
-    ]
+    ],
 )
 def test_stringification(batch_args):
     """Ensure stringification includes expected fields"""
@@ -136,4 +137,3 @@ def test_preamble():
 
     bsub.add_preamble(["first line", "last line"])
     assert len(bsub._preamble) == 4
-

tests/test_slurm_settings.py

@@ -24,6 +24,9 @@
 # OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import logging
+import os
+
 import pytest
 
 from smartsim.error import SSUnsupportedError
@@ -208,7 +211,7 @@ def test_mpmd_non_compound_no_exports():
     srun = SrunSettings("printenv")
     srun.in_batch = True
     srun.alloc = 12345
-    srun.env_vars = {} 
+    srun.env_vars = {}
     srun_2 = SrunSettings("printenv")
     srun_2.env_vars = {}
     srun.make_mpmd(srun_2)
@@ -245,6 +248,37 @@ def test_format_env_vars():
     assert all("SSKEYIN" not in x for x in formatted)
 
 
+def test_catch_existing_env_var(caplog, monkeypatch):
+    rs = SrunSettings(
+        "python",
+        env_vars={
+            "SMARTSIM_TEST_VAR": "B",
+        },
+    )
+    monkeypatch.setenv("SMARTSIM_TEST_VAR", "A")
+    monkeypatch.setenv("SMARTSIM_TEST_CSVAR", "A,B")
+    caplog.clear()
+    rs.format_env_vars()
+
+    msg = f"Variable SMARTSIM_TEST_VAR is set to A in current environment. "
+    msg += f"If the job is running in an interactive allocation, the value B will not be set. "
+    msg += "Please consider removing the variable from the environment and re-running the experiment."
+
+    for record in caplog.records:
+        assert record.levelname == "WARNING"
+        assert record.message == msg
+
+    caplog.clear()
+
+    env_vars = {"SMARTSIM_TEST_VAR": "B", "SMARTSIM_TEST_CSVAR": "C,D"}
+    settings = SrunSettings("python", env_vars=env_vars)
+    settings.format_comma_sep_env_vars()
+
+    for record in caplog.records:
+        assert record.levelname == "WARNING"
+        assert record.message == msg
+
+
 def test_format_comma_sep_env_vars():
     env_vars = {"OMP_NUM_THREADS": 20, "LOGGING": "verbose", "SSKEYIN": "name_0,name_1"}
     settings = SrunSettings("python", env_vars=env_vars)