-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up environment before each test #270
Changes from 4 commits
babe795
ddc8b38
d66e7b1
edd5aa2
4af5ba5
e5af049
3fe26d8
dc7c09e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,11 +25,16 @@ | |
# 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 .base import BatchSettings, RunSettings | ||
|
||
from ..log import get_logger | ||
|
||
logger = get_logger(__name__) | ||
|
||
|
||
class SrunSettings(RunSettings): | ||
def __init__( | ||
|
@@ -277,14 +282,32 @@ 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this an explicit null check just in case a user explicitly exports an empty env var? Doing so will allow us to handle this scenario better export EMPTY=
unset DNE by still logging the warning for import os
assert os.environ.get("EMPTY") == ""
assert os.environ.get("DNE") is None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll have to revisit this when we add key-only exports. The current check would mark them as pre-existing and log a misleading warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick clarification based on a discussion between me and @ankona, could we make this current check: if preexisting is None:
... # issue warning to handle the empty-env-var case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MattToast I am almost 100% sure you meant if preexisting is not None:
... # issue warning and I agree, I was not considering the empty environment variable. @ankona yes, I think we'll have to revisit this for that case, but adding a more specific check now would make the code a bit confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦 hahahaha, it only took 3 devs, but I do think we ended up at the right clause for this if statement! |
||
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)] | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra white space There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I |
||
def format_comma_sep_env_vars(self) -> Tuple[str, List[str]]: | ||
"""Build environment variable string for Slurm | ||
|
||
|
@@ -295,7 +318,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(): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,8 @@ | |
# 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 | ||
|
@@ -245,6 +247,40 @@ def test_format_env_vars(): | |
assert all("SSKEYIN" not in x for x in formatted) | ||
|
||
|
||
def test_catch_existing_env_var(caplog): | ||
rs = SrunSettings( | ||
"python", | ||
env_vars={ | ||
"SMARTSIM_TEST_VAR": "B", | ||
}, | ||
) | ||
os.environ["SMARTSIM_TEST_VAR"] = "A" | ||
os.environ["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 | ||
|
||
os.environ.pop("SMARTSIM_TEST_VAR", None) | ||
os.environ.pop("SMARTSIM_TEST_CSVAR", None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incredibly minor refactor request, don't feel the need to make this change if you don't want: If you'd like, (Shouldn't matter unless someone sources pytest AND has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for introducing me to the joys of |
||
|
||
|
||
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) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use
monkeypatch
here just to make sure that the state of the env is restored after the test suite runs?Shouldn't matter unless someone decides to source pytest, but never hurts to be safe, lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Monkeys are everywhere.