Skip to content

Commit 31b4e50

Browse files
scheibelppearce8
andauthored
Experiment info fragility (#1247)
* tolerate users providing specs (e.g. with variants) to 'experiment info' * proper distinction between 'bad variant' and 'bad value for valid variant' * internal naming for clarity: we must have an experiment, not an experiment class * try to improve error message * auto style * rm commented out code --------- Co-authored-by: pearce8 <pearce8@llnl.gov>
1 parent 090d124 commit 31b4e50

File tree

2 files changed

+35
-25
lines changed

2 files changed

+35
-25
lines changed

lib/benchpark/cmd/info.py

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import argparse
12
import os
23
import subprocess
34
import textwrap
@@ -138,20 +139,26 @@ def _info_ramble_name(experiment_class):
138139
)
139140
)
140141

141-
experiment_spec = benchpark.spec.ExperimentSpec(args.name)
142+
experiment_spec_str = " ".join(args.name)
143+
experiment_spec = benchpark.spec.ExperimentSpec(experiment_spec_str)
142144
conc = experiment_spec.concretize()
143-
experiment_class = conc.experiment
145+
try:
146+
experiment = conc.experiment
147+
except Exception as e:
148+
msg = (
149+
f"'{experiment_spec_str}' must be a valid experiment spec;"
150+
" some experiments require specifying additional variants"
151+
" (e.g. experiments not inheriting MpiOnlyExperiment must"
152+
" set +rocm or +cuda)."
153+
)
154+
raise ValueError(msg) from e
144155

145156
if args.spack:
146157
subprocess.run(
147158
[
148159
"spack",
149160
"info",
150-
(
151-
experiment_class.spack_name
152-
if experiment_class.spack_name
153-
else experiment_class.name
154-
),
161+
(experiment.spack_name if experiment.spack_name else experiment.name),
155162
]
156163
)
157164
return
@@ -160,20 +167,16 @@ def _info_ramble_name(experiment_class):
160167
[
161168
"ramble",
162169
"info",
163-
(
164-
experiment_class.ramble_name
165-
if experiment_class.ramble_name
166-
else experiment_class.name
167-
),
170+
(experiment.ramble_name if experiment.ramble_name else experiment.name),
168171
]
169172
)
170173
return
171174
else:
172175
actions = {
173-
"maintainers": (info_maintainers, [experiment_class]),
174-
"ramble_name": (_info_ramble_name, [experiment_class]),
175-
"spack_name": (_info_spack_name, [experiment_class]),
176-
"variants": (info_variants, [experiment_class]),
176+
"maintainers": (info_maintainers, [experiment]),
177+
"ramble_name": (_info_ramble_name, [experiment]),
178+
"spack_name": (_info_spack_name, [experiment]),
179+
"variants": (info_variants, [experiment]),
177180
}
178181

179182
# Call functions for enabled options, or all if no flag is set
@@ -218,7 +221,9 @@ def setup_parser(root_parser):
218221
experiment_parser.add_argument(
219222
"--maintainers", action="store_true", help="Maintainers"
220223
)
221-
experiment_parser.add_argument("name", help="Experiment name")
224+
experiment_parser.add_argument(
225+
"name", nargs=argparse.REMAINDER, help="Experiment name"
226+
)
222227

223228

224229
def command(args):

lib/benchpark/spec.py

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -323,14 +323,19 @@ def _concretize(self):
323323
name, values = variants_to_check.pop()
324324
checked.add((name, values))
325325

326-
conditions = [
327-
w
328-
for w, v_by_n in self.object_class.variants.items()
329-
for n, v in v_by_n.items()
330-
if n == name and v.validate_values_bool(values)
331-
]
332-
333-
if not conditions:
326+
conditions = []
327+
possible_variants = set()
328+
for when, v_by_n in self.object_class.variants.items():
329+
possible_variants.update(v_by_n.keys())
330+
for n, v in v_by_n.items():
331+
if n == name:
332+
if not v.validate_values_bool(values):
333+
raise Exception(
334+
f"'{values}' is not valid for variant '{name}' on {self.name}"
335+
)
336+
conditions.append(when)
337+
338+
if name not in possible_variants:
334339
raise Exception(f"{name} is not a valid variant of {self.name}")
335340

336341
# This variant is already valid on self

0 commit comments

Comments
 (0)