diff --git a/conftest.py b/conftest.py index 9b975e40d..823e7fc53 100644 --- a/conftest.py +++ b/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 diff --git a/doc/changelog.rst b/doc/changelog.rst index ac88349fa..d57992f6d 100644 --- a/doc/changelog.rst +++ b/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 diff --git a/smartsim/settings/slurmSettings.py b/smartsim/settings/slurmSettings.py index 3a0780389..5ecb961b7 100644 --- a/smartsim/settings/slurmSettings.py +++ b/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(): diff --git a/tests/backends/test_dataloader.py b/tests/backends/test_dataloader.py index 3bde965e0..c43bf28cb 100644 --- a/tests/backends/test_dataloader.py +++ b/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) diff --git a/tests/test_batch_settings.py b/tests/test_batch_settings.py index 56ad6c3e9..dd672f51c 100644 --- a/tests/test_batch_settings.py +++ b/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 - \ No newline at end of file diff --git a/tests/test_slurm_settings.py b/tests/test_slurm_settings.py index 639d85713..c4abcbb1d 100644 --- a/tests/test_slurm_settings.py +++ b/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)