Skip to content

[WIP] Allocation implementation for mpibind modifier #646

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

Draft
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

august-knox
Copy link
Collaborator

@august-knox august-knox commented Feb 27, 2025

Description

Alternative implementation for mpibind(#608), moving all mpi-command editing to the allocation modifier, where this already takes place by default. Functionality remains the same.
Example command:
benchpark experiment init --dest=saxpy saxpy mpibind=v +openmp

So far, modes include: off, on, v, vv, greedy:0

Adding/modifying a benchmark (docs: Adding a Benchmark)

  • Add/modify an experiments/benchmark_name/experiment.py to define a single node and multi-node experiments
  • Add/modify a dry run unit test in .github/workflows/run.yml
  • Remove mention of mpibind modifier since it is part of allocation modifier in this implementation
  • Test
  • Add documentation: how to test
  • @michaelmckinsey1 how to propagate info into Caliper files when Caliper modifier is on

@august-knox august-knox requested a review from rfhaque February 27, 2025 17:05
@github-actions github-actions bot added the experiment New or modified experiment label Feb 27, 2025
@august-knox august-knox mentioned this pull request Feb 27, 2025
9 tasks
Copy link
Collaborator

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few suggestions and a question (let me know if they are unclear or you think another approach is better).

@@ -24,6 +25,7 @@ class Amg2023(
WeakScaling,
ThroughputScaling,
Caliper,
Mpibind,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this class exist?

name="greedy:0",
description="Run mpibind in very greedy mode",
)
depends_on = ["mpibind"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these should be passed as alternate modes for the allocation modifier: I'd prefer this was passed as something like an mpibind variable (with all these possible values)

return f"{base_string} {mpi_string}{mpi_end}"
else:
mpi_end = self._usage_mode
return f"{base_string} {mpi_string}{mpi_end}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just adding an extra option onto the mpi command invocation.

I'd recommend modifying _init_batch_and_cmd_opts to include this extra option, vs doing string manipulation on the command.

def mpibind(self, executable_name, executable, app_inst=None):
pre_exec = []
post_exec = []
output_file = "{experiment_run_dir}/{experiment_name}.out"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(question) is .out the output captured by Ramble from running the experiment? And then parse_mpibind_output.py is searching for output from mpibind that is generated as part of running that experiment? I don't know how mpibind works: are these values determined by mpibind itself (i.e. not known before running the experiment)?

}

output_file = "mpibind_log_file.json"
with open(output_file, "w") as json_file:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks relative: are you sure it runs with the basedir you want? You might want to specify an abspath (e.g. as another argument to parse_mpibind_output.

@pearce8 pearce8 requested a review from nhanford April 7, 2025 19:00
@pearce8 pearce8 added the changes requested Changes requested label May 26, 2025
Copy link
Collaborator

@pearce8 pearce8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfhaque Are you still wanting to merge this? If yes, please update and mark ready for review. If not, please close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes requested experiment New or modified experiment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants