-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add flag enforce_max_duration #798
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Kudos, SonarCloud Quality Gate passed! |
Let's test it: @freedomtan @AhmedTElthakeb @mohitmundhragithub |
I still saw invalid color for > 600 seconds. Is this expected? |
No, as per @pgmpablo157321
|
@pgmpablo157321 Can you check if the result is expected:
|
Kudos, SonarCloud Quality Gate passed! |
@pgmpablo157321 is it possible to add something like total running time/duration to the summary generated by the |
Tested on unsupported device, run ends at 10mins but result still rendered as INVALID |
@freedomtan and @anhappdev to contact @pgmpablo157321 by e-mail |
@anhappdev Sorry for the late reply, I don't usually check the issues in this repo
I don't think that result is invalid because of the max duration. It seems it failed because of the early stopping requirements, it may sound related but I don't think they are. Early stopping is a feature that was introduced a while ago, but basically it is a method to check that
Yes, we can report this value in the summary as well. |
looks like not the |
I read the log carefully and did some tests. It turns out the
|
@freedomtan So is this behaviour correct? or what conditions do you expect to pass to have a |
@pgmpablo157321 Personally, I think yes. We'll discuss it to see if the early stopping requirement is what we want. @Mostelk and @mohitmundhragithub: what's your opinion? |
The goal is to test functionality of enforce_max_duration , then let us increase the duration to 45 minutes to satisfy the 64 min queries for early stopping, and see if we get valid result. We can also discuss what is reasonable min queries for early stopping in policy meeting, rather than removing this condition. |
Yes, I tested it before. If 64 queries are allowed by having large max duration, then we'll get VALID result. @pgmpablo157321: could you please merge the branch into the inference repo's main branch @anhappdev please rebase after that. |
@pgmpablo157321: ping |
@freedomtan to send email to check with @pgmpablo157321 |
For @pgmpablo157321's comment to have a mobile specific branch for loadgen:
|
summary(" Min queries satisfied : ", min_queries_met ? "Yes" : settings.enforce_max_duration? "NO" : "Skipped"); This one is problematic too. It says: if (min_queries_met) {
return "Yes";
} else {
if (settings.enforce_max_duration) {
return "NO";
} else {
return "SKIPPED";
}
} which means, So an easy fix is to rename |
@anhappdev I updated With that, we can have what @Mostelk proposed. |
Based on this fix,, we should make |
I assume this logic will not go into the main inference branch and we need to have a separate branch or a patch for this change. |
How about changing it back to |
I don't know. I think it's more a policy decision than a technical issue. |
max_duration: max_duration should allow the task to have 64 queries to get as commented in #701, let's see if we can get |
I tested tflite backend on couple devices. As we discussed in #701 before, 10 mins should be fine.
|
@anhappdev please help to make the color of VALID + early stopped results to be purple as in
|
The logic is updated for |
|
yes, please help fix it. I thought changing the log to warning (instead of error) is enough :-) |
@freedomtan I don't have write access on the inference repo. Please merge this PR |
|
The logic is now:
result_min_duration_met
&&result_min_queries_met
&&early_stopping_met
) => blue textresult_min_duration_met
&&early_stopping_met
) => purple textThe result screen will look like this:
![](https://private-user-images.githubusercontent.com/85728587/308460002-afda3a8a-000c-4ace-a7df-41cc1e715b8c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMzk3MDAsIm5iZiI6MTczOTEzOTQwMCwicGF0aCI6Ii84NTcyODU4Ny8zMDg0NjAwMDItYWZkYTNhOGEtMDAwYy00YWNlLWE3ZGYtNDFjYzFlNzE1YjhjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA5VDIyMTY0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcxODk4YTU1OTM3NGFmMTNlMmYyZmNkMDU2YzE2Yzc1YmJmZjc3MDZhMjg5OGI2OTA3ZTA1OGU5ZTg3ZjY1MzUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.DSz1qZMGnJ9DDgbw4IYQ2DMtoU7hbQbZLCGAqrKfieo)