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

Allow benchmarks config as json #908

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Conversation

Kern--
Copy link
Contributor

@Kern-- Kern-- commented Nov 2, 2023

Issue #, if available:
N/A

Description of changes:
Benchmarks can currently be passed as a csv file. This additionally adds json as an option to make it not order dependent and to allow us to add more complex options in the future (e.g. mounts).

Testing performed:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sondavidb
sondavidb previously approved these changes Nov 2, 2023
Copy link
Contributor

@sondavidb sondavidb left a comment

Choose a reason for hiding this comment

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

Just food for thought — should we have a function explicitly to detect a file type before sending them to their respective functions? The logical flow here makes sense but I wonder if it would be a bit clearer to separate out the two functions. But maybe not, since as far as I'm aware, the only way to detect if a file is a json is unmarshaling it, which if you do it just to check, you would have to do it again to store the data anyway. But just something to consider.

benchmark/utils.go Show resolved Hide resolved
austinvazquez
austinvazquez previously approved these changes Nov 3, 2023
turan18
turan18 previously approved these changes Nov 3, 2023
Benchmarks can currently be passed as a csv file. This additionally adds
json as an option to make it not order dependent and to allow us to add
more complex options in the future (e.g. mounts).

Signed-off-by: Kern Walster <walster@amazon.com>
@Kern--
Copy link
Contributor Author

Kern-- commented Nov 6, 2023

I don't think it makes sense to try to determine the file path without parsing the file. We could alternatively add a new flag for json files (although -f for csv makes just about anything for json feel like a second-class option), or my preference would be to eventually deprecate csv parsing for being too error prone.

@Kern-- Kern-- merged commit f25e459 into awslabs:main Nov 8, 2023
4 checks passed
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.

None yet

4 participants