Skip to content
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

Add command: benchpark system init #298

Merged
merged 109 commits into from
Aug 20, 2024
Merged

Add command: benchpark system init #298

merged 109 commits into from
Aug 20, 2024

Conversation

scheibelp
Copy link
Collaborator

@scheibelp scheibelp commented Jun 25, 2024

Closes #110

(note: ~500 of the +/- of this diff is from moving bin/benchpark to lib/main.py - I recommend collapsing these to make the diff more-readable)

Add a new command benchpark system init, that can generate a system config (a collection of files in a directory) that you can then pass to benchpark setup like:

benchpark setup saxpy/openmp <the-directory-created-by-benchpark-system-init> workspace/

Example 1:

./bin/benchpark system init --basedir=test-system-create aws instance_type=c4.xlarge
(old) ./bin/benchpark system create --basedir=test-system-create --type=aws --set instance-type=c4.xlarge

Creates a directory in test-system-create, and writes a variables.yaml file to it:

variables:
  scheduler: "mpi"
  sys_cores_per_node: "4"
  sys_mem_per_node: 7.5
  ...

This doesn't tie in with boto yet, so doesn't provide a means to generate a description for all instance types. IMO that would be better to handle in a separate PR.

Example 2:

./bin/benchpark system init --basedir=test-system-create tioga rocm=551 compiler=cce ~gtl
(old) ./bin/benchpark system create --basedir=test-system-create --type=tioga

will assemble a packages.yaml config such that:

  • there is only one set of ROCm packages (either all 5.4.3, or all 5.5.1, but not a mix of them)
  • the MPI external chosen matches the compiler choice

(Update August 3rd: done) This doesn't yet generate a variables.yaml or a compiler config, so the result is incomplete.

TODOs

  • --basedir argument can generate an entirely new system config directory, but benchpark setup can't actually use it yet
  • This should accommodate the possibility of packages/compilersconfig
    • As of 296e0e1, this provides a means to assemble a set of individual package configs into a single config
  • This has a --from option to allow taking an existing config, but doesn't support it yet. I think it would be useful to combine this option with --set to override settings from that file
    • (update August 6th: this is no longer relevant: you can now create system classes that fully replicate an existing directory in configs/)

@scheibelp scheibelp self-assigned this Jun 25, 2024
Copy link
Collaborator

@becker33 becker33 left a comment

Choose a reason for hiding this comment

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

Most of these are pretty simple/mechanical changes.

The only big picture issue is the benchpark system create name. I think it would be better UX to name this benchpark system init -- in the long run I think users are unlikely to think of the yaml on disk as the "system" that is created, since they'll be interacting with the python interface. So I think it's better to preserve the benchpark system create name to create the python files later (which is also analogous to Spack and Ramble).

We should also eventually provide a mechanism to init the system as part of the setup command, but that's a separate PR.

print("All tags that exist in Benchpark experiments:")
for k, v in benchpark_experiments_tags.items():
print(k)
basedir = pathlib.Path(os.path.abspath(__file__)).parent.parent
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a follow-on PR, we should make this bilingual like we did for the equivalent Spack script

lib/benchpark/error.py Show resolved Hide resolved
lib/benchpark/experiment.py Show resolved Hide resolved
lib/benchpark/experiment.py Outdated Show resolved Hide resolved
lib/benchpark/experiment.py Outdated Show resolved Hide resolved
lib/benchpark/sys_repo/systems/tioga/system.py Outdated Show resolved Hide resolved
lib/benchpark/system.py Outdated Show resolved Hide resolved
lib/benchpark/test_repo/repo.yaml Outdated Show resolved Hide resolved
lib/test/benchpark/directives.py Outdated Show resolved Hide resolved
@pearce8 pearce8 mentioned this pull request Aug 13, 2024
15 tasks
@scheibelp scheibelp changed the title [WIP] Add command: benchpark system create [WIP] Add command: benchpark system init Aug 13, 2024
@scheibelp
Copy link
Collaborator Author

@becker33 I have renamed benchpark system create to benchpark system init and

  • all comments that resolved trivially I took the liberty of collapsing
  • If I responded but made no change (e.g. I liked your suggestion but wanted to punt it) I put "not addressed yet" at the front
  • Other comments that I think I resolved but involved some discretion, I responded to but did not collapse

Also I don't think I mentioned it yet, but the style checker was giving me trouble trying to block this on style issues in the system/experiment repos. I had some trouble getting it to ignore those, so I just fixed those issues for now. IMO we shouldn't apply the same stringency to system/experiment definitions as we do to the core (but if you disagree, then this PR is in the right place).

@alecbcs alecbcs mentioned this pull request Aug 19, 2024
@alecbcs
Copy link
Member

alecbcs commented Aug 19, 2024

@scheibelp can we move lib/benchpark/system_cmd.py -> lib/benchpark/cmd/system.py ?

@scheibelp scheibelp changed the title [WIP] Add command: benchpark system init Add command: benchpark system init Aug 19, 2024
@scheibelp
Copy link
Collaborator Author

@becker33 SpecTemplate is renamed
@alecbcs system_cmd was moved

The only remaining comment is #298 (comment), which I'd like to address in a different PR.

@becker33 becker33 merged commit f645c49 into develop Aug 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Involving Project CI & Unit Tests dependencies Modifications to a Dependency File
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants