Don't check min_query_count with equal issue #1810
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
pgmpablo157321
force-pushed
the
min_query_count_check
branch
from
bf04e7d
to
5c5af51
Compare
|
@nvzhihanj @nvyihengz Please review and give your feedback. |
|
LGTM, thanks Pablo |
| @@ -1226,7 +1226,7 @@ def check_performance_dir( | |||
| # Check if this run uses early stopping. If it does, get the | |||
| # min_queries from the detail log, otherwise get this value | |||
| # from the config | |||
| if not uses_early_stopping: | |||
| if not (uses_early_stopping or (equal_issue_used_check and config.requires_equal_issue(model, division))): | |||
| required_min_query_count = config.get_min_query_count(model, scenario) | |||
There was a problem hiding this comment.
The problem can be solved if we read the min_query_count from the loadgen log and not the config right?
There was a problem hiding this comment.
Mmmm I don't think so. We are currently comparing the min_query_count from the log and with a fixed value in the config
There was a problem hiding this comment.
The comparison should be between the value from the log and the value stored in submission checker right?
There was a problem hiding this comment.
@pgmpablo157321 min_query_count is coming from the loadgen log. So, if equal issue mode is enabled, this key value should be automatically updated by the loadgen. Otherwise it is a bug to be fixed in the loadgen.
There was a problem hiding this comment.
The comparison should be between the value from the log and the value stored in submission checker right?
Yes, we are currently doing that. But min_query_count is a parameter from the mlperf.conf, (a lower limit for the number of queries), not the actual number of queries. The min duration also determines the number of queries.
@pgmpablo157321 min_query_count is coming from the loadgen log. So, if equal issue mode is enabled, this key value should be automatically updated by the loadgen. Otherwise it is a bug to be fixed in the loadgen.
This is unfortunately also true for the number of queries, we discuss that in the LLM chat and it's a bug that needs to be fixed for v5.0
There was a problem hiding this comment.
@pgmpablo157321 The check for the equal-issue mode misses commas between benchmark names: L810-L811 and L812-L813
pgmpablo157321
force-pushed
the
min_query_count_check
branch
3 times, most recently
from
203ea62
to
7669e44
Compare
pgmpablo157321
force-pushed
the
min_query_count_check
branch
from
7669e44
to
73a962a
Compare
Fix #1809
@attafosu