Skip to content

[CI] Add HeCBench to CI - #280

Open
maarquitos14 wants to merge 20 commits into
ROCm:amd-stagingfrom
maarquitos14:maronas/hecbench-ci
Open

maarquitos14 wants to merge 20 commits into
ROCm:amd-stagingfrom
maarquitos14:maronas/hecbench-ci

Conversation

@maarquitos14

Copy link
Copy Markdown
Contributor

Add a curated subset of HeCBench benchmarks to precommit CI. The subset was chosen following the criteria listed below:

  1. Avoid benchmarks requiring datasets requiring downloads. We want to maximize the time we have to build+run benchmarks: time spent downloading is time not spent building+running benchmarks.
  2. Build+run benchmarks that have shown failures in the past. We want to make sure we do not re-introduce regressions that were fixed in the past.
  3. Least time consuming, so we can run as many as possible in the given time.
  4. This one might be lifted in the future, but for now: do not run benchmarks requiring MPI. The reason is to avoid adding an extra installation in the container.

@idubinov idubinov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the implementation, was positively surprised, you did a good work, thanks.
I don't like the idea of this bunch of shell scripts that does unstable stuff. I understand that behind each script stay a lot of debugs and each row fixes some problems I cannot see due to lack of the experience with HeCBench. That's why I have nothing against this PR.

Comment thread .github/workflows/spirv-ci-linux.yml
Comment thread .github/workflows/spirv-ci-linux.yml Outdated
@@ -0,0 +1,438 @@
#!/usr/bin/env bash

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whole file looks like unstable duct tape solution. HeCBench readme makes feel that it should work with HIP out-of-box without changes

I see that CMake version is under development, but have you tried it?

HeC bench is actively developed. If it requires CMake/make files update for configuration generalization - isn't it better to update HeC? I understand that it may took some time, so if this file will be merged - create an issue please for updating HeC Bench

Correct me if i missed something

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whole file looks like unstable duct tape solution. HeCBench readme makes feel that it should work with HIP out-of-box without changes

The file was reduced significantly, and now basically simply injects --offload-arch=$(HIP_ARCH) after hipcc. This is needed to later pass HIP_ARCH=amdgcnspirv to target SPIRV, otherwise we cannot specify the target. I'm open to other suggestions if there is a better way to do this.

I see that CMake version is under development, but have you tried it?

I did, the build works fine with CMake for the subset we're using here, but the run doesn't. The corresponding run for the CMake side of things is through CTest, and it requires some setup (namely benchmark args and validation regex) that the repo still doesn't have. Building through CMake and then running through Makefile triggers a rebuild through the Makefile, so using CMake is not helpful at the moment.

HeC bench is actively developed. If it requires CMake/make files update for configuration generalization - isn't it better to update HeC? I understand that it may took some time, so if this file will be merged - create an issue please for updating HeC Bench

Yes, it is in active development. It does require a lot of update, from my POV. We can work on enabling that as a follow up, but since I started working on HeCBench I've seen that it is a set of benchmarks rather than a benchmark suite, because there is no consistency in build system and run driver. Each benchmark has its own independent way to build and run. There seems to be some progress in the CMake build system, but the run part is mostly a stub IMO. I don't think the maintainers are working on this, so this would be on us to do.

@@ -0,0 +1,100 @@
#!/usr/bin/env bash

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

where and when this file is used in ci?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At the moment, it is not used in the CI. Here's how it works at the moment: ci_benchmarks.txt contains the subset of benchmarks to run. Many of those benchmarks reference sibling directories (e.g. benchmark1-hip references benchmark1-cuda), so update_deps.sh takes ci_benchmarks.txt and outputs all the referenced directories that need to be cloned too in ci_benchmarks.deps.txt. We do this because we only check out the minimal set of directories needed to run the subset rather than the whole repo. At the moment, this step is done manually.

In a follow up PR, I plan to add a daily auto-updater mirroring the one you made for ROCM examples that will run update_deps.sh to refresh ci_benchmarks.deps.txt.

Comment thread .github/workflows/spirv-ci-linux.yml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants