Repository navigation
Conversation
…arks BenchmarkStore restored the saved option state *after* each BenchmarkSet constructor had already derived isActive from the pbtxt defaults, and never recomputed it. The options themselves were restored, so the UI looked right, but activeBenchmarks still reflected the defaults. Picking "LLM only" and restarting therefore ran the two Image Classification v2 benchmarks: image_classification ships offline+online enabled, while every llm parameter option ships disabled. The state is now handed to the BenchmarkSet constructor so it is in place before the first applyOptions(), and applyOptionStateMap() always ends by recomputing isActive, so the two can no longer drift apart. Three related defects in the same path, each covered by a test: - BenchmarkOptionSet.selected was fixed at construction, so min_selected / max_selected were enforced against a stale count. With max_selected: 1 that made it impossible to swap the chosen option. No shipped config sets either bound today, so this was latent. - An option id in the stored state that no longer exists in the config threw on a null unwrap, which would break config loading after an upgrade that renames or drops an option. It is now logged and skipped. - Applying a stored state entry by entry made the outcome depend on map order under a min/max constraint. Each option set is now applied atomically and rejected as a whole if it would violate its bounds.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
This was referenced Sep 22, 2026
Contributor
|
Let's wait for @farook-edev's one. |
Contributor
|
let's rebase this after the #1185 merged. |
anhappdev
pushed a commit
that referenced
this pull request
Oct 7, 2026
This PR adds missing line to properly apply benchmark set options after loading them from preferences (on app reopening) it supersedes #1180 it includes FIXME notes for a couple other bugs that aren't affecting the app currently, but will if we use min/max or incorrect option ID logic.
anhappdev
changed the title
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1185, which fixed "LLM only → restart → Image Classification v2 runs" by recomputing
isActiveafter the stored options are restored. This PR fixes the three defects #1185 left as FIXMEs, each with a test, and adds regression tests for the restart bug itself.optionSets[optionMap[entry.key]!]throws, which happens whenever atasks.pbtxtupdate renames or drops a visible option. Startup catches the error but never clears the stored state, so the app shows the resource-error screen on every launch until app data is wiped. Unknown ids are now logged and skipped.BenchmarkOptionSet.selectedwas frozen at construction, somin_selected/max_selectedchecked a stale count: withmax_selected: 1you could never swap the chosen option. The count now tracks set and unset.The last two are latent: no shipped config sets either bound.
The stored state also now goes to the
BenchmarkSetconstructor, so it is in place before the firstapplyOptions(), and the UI toggle goes through the sameapplyOptionStateMap()path.80 unit tests pass (10 new), no new
flutter analyzeissues. The benchmark-set UI/UX rework follows in its own PR.