[experimental] Adding a CudaExperiment, and variables to Base and CudaExperiment #400
Conversation
| 2. **Experiment instances**. Once instantiated, an experiment is | ||
| essentially a collection of files defining an experiment in a | ||
| Ramble workspace. | ||
| """ |
There was a problem hiding this comment.
This docstring is copied from Experiment and doesn't explain anything new or specific to this class.
| variants: Dict[ | ||
| "benchpark.spec.Spec", | ||
| Dict[str, benchpark.variant.Variant], | ||
| ] |
There was a problem hiding this comment.
You should not have to instantiate this here. Why does this inherit from ExperimentSystemBase vs. Experiment? If this is related to MRO concerns, see my later comment about using composition.
lib/benchpark/cuda.py
Outdated
| system_specs = {} | ||
| system_specs["cuda_version"] = "{default_cuda_version}" | ||
| system_specs["cuda_arch"] = "{cuda_arch}" | ||
| system_specs["blas"] = "cublas-cuda" #TODO: can we define this in Lapack/Blas CudaExperiment? |
There was a problem hiding this comment.
In my experience this differs from system to system, and it's best handled by saying here just that you need blas; the system can then register whatever blas external is appropriate (see for example in Sierra definition: https://github.com/LLNL/benchpark/blob/develop/var/sys_repo/systems/sierra/externals/cuda/00-version-10-1-243-packages.yaml).
lib/benchpark/cuda.py
Outdated
|
|
||
| raise NotImplementedError( | ||
| "Each experiment must implement compute_spack_section" | ||
| ) |
There was a problem hiding this comment.
With an inheritance scheme, this would have to return something, e.g.
return {
"packages": {k: v for k, v in package_specs.items() if v},
"environments": {app_name: {"packages": list(package_specs.keys())}},
}
(copied from Amg2023) at which point, a subclass would want to do something like:
class FooExperiment(CudaExperiment):
def compute_spack_section(self):
config = super().compute_spack_section()
# now add all the things specific to Foo
Using inheritance is somewhat inflexible, because everything you want to mix together would need to be part of a single inheritance chain (which is why I suggested composition in #356 (comment), for example).
lib/benchpark/experiment.py
Outdated
| @@ -70,12 +70,43 @@ def compute_modifiers_section(self): | |||
|
|
|||
| def compute_applications_section(self): | |||
| # TODO: is there some reasonable default? | |||
| variables = {} | |||
| variables["n_ranks"] = num_procs # TODO: num_procs will be defined in the child... | |||
There was a problem hiding this comment.
If that is so, this will need to be self.num_procs, and that will be a new required method for all subclasses (which can be checked by benchpark audit, FWIW).
| "pkg_spec": f"{app_name}@{app_version} +mpi", | ||
| "compiler": system_specs["compiler"], | ||
| } | ||
|
|
||
| raise NotImplementedError( | ||
| "Each experiment must implement compute_spack_section" | ||
| ) |
There was a problem hiding this comment.
If this does anything, it can't raise NotImplementedError. FWIW benchpark audit currently makes sure that the lowest-level definition (e.g. what lives in exp_repo) defines this method.
| package_specs = {} | ||
| package_specs[system_specs["mpi"]] = ( | ||
| {} | ||
| ) # empty package_specs value implies external package |
There was a problem hiding this comment.
This seems like a good candidate for a method in Experiment like
def needs_external(pkgs_dict, system_specs, pkg_name):
pkgs_dict[system_specs[pkg_name]] = {}
(mainly so this comment doesn't have to be repeated).
class Amg2023(Experiment,CudaExperiment):