-
Notifications
You must be signed in to change notification settings - Fork 54
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
Benchmark options #938
Merged
Merged
Benchmark options #938
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change passes the full ImageDescriptor to the benchmark tests rather than deconstructing it. This is really a setup so that we can pass more complex options in the future. Signed-off-by: Kern Walster <walster@amazon.com>
turan18
previously approved these changes
Nov 13, 2023
sondavidb
reviewed
Nov 14, 2023
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.
Unrelated, but is there a reason benchmarkTests.go is in camelcase and everything else uses underscores?
All of our existing benchmark images are run with the same set of containerd options which are only configurable at the test level to control things like which snapshotter is used. This is a problem for benchmarking GPU workloads, for example, where we need to pass additional options to mount the GPU in the container which don't apply to all images in the test. Additionally, our benchmarker assumes that the benchmarked images require no configuration, however this can make experimentation hard in cases where a single base-image can be used for multiple use cases depending on environment variables, confiration mounts, etc. This change adds the ability to configure image-specific options when loading benchmarks from json. The options are not required and if not passed, the benchmarker will behave as it did before this change. The set of options available in this change are those that were necessary for benchmarking the LLM workloads that I was trying to test. They are not comprehensive, but can be built upon as use cases arise. Signed-off-by: Kern Walster <walster@amazon.com>
65b5fcb
to
192801f
Compare
austinvazquez
approved these changes
Nov 14, 2023
turan18
approved these changes
Nov 14, 2023
No, I don't think so. Historical accident. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
N/A
Description of changes:
All of our existing benchmark images are run with the same set of containerd
options which are only configurable at the test level to control things
like which snapshotter is used.
This is a problem for benchmarking GPU workloads, for example, where we need to
pass additional options to mount the GPU in the container which don't
apply to all images in the test.
Additionally, our benchmarker assumes that the benchmarked images
require no configuration, however this can make experimentation hard in
cases where a single base-image can be used for multiple use cases
depending on environment variables, confiration mounts, etc.
This change adds the ability to configure image-specific options when
loading benchmarks from json. The options are not required and if not
passed, the benchmarker will behave as it did before this change.
The set of options available in this change are those that were
necessary for benchmarking the LLM workloads that I was trying to test.
They are not comprehensive, but can be built upon as use cases arise.
I kept 2 commits to separate some refactoring that I needed to make these change from the changes themselves. I'm happy to squash or pull out the refactoring into a separate commit if that's preferred.
Testing performed:
~/benchmark.json
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.