Add page for BERT workload#9
Conversation
andreea-alexandru
left a comment
andreea-alexandru
left a comment
There was a problem hiding this comment.
Looks good, needs some small fixes.
| {0,1,2,3} Instance size (0-single/1-small/2-medium/3-large) | ||
|
|
||
| options: | ||
| -h, --help show this help message and exit |
There was a problem hiding this comment.
The arguments should reflect the ones in https://github.com/fhe-benchmarking/BERT/blob/main/harness/utils.py. The README of the BERT repo should also be updated.
There was a problem hiding this comment.
Currently we only have one model and one dataset, thus we (DESILO) and @code-perspective discussed removing the relevant explanation as shown in here.
(Although it was a mistake not to remove that part from utils.py), We already remove the model/dataset arguments in the README. Thus, I would like to hear your thoughts on whether it would be better to add the arguments again, even if only one option is actually supported.
There was a problem hiding this comment.
Thank you for providing the context. It is fine to not have the model and dataset arguments at the moment, as long as the README, description, and actual options are all consistent.
|
|
||
| <p>Submissions are also required to meet the quality bar of <b>correct inference</b> result for single inference and <b>at least 75% accuracy</b> for batch inference. | ||
| </p> | ||
|
|
There was a problem hiding this comment.
Please describe the BERT model a bit more, e.g., BERT-Base 110M. Mention that other models or datasets may be added in the future.
There was a problem hiding this comment.
The BERT description has been updated. And the issue about model/dataset is mentioned as the above comment.
There was a problem hiding this comment.
I would still add a sentence on "Additional models and datasets may be added in the future."
| <th id="col21" class="server-timing-header" title="Total Time">Total</th> | ||
| <th id="col22" class="server-timing-header" title="Dataset generation">DB Gen</th> | ||
| <th id="col23" class="server-timing-header" title="Key Generation">Keygen</th> | ||
| <th id="col24" class="server-timing-header" title="Encrypted model preprocessing">Model Prep</th> |
There was a problem hiding this comment.
The server reported times from the measurements in the repo is I/O, not Model Prep. At the same time, it's worth unhiding the Model Prep column in the harness.
There was a problem hiding this comment.
Compared to the ml-inference case, we changed the table columns as follows:
- added "Timing (server) - Model Prep"
- removed "Timing (harness) - Input Gen"
I think it would be better to explain our intention. First, the reason we added "Model Prep" is that the encoding the model weights into plaintexts (taking about 20 mins) is an important unlike the other cases. And the encoding process must be performed on the server in our logic.
Second, I excluded one we considered less important from our perspective because one is added. Thus, there is no issue with adding "Input Gen" again.
Lastly, I am curious to know exactly which I/O you are referring to, because the I/O is included in every step.
There was a problem hiding this comment.
The harness timing is what the harness is measuring, which might be inflated. But every line in the measurements file under "Timing": should have a corresponding column in the table under Timing (harness). So don't remove Input Gen as it currently appears in the measurements files.
For the server reported time, it makes sense to specify Model Prep as a column, but it has to be part of the server reported timing in the measurements files, otherwise it will not get populated.
About I/O, I was referring that in the current measurements from the BERT repository, this is what is being displayed in the Server reported times:
"Server Reported": {
"Encrypted computation": "347970.3173s",
"I/O": "16796.2555s",
"Total": "364766.573s"
}
2 participants
No description provided.