Skip to content

Commit 358568e

Browse files
authored
Merge branch 'LLNL:develop' into develop
2 parents b44e285 + 297d9d5 commit 358568e

File tree

6 files changed

+262
-125
lines changed

6 files changed

+262
-125
lines changed

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1313
- Added tests for the `dict_deep_merge` function
1414
- Pytest-mock as a dependency for the test suite (necessary for using mocks and fixtures in the same test)
1515
- New github action test to make sure target branch has been merged into the source first, so we know histories are ok
16+
- Check in the status commands to make sure we're not pulling statuses from nested workspaces
17+
- Added `setuptools` as a requirement for python 3.12 to recognize the `pkg_resources` library
1618

1719
### Changed
1820
- `merlin info` is cleaner and gives python package info
1921
- merlin version now prints with every banner message
22+
- Applying filters for `merlin detailed-status` will now log debug statements instead of warnings
23+
- Modified the unit tests for the `merlin status` command to use pytest rather than unittest
24+
- Added fixtures for `merlin status` tests that copy the workspace to a temporary directory so you can see exactly what's run in a test
2025

2126
### Fixed
2227
- Bugfix for output of `merlin example openfoam_wf_singularity`
2328
- A bug with the CHANGELOG detection test when the target branch isn't in the ci runner history
2429
- Link to Merlin banner in readme
30+
- Issue with escape sequences in ascii art (caught by python 3.12)
2531

2632

2733
## [1.12.1]

merlin/ascii_art.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,11 @@
9696
9797
9898
__ __ _ _
99-
| \/ | | (_)
100-
| \ / | ___ _ __| |_ _ __
101-
| |\/| |/ _ \ '__| | | '_ \
99+
| \\/ | | (_)
100+
| \\ / | ___ _ __| |_ _ __
101+
| |\\/| |/ _ \\ '__| | | '_ \\
102102
| | | | __/ | | | | | | |
103-
|_| |_|\___|_| |_|_|_| |_|
103+
|_| |_|\\___|_| |_|_|_| |_|
104104
105105
Machine Learning for HPC Workflows
106106

merlin/study/status.py

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,8 @@ def get_step_statuses(self, step_workspace: str, started_step_name: str) -> Dict
344344
Given a step workspace and the name of the step, read in all the statuses
345345
for the step and return them in a dict.
346346
347-
:param `step_workspace`: The path to the step we're going to read statuses from
347+
:param step_workspace: The path to the step we're going to read statuses from
348+
:param started_step_name: The name of the step that we're gathering statuses for
348349
:returns: A dict of statuses for the given step
349350
"""
350351
step_statuses = {}
@@ -354,7 +355,12 @@ def get_step_statuses(self, step_workspace: str, started_step_name: str) -> Dict
354355

355356
# Traverse the step workspace and look for MERLIN_STATUS files
356357
LOG.debug(f"Traversing '{step_workspace}' to find MERLIN_STATUS.json files...")
357-
for root, _, _ in os.walk(step_workspace):
358+
for root, dirs, _ in os.walk(step_workspace, topdown=True):
359+
# Look for nested workspaces and skip them
360+
timestamp_regex = r"\d{8}-\d{6}$"
361+
curr_dir = os.path.split(root)[1]
362+
dirs[:] = [d for d in dirs if not re.search(timestamp_regex, curr_dir)]
363+
358364
# Search for a status file
359365
status_filepath = os.path.join(root, "MERLIN_STATUS.json")
360366
matching_files = glob(status_filepath)
@@ -869,8 +875,7 @@ def apply_filters(self):
869875
if matches_found == self.args.max_tasks:
870876
break
871877
else:
872-
# If our filters aren't a match for this task then delete it
873-
LOG.warning(f"No matching filter for '{sub_step_workspace}'.")
878+
LOG.debug(f"No matching filter for '{sub_step_workspace}'.")
874879

875880
# If we've hit the limit set by args.max_tasks, break out of the outer loop
876881
if matches_found == self.args.max_tasks:
@@ -1121,7 +1126,7 @@ def status_conflict_handler(*args, **kwargs) -> Any: # pylint: disable=W0613
11211126
11221127
There are currently 4 rules:
11231128
- string-concatenate: take the two conflicting values and concatenate them in a string
1124-
- use-initial-and-log-warning: use the value from dict_a and log a warning message
1129+
- use-dict_b-and-log-debug: use the value from dict_b and log a debug message
11251130
- use-longest-time: use the longest time between the two conflicting values
11261131
- use-max: use the larger integer between the two conflicting values
11271132
@@ -1136,8 +1141,8 @@ def status_conflict_handler(*args, **kwargs) -> Any: # pylint: disable=W0613
11361141
merge_rules = {
11371142
"task_queue": "string-concatenate",
11381143
"worker_name": "string-concatenate",
1139-
"status": "use-initial-and-log-warning",
1140-
"return_code": "use-initial-and-log-warning",
1144+
"status": "use-dict_b-and-log-debug",
1145+
"return_code": "use-dict_b-and-log-debug",
11411146
"elapsed_time": "use-longest-time",
11421147
"run_time": "use-longest-time",
11431148
"restarts": "use-max",
@@ -1150,13 +1155,13 @@ def status_conflict_handler(*args, **kwargs) -> Any: # pylint: disable=W0613
11501155

11511156
# params = self.spec.get_parameters()
11521157
# for token in params.parameters:
1153-
# merge_rules[token] = "use-initial-and-log-warning"
1158+
# merge_rules[token] = "use-dict_b-and-log-debug"
11541159

11551160
# Set parameter token key rules (commented for loop would be better but it's
11561161
# only possible if this conflict handler is contained within Status object; however,
11571162
# since this function needs to be imported outside of this file we can't do that)
11581163
if path is not None and "parameters" in path:
1159-
merge_rules[key] = "use-initial-and-log-warning"
1164+
merge_rules[key] = "use-dict_b-and-log-debug"
11601165

11611166
try:
11621167
merge_rule = merge_rules[key]
@@ -1168,13 +1173,13 @@ def status_conflict_handler(*args, **kwargs) -> Any: # pylint: disable=W0613
11681173

11691174
if merge_rule == "string-concatenate":
11701175
merge_val = f"{dict_a_val}, {dict_b_val}"
1171-
elif merge_rule == "use-initial-and-log-warning":
1172-
LOG.warning(
1173-
f"Conflict at key '{key}' while merging status files. Defaulting to initial value. "
1176+
elif merge_rule == "use-dict_b-and-log-debug":
1177+
LOG.debug(
1178+
f"Conflict at key '{key}' while merging status files. Using the updated value. "
11741179
"This could lead to incorrect status information, you may want to re-run in debug mode and "
11751180
"check the files in the output directory for this task."
11761181
)
1177-
merge_val = dict_a_val
1182+
merge_val = dict_b_val
11781183
elif merge_rule == "use-longest-time":
11791184
if dict_a_val == "--:--:--":
11801185
merge_val = dict_b_val

requirements/release.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,5 +9,6 @@ numpy
99
parse
1010
psutil>=5.1.0
1111
pyyaml>=5.1.2
12+
setuptools
1213
tabulate
1314
redis>=4.3.4

tests/fixtures/status.py

Lines changed: 112 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,23 @@
44
"""
55

66
import os
7+
import shutil
8+
from argparse import Namespace
79
from pathlib import Path
810

911
import pytest
12+
import yaml
1013

14+
from tests.unit.study.status_test_files import status_test_variables
1115

12-
@pytest.fixture(scope="class")
16+
17+
@pytest.fixture(scope="session")
1318
def status_testing_dir(temp_output_dir: str) -> str:
1419
"""
1520
A pytest fixture to set up a temporary directory to write files to for testing status.
1621
1722
:param temp_output_dir: The path to the temporary output directory we'll be using for this test run
23+
:returns: The path to the temporary testing directory for status testing
1824
"""
1925
testing_dir = f"{temp_output_dir}/status_testing/"
2026
if not os.path.exists(testing_dir):
@@ -23,15 +29,118 @@ def status_testing_dir(temp_output_dir: str) -> str:
2329
return testing_dir
2430

2531

26-
@pytest.fixture(scope="class")
32+
@pytest.fixture(scope="session")
2733
def status_empty_file(status_testing_dir: str) -> str: # pylint: disable=W0621
2834
"""
2935
A pytest fixture to create an empty status file.
3036
31-
:param status_testing_dir: A pytest fixture that defines a path to the the output directory we'll write to
37+
:param status_testing_dir: A pytest fixture that defines a path to the the output
38+
directory we'll write to
39+
:returns: The path to the empty status file
3240
"""
3341
empty_file = Path(f"{status_testing_dir}/empty_status.json")
3442
if not empty_file.exists():
3543
empty_file.touch()
3644

3745
return empty_file
46+
47+
48+
@pytest.fixture(scope="session")
49+
def status_spec_path(status_testing_dir: str) -> str: # pylint: disable=W0621
50+
"""
51+
Copy the test spec to the temp directory and modify the OUTPUT_PATH in the spec
52+
to point to the temp location.
53+
54+
:param status_testing_dir: A pytest fixture that defines a path to the the output
55+
directory we'll write to
56+
:returns: The path to the spec file
57+
"""
58+
test_spec = f"{os.path.dirname(__file__)}/../unit/study/status_test_files/status_test_spec.yaml"
59+
spec_in_temp_dir = f"{status_testing_dir}/status_test_spec.yaml"
60+
shutil.copy(test_spec, spec_in_temp_dir) # copy test spec to temp directory
61+
62+
# Modify the OUTPUT_PATH variable to point to the temp directory
63+
with open(spec_in_temp_dir, "r") as spec_file:
64+
spec_contents = yaml.load(spec_file, yaml.Loader)
65+
spec_contents["env"]["variables"]["OUTPUT_PATH"] = status_testing_dir
66+
with open(spec_in_temp_dir, "w") as spec_file:
67+
yaml.dump(spec_contents, spec_file)
68+
69+
return spec_in_temp_dir
70+
71+
72+
def set_sample_path(output_workspace: str):
73+
"""
74+
A pytest fixture to set the path to the samples file in the test spec.
75+
76+
:param output_workspace: The workspace that we'll pull the spec file to update from
77+
"""
78+
temp_merlin_info_path = f"{output_workspace}/merlin_info"
79+
expanded_spec_path = f"{temp_merlin_info_path}/status_test_spec.expanded.yaml"
80+
81+
# Read in the contents of the expanded spec
82+
with open(expanded_spec_path, "r") as expanded_file:
83+
expanded_contents = yaml.load(expanded_file, yaml.Loader)
84+
85+
# Modify the samples file path
86+
expanded_contents["merlin"]["samples"]["file"] = f"{temp_merlin_info_path}/samples.csv"
87+
88+
# Write the new contents to the expanded spec
89+
with open(expanded_spec_path, "w") as expanded_file:
90+
yaml.dump(expanded_contents, expanded_file)
91+
92+
93+
@pytest.fixture(scope="session")
94+
def status_output_workspace(status_testing_dir: str) -> str: # pylint: disable=W0621
95+
"""
96+
A pytest fixture to copy the test output workspace for status to the temporary
97+
status testing directory.
98+
99+
:param status_testing_dir: A pytest fixture that defines a path to the the output
100+
directory we'll write to
101+
:returns: The path to the output workspace in the temp status testing directory
102+
"""
103+
output_workspace = f"{status_testing_dir}/{status_test_variables.VALID_WORKSPACE}"
104+
shutil.copytree(status_test_variables.VALID_WORKSPACE_PATH, output_workspace) # copy over the files
105+
set_sample_path(output_workspace) # set the path to the samples file in the expanded yaml
106+
return output_workspace
107+
108+
109+
@pytest.fixture(scope="function")
110+
def status_args():
111+
"""
112+
A pytest fixture to set up a namespace with all the arguments necessary for
113+
the Status object.
114+
115+
:returns: The namespace with necessary arguments for the Status object
116+
"""
117+
return Namespace(
118+
subparsers="status",
119+
level="INFO",
120+
detailed=False,
121+
output_path=None,
122+
task_server="celery",
123+
cb_help=False,
124+
dump=None,
125+
no_prompts=True, # We'll set this to True here since it's easier to test this way
126+
)
127+
128+
129+
@pytest.fixture(scope="session")
130+
def status_nested_workspace(status_testing_dir: str) -> str: # pylint: disable=W0621
131+
"""
132+
Create an output workspace that contains another output workspace within one of its
133+
steps. In this case it will copy the status test workspace then within the 'just_samples'
134+
step we'll copy the status test workspace again but with a different name.
135+
136+
:param status_testing_dir: A pytest fixture that defines a path to the the output
137+
directory we'll write to
138+
:returns: The path to the top level workspace
139+
"""
140+
top_level_workspace = f"{status_testing_dir}/status_test_study_nested_20240520-163524"
141+
nested_workspace = f"{top_level_workspace}/just_samples/nested_workspace_20240520-163524"
142+
shutil.copytree(status_test_variables.VALID_WORKSPACE_PATH, top_level_workspace) # copy over the top level workspace
143+
shutil.copytree(status_test_variables.VALID_WORKSPACE_PATH, nested_workspace) # copy over the nested workspace
144+
set_sample_path(top_level_workspace) # set the path to the samples file in the expanded yaml of the top level workspace
145+
set_sample_path(nested_workspace) # set the path to the samples file in the expanded yaml of the nested workspace
146+
return top_level_workspace

0 commit comments

Comments
 (0)