-
Notifications
You must be signed in to change notification settings - Fork 52
Standalone score compute #433
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
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
f5cffb2 to
b917d71
Compare
|
I tested it locally on a few results With scaling.json - Without After manually deleting scaling.json - But if I don't manually delete scaling.json and run it without the |
|
Testing power scores - With Without After deleting |
|
Few more issues that need to be dealt with - Trying to compute scores or just 1 or 2 files returns None although it would help to just print out the individual scores of each of the files in the folder - Changing file names to be anything other than result_*.txt does not compute scores although this is expected. |
|
@pgmpablo157321 TODO items as discussed in the training WG
Additional piece for (2) would be to also print the samples to converge along with the scores for each log file so submitters get a sense of their convergence as well. Is that also something we can add? |
| "--has_power", action="store_true", help="Compute power score as well" | ||
| ) | ||
| parser.add_argument( | ||
| "--benchmark_folder", |
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'd recommend taking a list of files rather than a folder name. then the user could specify the list of files as folder/result*.txt to get all the result.txt files in a folder, but could also specify a single file, and could specify log files and directories that are named differently than result*.txt, like foo/bar/baz/*.log
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.
Unfortunately, this requires significant changes in the RCP checker, particularly changing the check_directory function and it's interactions:
| def check_directory(dir, usage, version, verbose, bert_train_samples, rcp_file=None, rcp_pass='full_rcp', rcp_bypass=False, set_scaling=False): |
Given the time to the next submission, I recommend that we postpone this change
|
Following changes were added:
|
|
Sample run 1: Output: Sample run 2 (manually deleting result_3.txt): Output: |
a47029e to
32e2eec
Compare
32e2eec to
105f189
Compare
ShriyaRishab
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.
Looks great, thanks!
|
Also added logging the sample count: |
05e7fae to
b0b2fe3
Compare
Fix #419