-
Notifications
You must be signed in to change notification settings - Fork 791
[UR][Benchmarks] GROMACS/Grappa benchmarks added to the suite #17934
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
Changes from 2 commits
d7490ca
d740745
acd567e
ac24577
1f4a931
0e95395
67eb2a4
3d93f9e
de067ca
ce86c6b
f530b0f
35c4630
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,324 @@ | ||||||
# Copyright (C) 2025 Intel Corporation | ||||||
# Part of the Unified-Runtime Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||
# See LICENSE.TXT | ||||||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||
|
||||||
import os | ||||||
import subprocess | ||||||
import tarfile | ||||||
import urllib.request | ||||||
from pathlib import Path | ||||||
from .base import Suite, Benchmark | ||||||
from options import options | ||||||
from utils.utils import git_clone, download, run, create_build_path | ||||||
from utils.result import Result | ||||||
|
||||||
|
||||||
class GromacsBench(Suite): | ||||||
|
||||||
def git_url(self): | ||||||
return "https://gitlab.com/gromacs/gromacs.git" | ||||||
|
||||||
def git_tag(self): | ||||||
return "v2025.1" | ||||||
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. Release today :)
Suggested change
Should not matter much, though. |
||||||
|
||||||
def grappa_url(self): | ||||||
return "https://zenodo.org/record/11234002/files/grappa-1.5k-6.1M_rc0.9.tar.gz" | ||||||
|
||||||
def grappa_file(self): | ||||||
return Path(os.path.basename(self.grappa_url())) | ||||||
|
||||||
def __init__(self, directory): | ||||||
self.directory = Path(directory).resolve() | ||||||
model_path = str(self.directory / self.grappa_file()).replace(".tar.gz", "") | ||||||
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
self.grappa_dir = Path(model_path) | ||||||
build_path = create_build_path(self.directory, "gromacs-build") | ||||||
self.gromacs_build_path = Path(build_path) | ||||||
self.gromacs_src = self.directory / "gromacs-repo" | ||||||
|
||||||
def name(self): | ||||||
return "Gromacs Bench" | ||||||
|
||||||
def benchmarks(self) -> list[Benchmark]: | ||||||
models = [ | ||||||
"0001.5", | ||||||
"0003", | ||||||
"0006", | ||||||
"0012", | ||||||
"0024", | ||||||
"0048", | ||||||
"0096", | ||||||
"0192", | ||||||
"0384", | ||||||
] | ||||||
return [GromacsSystemBenchmark(self, model) for model in models] | ||||||
|
||||||
def setup(self): | ||||||
if not (self.gromacs_src).exists(): | ||||||
|
||||||
self.gromacs_src = git_clone( | ||||||
self.directory, | ||||||
"gromacs-repo", | ||||||
self.git_url(), | ||||||
self.git_tag(), | ||||||
) | ||||||
else: | ||||||
if options.verbose: | ||||||
print(f"GROMACS repository already exists at {self.gromacs_src}") | ||||||
|
||||||
# Build GROMACS | ||||||
run( | ||||||
[ | ||||||
"cmake", | ||||||
f"-S {str(self.directory)}/gromacs-repo", | ||||||
f"-B {self.gromacs_build_path}", | ||||||
f"-DCMAKE_BUILD_TYPE=Release", | ||||||
f"-DCMAKE_CXX_COMPILER=clang++", | ||||||
f"-DCMAKE_C_COMPILER=clang", | ||||||
f"-DGMX_GPU=SYCL", | ||||||
f"-DGMX_SYCL_ENABLE_GRAPHS=ON", | ||||||
This comment was marked as resolved.
Sorry, something went wrong. 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. Done |
||||||
f"-DGMX_FFT_LIBRARY=MKL", | ||||||
f"-DGMX_BUILD_OWN_FFTW=ON", | ||||||
|
f"-DGMX_BUILD_OWN_FFTW=ON", |
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.
For PVC we also should set DGMX_GPU_NB_NUM_CLUSTER_PER_CELL_X=1
, see https://manual.gromacs.org/2025.1/install-guide/index.html#sycl-gpu-acceleration-for-intel-gpus.
Was that intentionally omitted because you want to reuse the same build across different Intel GPUS?
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.
Hmm... We don't have a way of specifying gpu in the benchmarks right now. Before it was always PVC, but now we are also testing with BMG.
We probably need a method in utils or somewhere that will autodetect gpu type. Something like:
enum GpuTypes {
INTEL_PVC,
INTEL_BMG,
OTHER // we can extend this with nvidia/amd gpus etc
...
}
options {
gpu = OTHER
}
detect_gpu() {
if options.sycl is None:
return OTHER
output = run(`sycl-ls --verbose`)
default_gpu = re.search(output, ...);
if default_gpu.contains(Data Center GPU Max):
return PVC
if default_gpu.contains(BMG?)
return BMG
return OTHER
}
somewhere in main:
options.gpu = detect_gpu()
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.
Sounds like useful functionality to add, but probably scope creep for this PR. Could have a TODO comment about adding this in once the script can detect the device being targeted.
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.
Sounds good. For now I suggest we add -DGMX_GPU_NB_NUM_CLUSTER_PER_CELL_X=1
and leave a TODO to make it conditional later.
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.
Sounds good. For now I suggest we add -DGMX_GPU_NB_NUM_CLUSTER_PER_CELL_X=1 and leave a TODO to make it conditional later.
FYI, this flag is not critical either way. It improves the performance slightly on PVC, and is at least compatible with BMG (whether it improves the performance or not is an open question).
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.
Another minor suggestion is to add -DGMX_CYCLE_SUBCOUNTERS=ON
for more detailed time breakdown in the log files; won't have much direct effect, but if there are any performance anomalies, the md.log
files might become slightly more useful (if they are preserved).
Outdated
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.
use the existing utils.download
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.
Done
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.
Remove download_and_extract_grappa method and use utils.download instead for simplicity. If you find adding a verbose message is useful, please add it to utils.download
Outdated
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.
this isn't necessary, download already checks it.
PatKamin marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Add "benchmark_type". Name needs to be unique.
Outdated
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.
Move this check to setup() as you try running this binary there first without a check.
Outdated
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.
run takes an array of ld_library=[] that is then passed to one LD_LIBRARY_PATH.
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.
OK
Outdated
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.
I'm guessing this controls whether we use eager vs graph?
Can you make this configurable and add "eager"/"graph" variants of the benchmarks?
Outdated
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.
if verbose ?
benchmarks should not print anything.
Outdated
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.
this comment is no longer correct
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
I wouldn't bother passing the benchmark_type just so that you can print error message differently. The framework already collects errors into a map indexed by the benchmark name.
Outdated
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.
remove old code.
pbalcer marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
pbalcer marked this conversation as resolved.
Show resolved
Hide resolved
EwanC marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
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.
remove the imports you don't use.
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.
Sure