Feature/experiment scaling #375

rfhaque · 2024-09-25T03:05:21Z

Draft implementation of the scaling functionality for benchmarks. Currently only strong scaling is implemented. Works for amg2023/openmp
bin/benchpark experiment init --dest ./experiments kripke programming_model=openmp scaling=strong strong-scaling-factor=2 strong-scaling-num-exprs=3

and kripke/openmp
bin/benchpark experiment init --dest ./experiments amg2023 programming_model=openmp experiment=strong strong-scaling-factor=2 strong-scaling-num-exprs=4

becker33 · 2024-09-25T20:20:55Z

var/exp_repo/experiments/scaling/experiment.py

+    variant(
+        "strong-scaling-factor",
+        default="2",
+        description="Strong-scaling factor (factor by which to increase resources)",
+    )
+
+    variant(
+        "strong-scaling-num-exprs",
+        default="4",
+        description="Number of strong-scaling experiments",
+    )
+
+    variant(
+        "weak-scaling-factor",
+        default="2",
+        description="Weak-scaling factor (factor by which to increase resources and problem sizes)",
+    )
+
+    variant(
+        "weak-scaling-num-exprs",
+        default="4",
+        description="Number of weak-scaling experiments",
+    )


We need to implement conditional variants so we can make this clearer. This could be

variant("scaling", default="none", values=("none", "strong", "weak"), description="...") variant("scaling-factor", default=2, values=lambda x: x >= 2, when="scaling=weak", description="...") variant("scaling-factor", default=2, values=lambda x: x >= 2, when="scaling=strong", description="...") variant("scaling-iterations", default=4, values=lambda x: x > 1, when="scaling=weak", description="...") variant("scaling-iterations", default=4, values=lambda x: x > 1, when="scaling=strong", description="...")

And with a little additional syntax beyond that we could get rid of the duplicate lines.

becker33 · 2024-09-25T23:06:46Z

var/exp_repo/experiments/scaling/experiment.py

+from benchpark.directives import variant
+
+
+class Scaling(object):


I would call this ScalingExperiment instead of just Scaling, I think it's a bit clearer that way

becker33 · 2024-09-25T23:26:01Z

var/exp_repo/experiments/scaling/experiment.py

+    def generate_strong_scaling_parameters(self, initial_resource_list: list):
+        scaling_factor = int(self.spec.variants["strong-scaling-factor"][0])
+        num_exprs = int(self.spec.variants["strong-scaling-num-exprs"][0]) - 1
+        round_robin_order = self.compute_round_robin_order(initial_resource_list)
+        resource_list = [[x] for x in initial_resource_list]
+
+        while num_exprs > 0:
+            for idx in round_robin_order:
+                for i, r in enumerate(resource_list):
+                    r.append(r[-1]*scaling_factor if i == idx else r[-1])
+                num_exprs=num_exprs-1
+                if not num_exprs:
+                    break
+        return resource_list


I think this method can be simplified slightly

Suggested change

def generate_strong_scaling_parameters(self, initial_resource_list: list):

scaling_factor = int(self.spec.variants["strong-scaling-factor"][0])

num_exprs = int(self.spec.variants["strong-scaling-num-exprs"][0]) - 1

round_robin_order = self.compute_round_robin_order(initial_resource_list)

resource_list = [[x] for x in initial_resource_list]

while num_exprs > 0:

for idx in round_robin_order:

for i, r in enumerate(resource_list):

r.append(r[-1]*scaling_factor if i == idx else r[-1])

num_exprs=num_exprs-1

if not num_exprs:

break

return resource_list

def generate_strong_scaling_parameters(self, initial_resource_list: list):

scaling_factor = int(self.spec.variants["strong-scaling-factor"][0])

num_exprs = int(self.spec.variants["strong-scaling-num-exprs"][0])

round_robin_order = self.compute_round_robin_order(initial_resource_list)

resource_list = [[x] for x in initial_resource_list]

for i in range(num_exprs - 1):

idx = (i + round_robin_order[0]) * len(initial_resource_list)

for r_idx, resource in enumerate(resource_list):

next_value = resource[-1] * scaling_value if r_idx == idx else resource[-1]

resource.append(next_value)

return resource_list

becker33 · 2024-09-25T23:27:54Z

var/exp_repo/experiments/scaling/experiment.py

+                    break
+        return resource_list
+
+    def generate_weak_scaling_parameters(self, initial_resource_list: list, initial_problem_size_list: list):


I don't think it's necessarily safe for us to assume that the resource requirements and the problem size are expressed in such neatly compatible ways. I think we should probably assume these could be different dimensions (e.g. resource list is a scalar, problem size is 3D) and round-robin them each separately.

becker33 · 2024-09-26T21:59:04Z

The variant logic can be simplified once we merge #379

…-scaling

scheibelp

I don't see any major issues, but have a few suggestions

scheibelp · 2024-10-08T00:31:04Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+from benchpark.directives import variant
+
+
+class ScalingExperiment(object):


(no change needed) I originally thought it would be problematic that this defines variant()s but does not inherit Experiment, but I think that as long as any package that inherits this also inherits Experiment, there will be no issue with this.

That being said, IIRC one of your arguments about not inheriting Experiment was a concern with overriding methods in Experiment, which is technically still possible regardless of whether ScalingExperiment inherits Experiment.

scheibelp · 2024-10-08T17:59:01Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+    )
+
+    # input parameters:
+    # 1. input_variables: dict[str, int | tuple(str), list[int]]. Input variables


If this is another way to express:

input_variables key value pairs are either of type str: int or tuple(str): list(int)

I can say this mypy-like syntax of expression is confusing (e.g. if list(int) values always have tuple(str) keys and never str keys, this isn't how it can be expressed in mypy). I recommend replacing what you have on this line with the quoted explanation that you used elsewhere.

If that's not the intended meaning, can you explain how this input differs?

scheibelp · 2024-10-08T18:01:43Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+    # the number of dimensions in an (ascending) round-robin order
+    #
+    # output:
+    # scaling_order: list[int]. list of num_exprs values, one for each dimension,


num_exprs is not an input to this particular function, I think this is a list of indices.

scheibelp · 2024-10-08T18:39:38Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+    # output:
+    # output_variables: dict[str, int | list[int]]. num_exprs values for each
+    # dimension of the input variable scaled by the scaling_factor according to the
+    # scaling policy


I think example inputs and outputs would be useful in the comments/doc for this function, which you could adapt from https://github.com/LLNL/benchpark/blob/develop/experiments/amg2023/openmp/ramble.yaml, for example:

input: [[10], [10], [10]] [[2], [2], [2]] output: nx: ['10', '20', '20'] ny: ['10', '10', '20'] nz: ['10', '10', '10'] px: ['2', '4', '4'] py: ['2', '2', '4'] pz: ['2', '2', '2']

scheibelp · 2024-10-08T18:48:57Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+                            if p_idx
+                            == scaling_order_index[exp_num % len(scaling_order_index)]
+                            else p_val[-1]
+                        )


IMO a comment like:

Take initial parameterized vector for experiment, for each experiment after the first, scale one dimension of that vector by the scaling factor; cycle through the dimensions in round-robin fashion.

would be useful here

scheibelp · 2024-10-08T21:22:30Z

var/exp_repo/experiments/amg2023/experiment.py

+        nx = "nx"
+        ny = "ny"
+        nz = "nz"
+        num_procs = f"{{{px}}} * {{{py}}} * {{{pz}}}"


(minor) I'm assuming you are using px as an abbreviation for "px", and not because it's value might change: in that case"{px} * {py} * {pz}" is simpler

@scheibelp This will be removed once I add variants for all px, py and pz

scheibelp · 2024-10-09T00:28:10Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+        val = input_variables[next(iter(input_variables))]
+        min_dim = val.index(min(val)) if isinstance(val, list) else 0
+
+        return [(min_dim + i) % n_dims for i in range(n_dims)]


(suggestion) Since this doesn't sort (e.g. doesn't make sure that the second-lowest dimension is handled second), IMO this would be simpler if it just returned min_dim.

scheibelp · 2024-10-09T00:57:08Z

var/exp_repo/experiments/scalingexperiment/experiment.py

+        default="4",
+        values=int,
+        description="Number of experiments to be generated",
+    )


#379 was merged, but these are not conditioned on when= (as mentioned in #375 (comment)). I think you can effectively turn scaling off by setting scaling-iterations=1, although it still might be clearer to use when.

rfhaque · 2024-10-11T23:23:04Z

lib/benchpark/experiment.py

+    variant(
+        "scaling-factor",
+        default="2",
+        values=int,
+        description="Factor by which to scale values of problem variables",
+    )
+
+    variant(
+        "scaling-iterations",
+        default="4",
+        values=int,
+        description="Number of experiments to be generated",
+    )
+


@scheibelp I have incorporated your earlier suggestions into the scaling methods. I also moved the entire scaling implementation directly into the base Experiment class. Please review and comment this is a good approach. Especially the fact that the base Experiment class will have two variants scaling-factor and scaling-iterations defined in it

rfhaque · 2024-10-11T23:47:15Z

lib/benchpark/experiment.py

+    # dimension of the input variable scaled by the scaling_factor according to the
+    # scaling policy
+    def scale_experiment_variables(
+        self, input_variables, scaling_factor, num_exprs, scaling_variable=None


Hi @dyokelson, I've added the option to specify a scaling_variable to the experiment (all variables in input_variables will be scaled according to the ordering defined on scaling_variable

rfhaque · 2024-10-11T23:48:47Z

var/exp_repo/experiments/amg2023/experiment.py

+                input_params,
+                int(self.spec.variants["scaling-factor"][0]),
+                int(self.spec.variants["scaling-iterations"][0]),
+                scaling_variable,


@dyokelson Usage of the scaling_variable

rfhaque · 2024-10-12T21:52:20Z

@pearce8 @scheibelp If the changes in the PR are acceptable, we can take this to main. I can make further changes in a separate PR

alecbcs and others added 11 commits August 21, 2024 17:14

Add initial framework for experiment init

f5d42ea

saxpy: fix mismatch between variable name and usage

eaf72d0

cmd/experiment: fix interface to Experiment class

19a9cfd

cmd/experiment: implement benchpark experiment list

0823c12

cmd/experiment: remove vestigial references to 'system'

d80ecef

spec: fix bug with ConcreteSpec.satisfies and ConcreteSpec.intersects

7b9c2e2

spec: remove vestigial 'autospec' decorator

7c6f80c

Merge branch 'develop' of github.com:LLNL/benchpark into develop

2238ac1

Merge branch 'develop' of github.com:LLNL/benchpark into develop

3221740

Merge branch 'develop' of github.com:LLNL/benchpark into develop

5104285

Implementation of strong scaling

95df1f4

rfhaque requested review from scheibelp, becker33 and pearce8 September 25, 2024 03:05

github-actions bot added the experiment New or modified experiment label Sep 25, 2024

Weak scaling implementation

3faccd8

becker33 reviewed Sep 25, 2024

View reviewed changes

pearce8 marked this pull request as draft September 27, 2024 19:47

Merge branch 'develop' into feature/experiment-scaling

cc89e6c

becker33 mentioned this pull request Sep 27, 2024

experiment class for kripke #344

Merged

pearce8 added the WIP A work-in-progress not yet ready to commit label Sep 27, 2024

Riyaz Haque added 8 commits September 28, 2024 15:39

Merge with develop

e9ec6de

Merge remote-tracking branch 'origin/develop' into feature/experiment…

09f4e16

…-scaling

Scaling implementation

315a35d

kripke scaling experiment

b0fca4e

Merge with develop

f429f0b

Fix lint formatting

5702ef1

Fix expr name

0022794

Fix lint formatting

af8ae63

Riyaz Haque added 3 commits October 5, 2024 22:28

Fix variable name

4e71e4b

amg2023 scaling implementation

676b58b

Fix lint formatiing

41b7588

scheibelp reviewed Oct 9, 2024

View reviewed changes

Move scaling to lib/benchpark/experiment.py

49fd63a

rfhaque commented Oct 11, 2024

View reviewed changes

Riyaz Haque added 2 commits October 11, 2024 16:41

Update method documentation

419c7f5

Fix lint formatting

03c3e3d

rfhaque commented Oct 11, 2024

View reviewed changes

Fix lint formatting

cfee34d

pearce8 added 2 commits October 15, 2024 14:49

Separating scaling and programming models in AMG experiment

f14d9a8

formatting

6a4d5fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/experiment scaling #375

Feature/experiment scaling #375

rfhaque commented Sep 25, 2024 •

edited

Loading

becker33 Sep 25, 2024

becker33 Sep 25, 2024

becker33 Sep 25, 2024

becker33 Sep 25, 2024

becker33 commented Sep 26, 2024

scheibelp left a comment

scheibelp Oct 8, 2024

scheibelp Oct 8, 2024

scheibelp Oct 8, 2024

scheibelp Oct 8, 2024

scheibelp Oct 8, 2024

scheibelp Oct 8, 2024

rfhaque Oct 12, 2024

scheibelp Oct 9, 2024

scheibelp Oct 9, 2024

rfhaque Oct 11, 2024

rfhaque Oct 11, 2024

rfhaque Oct 11, 2024

rfhaque commented Oct 12, 2024

		from benchpark.directives import variant


		class Scaling(object):

		from benchpark.directives import variant


		class ScalingExperiment(object):

Feature/experiment scaling #375

Are you sure you want to change the base?

Feature/experiment scaling #375

Conversation

rfhaque commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 commented Sep 26, 2024

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfhaque commented Oct 12, 2024

rfhaque commented Sep 25, 2024 •

edited

Loading