-
Notifications
You must be signed in to change notification settings - Fork 4
Add functions to get gpu/cpu/version info, add to benchmark output #76
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
base: main
Are you sure you want to change the base?
Conversation
2668ecd to
a325bee
Compare
simoneves
left a comment
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.
Implementation looks fine. Unclear on context.
| benchmark_output | ||
|
|
||
| # Generated Config | ||
| presto/docker/config/generated |
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.
Good idea, but drop this and I'll add it to my HERC-82 PR
| """Get total system memory in GB.""" | ||
| try: | ||
| # Read from /proc/meminfo (Linux) | ||
| with open("/proc/meminfo", "r") as f: |
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.
There's an ongoing discussion as to the best way to get this value that works reliably on all machines. Take a look at PR #48 for more notes.
| check=True | ||
| ) | ||
| # Parse CUDA version from output (typically in header) | ||
| # Example: "CUDA Version: 12.2" |
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 guess this is useful info, but it's also implicit from the driver version.
|
@paul-aiyedun @simoneves I forgot about this PR. Do you think it's worth getting something like this in, understanding that the assumption is you're running the script on the same machine you're running benchmarks on? Or is this metadata worthless unless we update the prestodb API to include it and get it from there? |
|
I am still unclear on the context of this, and indeed the process you intended it to be inserted into may have changed. @paul-aiyedun and @misiugodfrey will know more. I would re-base it or merge-update it to current |
|
@simoneves High level context is it's going to be necessary (or at least very beneficial) to capture metadata about the benchmark run if we intend to do any visualization or data analysis with the results themselves afterwards. The alternative seems like a manually collated spreadsheet like has been done up until this point. This branch is old and will need rebasing, but just checking to make sure i'm not barking up the wrong tree here before proceeding. Updating the presto API seems like a major project that makes big changes to Presto's API as far as I can tell, so maybe this would still be useful in the meantime. |
|
I understand the requirement, but I do not feel qualified to review the change. This is all @paul-aiyedun's code and I am not familiar with it at all. |
Per discussion here:
https://docs.google.com/document/d/1wD9YDDze5vr3K6kpU4eKwlk55GwvFx9vVTTj_X8nj-w/edit?tab=t.0
Adding properties to
behcnmark_result.jsonto contain: