Resolving objectives issue introduced with introduction of pass search #1585
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
shaahji
force-pushed
the
shaahji/objectives
branch
from
9538e76 to
7fe2dd5
Compare
| } | ||
| return all((multipliers[obj] * result[obj].value) >= (multipliers[obj] * goal) for obj, goal in goals.items()) | ||
| result, _ = self._results[search_point_index] | ||
| return all( |
There was a problem hiding this comment.
what happens if the results is missing the value for a metric that has a goal coming from another pass' evaluator?
There was a problem hiding this comment.
might need an if condition in the loop here?
There was a problem hiding this comment.
The signal doesn't include any identification for which pass generated it so yes, that's still a problem.
Honestly, the whole concept of objectives, goals, and metrics, need a revisit with emphasis on search.
There was a problem hiding this comment.
what if you add if name in results check so that it doesn't hit an index error atleast. if the passes used the same metric but different objectives, then the last one taking priority already enforces that one metric name can only have one objective.
There was a problem hiding this comment.
At the SearchStrategy level, I can see a reason i.e. to conditionally discard search points that don't meet a goal.
Within the context of SearchResult, how does goals matter? As long as the results are sorted based on priority and the signal values, why does goals matter? SearchResult shoudn't even be accounting for any goals at all.
There was a problem hiding this comment.
since this method is used for early stopping here
Olive/olive/search/search_strategy.py
Line 242 in c22f68b
There was a problem hiding this comment.
At the
SearchStrategylevel, I can see a reason i.e. to conditionally discard search points that don't meet a goal.Within the context of
SearchResult, how does goals matter? As long as the results are sorted based on priority and the signal values, why does goals matter?SearchResultshoudn't even be accounting for any goals at all.
My comment was in context to apply_goals argument in SearchResult._get_results_list.
I added the condition in prevent indexing error in the function.
There was a problem hiding this comment.
I see. The apply goals option was added so that from all of the candidate output models, the user can filter out the ones that didn't meet their goal. otherwise, the ranked models can have models that do really well on a high priority metric but don't meet the minimum requirements for another metric.
Olive allows multiple passes under the "passes" key in config where each entry can have dictate its own evaluator config i.e. the evaluation config to use for that specific pass. However, with pass search, this becomes an issue because each pass within the group can dictate conflicting objectives and goals. Circumventing the issue by collecting all the objectives across the group with the last one in the list winning if named the same. Also, handle the case where not all objectives are generated as part of the post evaluation signal.
shaahji
force-pushed
the
shaahji/objectives
branch
from
7fe2dd5 to
e641d84
Compare
Resolving objectives issue introduced with introduction of pass search
Olive allows multiple passes under the "passes" key in config where each entry can have dictate its own evaluator config i.e. the evaluation config to use for that specific pass. However, with pass search, this becomes an issue because each pass within the group can dictate conflicting objectives and goals. Circumventing the issue by collecting all the objectives across the group with the last one in the list winning if named the same. Also, handle the case where not all objectives are generated as part of the
post evaluation signal.
Checklist before requesting a review
lintrunner -a(Optional) Issue link