-
Notifications
You must be signed in to change notification settings - Fork 42
Enable Initializing Second Experiment From Library #931
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6ae163c
9553029
997dcf5
b8778ec
c9ca1ea
4015da7
1e25f53
2792c85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| # Copyright 2023 Lawrence Livermore National Security, LLC and other | ||
|
Collaborator
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 this module should be called
Collaborator
Author
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. Ok I will rename this to |
||
| # Benchpark Project Developers. See the top-level COPYRIGHT file for details. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
|
|
||
| from benchpark.error import BenchparkError | ||
| from benchpark.directives import variant, requires | ||
| from benchpark.experiment import ExperimentHelper | ||
| from enum import Enum | ||
|
|
||
|
|
||
| class ModelsType(Enum): | ||
| Mpionly = "mpi" | ||
| Openmp = "openmp" | ||
| Cuda = "cuda" | ||
| Rocm = "rocm" | ||
|
|
||
|
|
||
| def Models(*types): | ||
| for ty in types: | ||
| if not isinstance(ty, ModelsType): | ||
| raise ValueError(f"Invalid programming model: {ty}") | ||
|
|
||
| # Normalize once so we can reuse | ||
| _available = tuple(t.value for t in types) | ||
|
|
||
| class BaseModel: | ||
| requires("mpi", when="+mpi") | ||
| requires("rocm", when="+rocm") | ||
| requires("cuda", when="+cuda") | ||
| requires("openmp", when="+openmp") | ||
|
|
||
| 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") | ||
|
Collaborator
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. 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).
Collaborator
Author
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. This is true, regardless of which models are defined, if the experiment inherits from I don't know of a solution to the underlying bug that would enable separate classes. Perhaps something like 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. |
||
|
|
||
| # Class-level list of supported models for any class that includes this mixin | ||
| _available_programming_models = _available | ||
|
|
||
| # Handy instance-level property (works on Experiment instances) | ||
| @property | ||
| def available_programming_models(self): | ||
| # If multiple mixins contribute, merge them from MRO at runtime | ||
| models = set() | ||
| for cls in type(self).mro(): | ||
| models.update(getattr(cls, "_available_programming_models", ())) | ||
| return tuple(sorted(models)) | ||
|
|
||
| # Quick check helper | ||
| @staticmethod | ||
| def supports_model(name: str) -> bool: | ||
| return name in _available | ||
|
|
||
| # Helper class (unchanged except for optional new method) | ||
| class Helper(ExperimentHelper): | ||
| def get_helper_name_prefix(self): | ||
| models = [] | ||
| for s in [ | ||
| ModelsType.Mpionly.value, | ||
| ModelsType.Openmp.value, | ||
| ModelsType.Cuda.value, | ||
| ModelsType.Rocm.value, | ||
| ]: | ||
| if self.spec.satisfies("+" + s): | ||
| models.append(s) | ||
| if len(models) > 0: | ||
| return models | ||
| return "no_model" | ||
|
|
||
| # Optional: expose *available* (not selected) models via helper, too | ||
| def get_available_models(self): | ||
| models = set() | ||
| for cls in type(self).__mro__: | ||
| models.update(getattr(cls, "_available_programming_models", ())) | ||
| return tuple(sorted(models)) | ||
|
|
||
| return type( | ||
| "ProgrammingModel", | ||
| (BaseModel,), | ||
| { | ||
| "Helper": Helper, | ||
| }, | ||
| ) | ||
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # Copyright 2023 Lawrence Livermore National Security, LLC and other | ||
| # Benchpark Project Developers. See the top-level COPYRIGHT file for details. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import pytest | ||
|
|
||
| import benchpark.spec | ||
| from benchpark.error import BenchparkError | ||
|
|
||
|
|
||
| def test_programming_model_checks(): | ||
| # babelstream mpi-only not valid | ||
| with pytest.raises(BenchparkError, match=r"mpi.*are not valid programming models"): | ||
| spec = benchpark.spec.ExperimentSpec("babelstream").concretize() | ||
| experiment = spec.experiment # noqa: F841 | ||
|
|
||
| # stream+openmp not valid | ||
| with pytest.raises(Exception, match="not a valid variant"): | ||
| spec = benchpark.spec.ExperimentSpec( | ||
| "stream+openmp workload=stream" | ||
| ).concretize() | ||
| experiment = spec.experiment | ||
|
|
||
| # Multiple scaling options not valid | ||
| with pytest.raises(BenchparkError, match="cannot specify multiple scaling options"): | ||
| spec = benchpark.spec.ExperimentSpec("kripke+strong+weak").concretize() | ||
| experiment = spec.experiment |
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.
These tests can now be moved to lib/benchpark/test/experiment_errors.py, which shows how much more concise the pytest format is