Enable Initializing Second Experiment From Library #931
Conversation
github-actions
bot
added
feature
| # 1) Non-MPIOnlyExperiment Test | ||
| set +e | ||
| # babelstream is not an MPIOnlyExperiment | ||
| stderr_output="$({ ./bin/benchpark experiment init --dest babelstream --system dane babelstream; } 2>&1)" | ||
| status=$? | ||
| set -e | ||
| [[ $status -ne 0 ]] || fail "Expected non-zero exit status, got $status" | ||
| # Check for the specific BenchparkError message | ||
| expected="cannot run with MPI only" | ||
| grep -Fq "$expected" <<< "$stderr_output" && echo "failed as expected: $expected" || fail "Expected error message not found in stderr. Got: $stderr_output" | ||
| # 2) Multiple scaling options | ||
| set +e | ||
| # Cannot use both +strong and +weak | ||
| stderr_output="$({ ./bin/benchpark experiment init --dest kripke --system dane kripke+strong+weak; } 2>&1)" | ||
| status=$? | ||
| set -e | ||
| [[ $status -ne 0 ]] || fail "Expected non-zero exit status, got $status" | ||
| # Check for the specific BenchparkError message | ||
| expected="spec cannot specify multiple scaling options" | ||
| grep -Fq "$expected" <<<"$stderr_output" && echo "failed as expected: $expected" || fail "Expected error message not found in stderr. Got: $stderr_output" |
There was a problem hiding this comment.
These tests can now be moved to lib/benchpark/test/experiment_errors.py, which shows how much more concise the pytest format is
michaelmckinsey1
changed the title
michaelmckinsey1
added
ready for review
|
Ready for a review, but definitely not merging because only 2 of the experiments are updated for this change |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have a question and a request
| @@ -0,0 +1,85 @@ | |||
| # Copyright 2023 Lawrence Livermore National Security, LLC and other | |||
There was a problem hiding this comment.
I think this module should be called experiment_models, or live in an an experiments subdir
There was a problem hiding this comment.
Ok I will rename this to experiment_models
| variant("mpi", default=True, description="Run with MPI") | ||
| variant("rocm", default=False, description="Build and run with ROCm") | ||
| variant("cuda", default=False, description="Build and run with CUDA") | ||
| variant("openmp", default=False, description="Build and run with OpenMP") |
There was a problem hiding this comment.
It looks like these variants are always defined on every experiment, regardless of whether it inherits the associated model types.
It should be allowed to split these variants into separate modules and imported classes (as they were originally); at the same time, customizing it all from one file is ok too, I wonder if it's possible to address the above concern though (it might not be possible to conditionally define the variants).
There was a problem hiding this comment.
This is true, regardless of which models are defined, if the experiment inherits from Models(...) all programming models will show in the spec +mpi~rocm~cuda~openmp. I would like to note that this is the same behavior for the Scaling, e.g. even if the experiment does not define Throughput scaling it will show ~throughput in the spec. Not that this is correct either, but if we can find a solution for this it should be applied to both the programming models and scaling.
I don't know of a solution to the underlying bug that would enable separate classes. Perhaps something like
# mpi.py
def Mpi():
class BaseMPI:
variant("mpi")
# rocm.py
def Rocm():
class BaseRocm:
variant("rocm")
would provide the correct behavior, but the design seems maybe a bit convoluted.
It seems like the relevant piece for the behavior we desire is making sure the variant is not applied when the class is imported.
michaelmckinsey1
added
changes requested
Description
This can be worked around by running the pytests individually, but this complicates the code coverage.--systemDuring Experiment Init #982files#diff-a3ddd7238fc6e36daeb0ef76e93fe15cc87824bd3299888075210c5ca6a474d3
models.pytoexperiment_models.py