-
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 7 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 @@ | ||
/html/data.js |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,223 @@ | ||||
# 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 | ||||
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" | ||||
|
||||
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]: | ||||
return [ | ||||
GromacsBenchmark(self, "0006", "pme", "graphs"), | ||||
GromacsBenchmark(self, "0006", "pme", "eager"), | ||||
GromacsBenchmark(self, "0192", "rf", "graphs"), | ||||
GromacsBenchmark(self, "0192", "rf", "eager"), | ||||
|
||||
] | ||||
|
||||
def setup(self): | ||||
self.gromacs_src = git_clone( | ||||
self.directory, | ||||
"gromacs-repo", | ||||
self.git_url(), | ||||
self.git_tag(), | ||||
) | ||||
|
||||
# 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).
PatKamin marked this conversation as resolved.
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Do we want to also have some correctness checks?
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.
In case of non-zero exit code, CalledProcessError will be raised and time extraction will not happen. Do you think of some extra means here?
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 was thinking about additional correctness validation (e.g., I had issues with early versions of graphs where some operations were not captured: the simulation won't necessarily crash, but the results would be off). Running the full test suite would be an overkill, but, e.g., an easy test here could be to grep the md.log for Conserved energy drift
value. If abs(drift) < 1e-3, then things are roughtly okay (the threshold value is not universal, but that's what in my experience is ok for the Grappa set).
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. The value 1e-3 does not work for eager rf, so the verification is added for pme only.
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.
The value 1e-3 does not work for eager rf, so the verification is added for pme only.
That's a bit suspicious. Do you have at hand is the drift value for RF? A small system + a short run, it could be more variable than usual; but if we're talking about ~1e0 and up, that's way broken.
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.
If data.js
is up-to-date, this leads to
{
"results": [
{
"label": "gromacs-0006-pme",
"value": 2.026,
"command": [
"/",
"h",
"o",
"m",
"e",
"/",
"m",
"a",
"t",
"e",
"u",
"s",
....
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.
yeah, this should just be command=command
.
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.
Fixed
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.
Release today :)
Should not matter much, though.